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

[Jon Doron]

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 <address@hidden> 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 <address@hidden>
> > Signed-off-by: Roman Kagan <address@hidden>
> > Signed-off-by: Maciej S. Szmigiero <address@hidden>
> > Signed-off-by: Jon Doron <address@hidden>
> > ---
> >  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.