Re: [PATCH v4 5/6] i386: Hyper-V VMBus ACPI DSDT entry

[Jon Doron <arilou@gmail.com>]

On 05/05/2020, Igor Mammedov wrote:

I dont know what were the original intentions of the original patch 
authors (at this point I simply rebased it, and to be honest I did not 
need this patch to get where I was going to, but it was part of the 
original patchset).

But I'm willing to do any changes so we can keep going forward with 
this.

>On Fri, 24 Apr 2020 15:34:43 +0300
>Jon Doron <arilou@gmail.com> wrote:
>
>> Guest OS uses ACPI to discover VMBus presence.  Add a corresponding
>> entry to DSDT in case VMBus has been enabled.
>>
>> Experimentally Windows guests were found to require this entry to
>> include two IRQ resources. They seem to never be used but they still
>> have to be there.
>>
>> Make IRQ numbers user-configurable via corresponding properties; use 7
>> and 13 by default.
>well, it seems that at least linux guest driver uses one IRQ,
>abeit not from ACPI descriptior
>
>perhaps it's what hyperv host puts into _CRS.
>Could you dump ACPI tables and check how hyperv describes vmbus in acpi?
>
>

I can no longer get to the HyperV computer I had (in the office so 
hopefully if someone else has access to HyperV machine and willing to 
reply here with the dumped ACPI tables that would be great).

>also what if vmbus irq collides with an irq that is already taken,
>it would be better to initialize and consume irqs it climes to use
>so in case if conflict one would get a error.
>

Sounds right. I tried to see if there is any place in acpi that checks 
if an IRQ is taken or not but could not find any, can you point me out 
to a place where it's done?

If not then I guess we need a function that iterates through all 
registered IRQs so we can find if we have a conflict.
Probably the best places to put it is where you build the acpi aml, but 
that would really make the code more complicated (i.e 
build_append_int_noprefix and aml_interrupt).

In case I have not understood you right please let me know.


>> Signed-off-by: Evgeny Yakovlev <eyakovlev@virtuozzo.com>
>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Signed-off-by: Jon Doron <arilou@gmail.com>
>> ---
>>  hw/hyperv/vmbus.c                |  7 ++++++
>>  hw/i386/acpi-build.c             | 43 ++++++++++++++++++++++++++++++++
>>  include/hw/hyperv/vmbus-bridge.h |  3 +++
>>  3 files changed, 53 insertions(+)
>>
>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>> index 1f5873ab60..0df7afe0ca 100644
>> --- a/hw/hyperv/vmbus.c
>> +++ b/hw/hyperv/vmbus.c
>> @@ -2641,6 +2641,12 @@ static const VMStateDescription vmstate_vmbus_bridge = {
>>      },
>>  };
>>
>> +static Property vmbus_bridge_props[] = {
>> +    DEFINE_PROP_UINT8("irq0", VMBusBridge, irq0, 7),
>> +    DEFINE_PROP_UINT8("irq1", VMBusBridge, irq1, 13),
>> +    DEFINE_PROP_END_OF_LIST()
>> +};
>> +
>>  static void vmbus_bridge_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *k = DEVICE_CLASS(klass);
>> @@ -2651,6 +2657,7 @@ static void vmbus_bridge_class_init(ObjectClass *klass, void *data)
>>      sk->explicit_ofw_unit_address = vmbus_bridge_ofw_unit_address;
>>      set_bit(DEVICE_CATEGORY_BRIDGE, k->categories);
>>      k->vmsd = &vmstate_vmbus_bridge;
>> +    device_class_set_props(k, vmbus_bridge_props);
>>      /* override SysBusDevice's default */
>>      k->user_creatable = true;
>>  }
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 2a7e55bae7..d235074fb8 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -50,6 +50,7 @@
>>  #include "hw/mem/nvdimm.h"
>>  #include "sysemu/numa.h"
>>  #include "sysemu/reset.h"
>> +#include "hw/hyperv/vmbus-bridge.h"
>>
>>  /* Supported chipsets: */
>>  #include "hw/southbridge/piix.h"
>> @@ -1270,9 +1271,47 @@ static Aml *build_com_device_aml(uint8_t uid)
>>      return dev;
>>  }
>>
>> +static Aml *build_vmbus_device_aml(VMBusBridge *vmbus_bridge)
>> +{
>> +    Aml *dev;
>> +    Aml *method;
>> +    Aml *crs;
>> +
>> +    dev = aml_device("VMBS");
>> +    aml_append(dev, aml_name_decl("STA", aml_int(0xF)));
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("VMBus")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(0x0)));
>> +    aml_append(dev, aml_name_decl("_DDN", aml_string("VMBUS")));
>> +
>> +    method = aml_method("_DIS", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_store(aml_and(aml_name("STA"), aml_int(0xD), NULL),
>> +                                     aml_name("STA")));
>> +    aml_append(dev, method);
>> +
>> +    method = aml_method("_PS0", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_store(aml_or(aml_name("STA"), aml_int(0xF), NULL),
>> +                                     aml_name("STA")));
>> +    aml_append(dev, method);
>> +
>> +    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>> +    aml_append(method, aml_return(aml_name("STA")));
>> +    aml_append(dev, method);
>
>do you reaaly need all that _STA/_DIS/_PS0,
>does it work without thouse methods?
>
>> +
>> +    aml_append(dev, aml_name_decl("_PS3", aml_int(0x0)));
>should be method
>
>> +
>> +    crs = aml_resource_template();
>> +    aml_append(crs, aml_irq_no_flags(vmbus_bridge->irq0));
>> +    /* FIXME: newer HyperV gets by with only one IRQ */
>then why are you adding the second IRQ, does it work with 1 IRQ?
>
>> +    aml_append(crs, aml_irq_no_flags(vmbus_bridge->irq1));
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +
>> +    return dev;
>> +}
>> +
>>  static void build_isa_devices_aml(Aml *table)
>>  {
>>      ISADevice *fdc = pc_find_fdc0();
>> +    VMBusBridge *vmbus_bridge = vmbus_bridge_find();
>>      bool ambiguous;
>>
>>      Aml *scope = aml_scope("_SB.PCI0.ISA");
>> @@ -1296,6 +1335,10 @@ static void build_isa_devices_aml(Aml *table)
>>          build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
>>      }
>>
>> +    if (vmbus_bridge) {
>> +        aml_append(scope, build_vmbus_device_aml(vmbus_bridge));
>> +    }
>it seems that bridge is sysbus device, why it's put under ISA bus?
>

That's where the original author put it, where would you prefer it will 
go?

>>      aml_append(table, scope);
>>  }
>>
>> diff --git a/include/hw/hyperv/vmbus-bridge.h b/include/hw/hyperv/vmbus-bridge.h
>> index 9cc8f780de..c0a06d832c 100644
>> --- a/include/hw/hyperv/vmbus-bridge.h
>> +++ b/include/hw/hyperv/vmbus-bridge.h
>> @@ -19,6 +19,9 @@ typedef struct VMBus VMBus;
>>  typedef struct VMBusBridge {
>>      SysBusDevice parent_obj;
>>
>> +    uint8_t irq0;
>> +    uint8_t irq1;
>> +
>>      VMBus *bus;
>>  } VMBusBridge;
>>
>

For your other questions I have no clue like I said I have only rebased 
the latest revision (which was about 1-2 years old).

-- Jon.