| Message ID | 20200424123444.3481728-6-arilou@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hyperv: VMBus implementation | expand |
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? 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. > 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? > 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; >
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.
On 05.05.2020 17:38, Jon Doron wrote: > 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). > Here is a VMBus ACPI device description from Hyper-V in Windows Server 2019: Device (\_SB.VMOD.VMBS) { Name (STA, 0x0F) Name (_ADR, Zero) // _ADR: Address Name (_DDN, "VMBUS") // _DDN: DOS Device Name Name (_HID, "VMBus") // _HID: Hardware ID Name (_UID, Zero) // _UID: Unique ID Method (_DIS, 0, NotSerialized) // _DIS: Disable Device { STA &= 0x0D } Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 { STA |= 0x0F } Method (_STA, 0, NotSerialized) // _STA: Status { Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ } Name (_PS3, Zero) // _PS3: Power State 3 Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IRQ (Edge, ActiveHigh, Exclusive, ) {5} }) } It seems to use just IRQ 5. Maciej
Thank you Maciej :) Igor it seems like the IRQ being used is 5 and not 7 & 13 like in the current patch. Seems like it needs to reside in the _CRS like you said. Seems like it has all those _STA/_DIS/_PS0 just like the way it's currently in the patch (unless I'm missing something). Notice _PS3 is not a Method. So just to summarize the changes i need to do: 1. Change from 2 IRQs to single one (and use 5 as the default) 2. IRQs needs to be under _CRS. 3. You mentioned you want under a different location than the ISA bug where would you want it to be? Please let me know if there is anything else. Thanks, -- Jon. On 06/05/2020, Maciej S. Szmigiero wrote: >On 05.05.2020 17:38, Jon Doron wrote: >> 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). >> > >Here is a VMBus ACPI device description from Hyper-V in Windows Server 2019: > >Device (\_SB.VMOD.VMBS) >{ > Name (STA, 0x0F) > Name (_ADR, Zero) // _ADR: Address > Name (_DDN, "VMBUS") // _DDN: DOS Device Name > Name (_HID, "VMBus") // _HID: Hardware ID > Name (_UID, Zero) // _UID: Unique ID > Method (_DIS, 0, NotSerialized) // _DIS: Disable Device > { > STA &= 0x0D > } > > Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 > { > STA |= 0x0F > } > > Method (_STA, 0, NotSerialized) // _STA: Status > { > Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ > } > > Name (_PS3, Zero) // _PS3: Power State 3 > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings > { > IRQ (Edge, ActiveHigh, Exclusive, ) > {5} > }) >} > >It seems to use just IRQ 5. > >Maciej
On Tue, May 05, 2020 at 03:06:37PM +0200, Igor Mammedov wrote: > 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 I guess you mean synthetic interrupts. Linux doesn't seem to use ACPI-discovered IRQs. > perhaps it's what hyperv host puts into _CRS. > Could you dump ACPI tables and check how hyperv describes vmbus in acpi? Exactly, this was how this was conceived in the first place. > 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. That was the plan initially. However, since no guest actually used those irqs, it appeared not worth the effort. Dunno what problems can arise from the conflicts. > > 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? This was just copied from HyperV. It may make sense to test without. > > + > > + aml_append(dev, aml_name_decl("_PS3", aml_int(0x0))); > should be method Not our fault :) Again this was copied. > > + > > + 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? This FIXME was left by me when I noticed that more recent HyperV servers only stick one IRQ there, but I didn't get around to dig further. > > + 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? This is where it's put by HyperV in Gen1 machines. For Gen2, which has neither PCI nor ISA, it's put in a dedicated container. Either way, the ancestor nodes are consulted for the memory windows for certain devices like vmbus-pci bridges or vmbus framebuffer, so it may be necessary to adjust this when / if we add those devices. Thanks, Roman. > > 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; > > > >
On Thu, May 07, 2020 at 06:14:25AM +0300, Jon Doron wrote: > Igor it seems like the IRQ being used is 5 and not 7 & 13 like in the > current patch. HyperV using irq 5 doesn't mean QEMU has to too. Especially so as no guest was noticed to use the irqs in ACPI. I'd rather try and test if the guest requires any those at all. > Seems like it needs to reside in the _CRS like you said. They already are there. > Seems like it has all those _STA/_DIS/_PS0 just like the way it's currently > in the patch (unless I'm missing something). Right, but, as you can see, they are pretty dumb, so the question is whether they are necessary or the guests can do without (Linux apparently can). Thanks, Roman. > Notice _PS3 is not a Method. > > So just to summarize the changes i need to do: > 1. Change from 2 IRQs to single one (and use 5 as the default) > 2. IRQs needs to be under _CRS. > 3. You mentioned you want under a different location than the ISA bug where > would you want it to be? > > Please let me know if there is anything else. > > Thanks, > -- Jon. > > On 06/05/2020, Maciej S. Szmigiero wrote: > > On 05.05.2020 17:38, Jon Doron wrote: > > > 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). > > > > > > > Here is a VMBus ACPI device description from Hyper-V in Windows Server 2019: > > > > Device (\_SB.VMOD.VMBS) > > { > > Name (STA, 0x0F) > > Name (_ADR, Zero) // _ADR: Address > > Name (_DDN, "VMBUS") // _DDN: DOS Device Name > > Name (_HID, "VMBus") // _HID: Hardware ID > > Name (_UID, Zero) // _UID: Unique ID > > Method (_DIS, 0, NotSerialized) // _DIS: Disable Device > > { > > STA &= 0x0D > > } > > > > Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 > > { > > STA |= 0x0F > > } > > > > Method (_STA, 0, NotSerialized) // _STA: Status > > { > > Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ > > } > > > > Name (_PS3, Zero) // _PS3: Power State 3 > > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings > > { > > IRQ (Edge, ActiveHigh, Exclusive, ) > > {5} > > }) > > } > > > > It seems to use just IRQ 5. > > > > Maciej >
On Mon, 11 May 2020 21:21:56 +0300 Roman Kagan <rvkagan@yandex-team.ru> wrote: > On Tue, May 05, 2020 at 03:06:37PM +0200, Igor Mammedov wrote: > > 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 > > I guess you mean synthetic interrupts. Linux doesn't seem to use > ACPI-discovered IRQs. indeed it doesn't, but that doesn't mean that Windows doesn't as well. > > perhaps it's what hyperv host puts into _CRS. > > Could you dump ACPI tables and check how hyperv describes vmbus in acpi? > > Exactly, this was how this was conceived in the first place. > > > 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. > > That was the plan initially. However, since no guest actually used > those irqs, it appeared not worth the effort. Dunno what problems can > arise from the conflicts. I'd rather avoid using random IRQ numbers (considering we are dealing with black-box here). So if it's really necessary to have IRQ described here, I'd suggest to implement them in device model so they would be reserved and QEMU would error out in a sane way if IRQ conflict is detected. > > > 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? > > This was just copied from HyperV. It may make sense to test without. > > > > + > > > + aml_append(dev, aml_name_decl("_PS3", aml_int(0x0))); > > should be method > > Not our fault :) Again this was copied. > > > > + > > > + 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? > > This FIXME was left by me when I noticed that more recent HyperV servers > only stick one IRQ there, but I didn't get around to dig further. > > > > + 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? > > This is where it's put by HyperV in Gen1 machines. For Gen2, which has > neither PCI nor ISA, it's put in a dedicated container. > > Either way, the ancestor nodes are consulted for the memory windows for > certain devices like vmbus-pci bridges or vmbus framebuffer, so it may > be necessary to adjust this when / if we add those devices. > > Thanks, > Roman. > > > > 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; > > > > > > > >
On Mon, 11 May 2020 23:11:23 +0300 Roman Kagan <rvkagan@yandex-team.ru> wrote: > On Thu, May 07, 2020 at 06:14:25AM +0300, Jon Doron wrote: > > Igor it seems like the IRQ being used is 5 and not 7 & 13 like in the > > current patch. > > HyperV using irq 5 doesn't mean QEMU has to too. Especially so as no > guest was noticed to use the irqs in ACPI. I'd rather try and test if > the guest requires any those at all. > > > Seems like it needs to reside in the _CRS like you said. > > They already are there. > > > Seems like it has all those _STA/_DIS/_PS0 just like the way it's currently > > in the patch (unless I'm missing something). > > Right, but, as you can see, they are pretty dumb, so the question is > whether they are necessary or the guests can do without (Linux > apparently can). Agreed with all of above, Instead of blind copying dubious AML, we should try to figure out what's really necessary of it and throw away the rest. > > Thanks, > Roman. > > > Notice _PS3 is not a Method. > > > > So just to summarize the changes i need to do: > > 1. Change from 2 IRQs to single one (and use 5 as the default) > > 2. IRQs needs to be under _CRS. > > 3. You mentioned you want under a different location than the ISA bug where > > would you want it to be? > > > > Please let me know if there is anything else. > > > > Thanks, > > -- Jon. > > > > On 06/05/2020, Maciej S. Szmigiero wrote: > > > On 05.05.2020 17:38, Jon Doron wrote: > > > > 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). > > > > > > > > > > Here is a VMBus ACPI device description from Hyper-V in Windows Server 2019: > > > > > > Device (\_SB.VMOD.VMBS) > > > { > > > Name (STA, 0x0F) > > > Name (_ADR, Zero) // _ADR: Address > > > Name (_DDN, "VMBUS") // _DDN: DOS Device Name > > > Name (_HID, "VMBus") // _HID: Hardware ID > > > Name (_UID, Zero) // _UID: Unique ID > > > Method (_DIS, 0, NotSerialized) // _DIS: Disable Device > > > { > > > STA &= 0x0D > > > } > > > > > > Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 > > > { > > > STA |= 0x0F > > > } > > > > > > Method (_STA, 0, NotSerialized) // _STA: Status > > > { > > > Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ > > > } > > > > > > Name (_PS3, Zero) // _PS3: Power State 3 > > > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings > > > { > > > IRQ (Edge, ActiveHigh, Exclusive, ) > > > {5} > > > }) > > > } > > > > > > It seems to use just IRQ 5. > > > > > > Maciej > > >
On 13/05/2020, Igor Mammedov wrote: Do you guys know perhaps which module is reading the ACPI configuration for VMBus? vmbus.sys / vmbkmcl.sys / winhv.sys? is it the kernel or HAL? I dont have any real HyperV Windows to play with... Roman, do you remember when is this information being used? Do we need a full emulation setup (aka your hv-scsi / hv-net) patches in order to see Windows working this ACPI entry? Thanks, -- Jon. >On Mon, 11 May 2020 23:11:23 +0300 >Roman Kagan <rvkagan@yandex-team.ru> wrote: > >> On Thu, May 07, 2020 at 06:14:25AM +0300, Jon Doron wrote: >> > Igor it seems like the IRQ being used is 5 and not 7 & 13 like in the >> > current patch. >> >> HyperV using irq 5 doesn't mean QEMU has to too. Especially so as no >> guest was noticed to use the irqs in ACPI. I'd rather try and test if >> the guest requires any those at all. >> >> > Seems like it needs to reside in the _CRS like you said. >> >> They already are there. >> >> > Seems like it has all those _STA/_DIS/_PS0 just like the way it's currently >> > in the patch (unless I'm missing something). >> >> Right, but, as you can see, they are pretty dumb, so the question is >> whether they are necessary or the guests can do without (Linux >> apparently can). > >Agreed with all of above, >Instead of blind copying dubious AML, we should try to figure out what's >really necessary of it and throw away the rest. > >> >> Thanks, >> Roman. >> >> > Notice _PS3 is not a Method. >> > >> > So just to summarize the changes i need to do: >> > 1. Change from 2 IRQs to single one (and use 5 as the default) >> > 2. IRQs needs to be under _CRS. >> > 3. You mentioned you want under a different location than the ISA bug where >> > would you want it to be? >> > >> > Please let me know if there is anything else. >> > >> > Thanks, >> > -- Jon. >> > >> > On 06/05/2020, Maciej S. Szmigiero wrote: >> > > On 05.05.2020 17:38, Jon Doron wrote: >> > > > 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). >> > > > >> > > >> > > Here is a VMBus ACPI device description from Hyper-V in Windows Server 2019: >> > > >> > > Device (\_SB.VMOD.VMBS) >> > > { >> > > Name (STA, 0x0F) >> > > Name (_ADR, Zero) // _ADR: Address >> > > Name (_DDN, "VMBUS") // _DDN: DOS Device Name >> > > Name (_HID, "VMBus") // _HID: Hardware ID >> > > Name (_UID, Zero) // _UID: Unique ID >> > > Method (_DIS, 0, NotSerialized) // _DIS: Disable Device >> > > { >> > > STA &= 0x0D >> > > } >> > > >> > > Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 >> > > { >> > > STA |= 0x0F >> > > } >> > > >> > > Method (_STA, 0, NotSerialized) // _STA: Status >> > > { >> > > Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ >> > > } >> > > >> > > Name (_PS3, Zero) // _PS3: Power State 3 >> > > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings >> > > { >> > > IRQ (Edge, ActiveHigh, Exclusive, ) >> > > {5} >> > > }) >> > > } >> > > >> > > It seems to use just IRQ 5. >> > > >> > > Maciej >> > >> >
On Fri, May 15, 2020 at 11:56:36AM +0300, Jon Doron wrote: > On 13/05/2020, Igor Mammedov wrote: > > Do you guys know perhaps which module is reading the ACPI configuration for > VMBus? vmbus.sys / vmbkmcl.sys / winhv.sys? is it the kernel or HAL? No idea... > I dont have any real HyperV Windows to play with... > > Roman, do you remember when is this information being used? Do we need a > full emulation setup (aka your hv-scsi / hv-net) patches in order to see > Windows working this ACPI entry? I think the series you've posted should be enough to see VMBus in the DeviceManager. hv-scsi and hv-net should not further depend on ACPI. hv-pci does: it (at least in its Linux driver implementation) wants the VMBus entry or any of its ancestors to define range(s) where mmio windows can be placed. This may be worth considering when deciding where to stick the VMBus entry to. Thanks, Roman. > Thanks, > -- Jon. > > > On Mon, 11 May 2020 23:11:23 +0300 > > Roman Kagan <rvkagan@yandex-team.ru> wrote: > > > > > On Thu, May 07, 2020 at 06:14:25AM +0300, Jon Doron wrote: > > > > Igor it seems like the IRQ being used is 5 and not 7 & 13 like in the > > > > current patch. > > > > > > HyperV using irq 5 doesn't mean QEMU has to too. Especially so as no > > > guest was noticed to use the irqs in ACPI. I'd rather try and test if > > > the guest requires any those at all. > > > > > > > Seems like it needs to reside in the _CRS like you said. > > > > > > They already are there. > > > > > > > Seems like it has all those _STA/_DIS/_PS0 just like the way it's currently > > > > in the patch (unless I'm missing something). > > > > > > Right, but, as you can see, they are pretty dumb, so the question is > > > whether they are necessary or the guests can do without (Linux > > > apparently can). > > > > Agreed with all of above, > > Instead of blind copying dubious AML, we should try to figure out what's > > really necessary of it and throw away the rest. > > > > > > > > Thanks, > > > Roman. > > > > > > > Notice _PS3 is not a Method. > > > > > > > > So just to summarize the changes i need to do: > > > > 1. Change from 2 IRQs to single one (and use 5 as the default) > > > > 2. IRQs needs to be under _CRS. > > > > 3. You mentioned you want under a different location than the ISA bug where > > > > would you want it to be? > > > > > > > > Please let me know if there is anything else. > > > > > > > > Thanks, > > > > -- Jon. > > > > > > > > On 06/05/2020, Maciej S. Szmigiero wrote: > > > > > On 05.05.2020 17:38, Jon Doron wrote: > > > > > > 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). > > > > > > > > > > > > > > > > Here is a VMBus ACPI device description from Hyper-V in Windows Server 2019: > > > > > > > > > > Device (\_SB.VMOD.VMBS) > > > > > { > > > > > Name (STA, 0x0F) > > > > > Name (_ADR, Zero) // _ADR: Address > > > > > Name (_DDN, "VMBUS") // _DDN: DOS Device Name > > > > > Name (_HID, "VMBus") // _HID: Hardware ID > > > > > Name (_UID, Zero) // _UID: Unique ID > > > > > Method (_DIS, 0, NotSerialized) // _DIS: Disable Device > > > > > { > > > > > STA &= 0x0D > > > > > } > > > > > > > > > > Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 > > > > > { > > > > > STA |= 0x0F > > > > > } > > > > > > > > > > Method (_STA, 0, NotSerialized) // _STA: Status > > > > > { > > > > > Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ > > > > > } > > > > > > > > > > Name (_PS3, Zero) // _PS3: Power State 3 > > > > > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings > > > > > { > > > > > IRQ (Edge, ActiveHigh, Exclusive, ) > > > > > {5} > > > > > }) > > > > > } > > > > > > > > > > It seems to use just IRQ 5. > > > > > > > > > > Maciej > > > > > > > > >
On 13/05/20 17:34, Igor Mammedov wrote: > I'd rather avoid using random IRQ numbers (considering we are dealing with > black-box here). So if it's really necessary to have IRQ described here, > I'd suggest to implement them in device model so they would be reserved > and QEMU would error out in a sane way if IRQ conflict is detected. We don't generally detect ISA IRQ conflicts though, do we? Paolo
On Thu, 21 May 2020 18:02:07 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 13/05/20 17:34, Igor Mammedov wrote: > > I'd rather avoid using random IRQ numbers (considering we are > > dealing with black-box here). So if it's really necessary to have > > IRQ described here, I'd suggest to implement them in device model > > so they would be reserved and QEMU would error out in a sane way if > > IRQ conflict is detected. > > We don't generally detect ISA IRQ conflicts though, do we? that I don't know that's why I'm not suggesting how to do it. The point is hard-coding in AML random IRQs is not right thing to do, (especially with the lack of 'any' spec), as minimum AML should pull it from device model and that probably should be configurable and set by board. Other thing is: I haven't looked at VMBus device model in detail, but DSDT part aren't matching device though (device model is not ISA device hence AML part shouldn't be on in ISA scope), where to put it is open question. There were other issues with AML code, I've commented on, so I was waiting on respin with comments addressed. I don't think that this patch is good enough for merging. > > Paolo >
On 22/05/2020, Igor Mammedow wrote: >On Thu, 21 May 2020 18:02:07 +0200 >Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 13/05/20 17:34, Igor Mammedov wrote: >> > I'd rather avoid using random IRQ numbers (considering we are >> > dealing with black-box here). So if it's really necessary to have >> > IRQ described here, I'd suggest to implement them in device model >> > so they would be reserved and QEMU would error out in a sane way if >> > IRQ conflict is detected. >> >> We don't generally detect ISA IRQ conflicts though, do we? > >that I don't know that's why I'm not suggesting how to do it. >The point is hard-coding in AML random IRQs is not right thing to do, >(especially with the lack of 'any' spec), as minimum AML should pull >it from device model and that probably should be configurable and set >by board. > >Other thing is: >I haven't looked at VMBus device model in detail, but DSDT part aren't >matching device though (device model is not ISA device hence AML part >shouldn't be on in ISA scope), where to put it is open question. >There were other issues with AML code, I've commented on, so I was >waiting on respin with comments addressed. >I don't think that this patch is good enough for merging. > > But it seems like the current patch does match what's Microsoft HyperV is publishing in it's APCI tables. I dont think it's correct for us to "fix" Microsoft emulation even if it's wrong, since that's what Windows probably expects to see... I tried looking where Microsoft uses the ACPI tables to identify the VMBus but without much luck in order to understand how flexible a change would be for the OS to still detect the VMBus device, but in general I think "correcting" something that is emulated 1:1 because there is no spec is the right way. >> >> Paolo >> >
On 28/05/2020, Jon Doron wrote: >On 22/05/2020, Igor Mammedow wrote: >>On Thu, 21 May 2020 18:02:07 +0200 >>Paolo Bonzini <pbonzini@redhat.com> wrote: >> >>>On 13/05/20 17:34, Igor Mammedov wrote: >>>> I'd rather avoid using random IRQ numbers (considering we are >>>> dealing with black-box here). So if it's really necessary to have >>>> IRQ described here, I'd suggest to implement them in device model >>>> so they would be reserved and QEMU would error out in a sane way if >>>> IRQ conflict is detected. >>> >>>We don't generally detect ISA IRQ conflicts though, do we? >> >>that I don't know that's why I'm not suggesting how to do it. >>The point is hard-coding in AML random IRQs is not right thing to do, >>(especially with the lack of 'any' spec), as minimum AML should pull >>it from device model and that probably should be configurable and set >>by board. >> >>Other thing is: >>I haven't looked at VMBus device model in detail, but DSDT part aren't >>matching device though (device model is not ISA device hence AML part >>shouldn't be on in ISA scope), where to put it is open question. >>There were other issues with AML code, I've commented on, so I was >>waiting on respin with comments addressed. >>I don't think that this patch is good enough for merging. >> >> > >But it seems like the current patch does match what's Microsoft HyperV >is publishing in it's APCI tables. > >I dont think it's correct for us to "fix" Microsoft emulation even if >it's wrong, since that's what Windows probably expects to see... > >I tried looking where Microsoft uses the ACPI tables to identify the >VMBus but without much luck in order to understand how flexible a >change would be for the OS to still detect the VMBus device, but in >general I think "correcting" something that is emulated 1:1 because >there is no spec is the right way. > Bah sorry meant to say: In general correcting some virtual emulated device which is current is matching 1:1 is wrong, I think the right way to handle this type of things is to copy them 1:1 and not try to "fix" or "correct" them since that's what Windows expects. >>> >>>Paolo >>> >>
On Thu, 28 May 2020 08:26:42 +0300 Jon Doron <arilou@gmail.com> wrote: > On 22/05/2020, Igor Mammedow wrote: > >On Thu, 21 May 2020 18:02:07 +0200 > >Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> On 13/05/20 17:34, Igor Mammedov wrote: > >> > I'd rather avoid using random IRQ numbers (considering we are > >> > dealing with black-box here). So if it's really necessary to have > >> > IRQ described here, I'd suggest to implement them in device model > >> > so they would be reserved and QEMU would error out in a sane way if > >> > IRQ conflict is detected. > >> > >> We don't generally detect ISA IRQ conflicts though, do we? > > > >that I don't know that's why I'm not suggesting how to do it. > >The point is hard-coding in AML random IRQs is not right thing to do, > >(especially with the lack of 'any' spec), as minimum AML should pull > >it from device model and that probably should be configurable and set > >by board. > > > >Other thing is: > >I haven't looked at VMBus device model in detail, but DSDT part aren't > >matching device though (device model is not ISA device hence AML part > >shouldn't be on in ISA scope), where to put it is open question. > >There were other issues with AML code, I've commented on, so I was > >waiting on respin with comments addressed. > >I don't think that this patch is good enough for merging. > > > > > > But it seems like the current patch does match what's Microsoft HyperV > is publishing in it's APCI tables. > > I dont think it's correct for us to "fix" Microsoft emulation even if > it's wrong, since that's what Windows probably expects to see... > > I tried looking where Microsoft uses the ACPI tables to identify the > VMBus but without much luck in order to understand how flexible a change > would be for the OS to still detect the VMBus device, but in general > I think "correcting" something that is emulated 1:1 because there is no > spec is the right way. I'd agree, if removing nonsense would break VMBus detection (does it?). if something is that doesn't make sense but has to stay because it is need to make windows happy, that's fine , just add annotate is with comment, so it won't confuse anyone why that code exists there later on. I suggest to: 1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong 2. drop one IRQ, newer hyper-v seems to be doing fine with only one 3. it's not ISA device, I'd suggest to move into _SB scope 4. I don't know much about IRQs but git grep DEFINE_PROP_ | grep -i iqr yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). So I'd leave it upto Paolo or someone else to decide/comment on. > > >> > >> Paolo > >> > > >
On 28/05/2020, Igor Mammedov wrote: >On Thu, 28 May 2020 08:26:42 +0300 >Jon Doron <arilou@gmail.com> wrote: > >> On 22/05/2020, Igor Mammedow wrote: >> >On Thu, 21 May 2020 18:02:07 +0200 >> >Paolo Bonzini <pbonzini@redhat.com> wrote: >> > >> >> On 13/05/20 17:34, Igor Mammedov wrote: >> >> > I'd rather avoid using random IRQ numbers (considering we are >> >> > dealing with black-box here). So if it's really necessary to have >> >> > IRQ described here, I'd suggest to implement them in device model >> >> > so they would be reserved and QEMU would error out in a sane way if >> >> > IRQ conflict is detected. >> >> >> >> We don't generally detect ISA IRQ conflicts though, do we? >> > >> >that I don't know that's why I'm not suggesting how to do it. >> >The point is hard-coding in AML random IRQs is not right thing to do, >> >(especially with the lack of 'any' spec), as minimum AML should pull >> >it from device model and that probably should be configurable and set >> >by board. >> > >> >Other thing is: >> >I haven't looked at VMBus device model in detail, but DSDT part aren't >> >matching device though (device model is not ISA device hence AML part >> >shouldn't be on in ISA scope), where to put it is open question. >> >There were other issues with AML code, I've commented on, so I was >> >waiting on respin with comments addressed. >> >I don't think that this patch is good enough for merging. >> > >> > >> >> But it seems like the current patch does match what's Microsoft HyperV >> is publishing in it's APCI tables. >> >> I dont think it's correct for us to "fix" Microsoft emulation even if >> it's wrong, since that's what Windows probably expects to see... >> >> I tried looking where Microsoft uses the ACPI tables to identify the >> VMBus but without much luck in order to understand how flexible a change >> would be for the OS to still detect the VMBus device, but in general >> I think "correcting" something that is emulated 1:1 because there is no >> spec is the right way. > >I'd agree, if removing nonsense would break VMBus detection (does it?). >if something is that doesn't make sense but has to stay because it is need >to make windows happy, that's fine , just add annotate is with comment, >so it won't confuse anyone why that code exists there later on. > >I suggest to: > 1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong > 2. drop one IRQ, newer hyper-v seems to be doing fine with only one > 3. it's not ISA device, I'd suggest to move into _SB scope > 4. I don't know much about IRQs but > git grep DEFINE_PROP_ | grep -i iqr > yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns > IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). > So I'd leave it upto Paolo or someone else to decide/comment on. > Sounds like a plan, I'll try to come up with the test results (at least for Windows 10 guest which is what I have setup) and update this thread with the results. -- Jon. >> >> >> >> >> Paolo >> >> >> > >> >
On 28/05/2020, Jon Doron wrote: >On 28/05/2020, Igor Mammedov wrote: >>On Thu, 28 May 2020 08:26:42 +0300 >>Jon Doron <arilou@gmail.com> wrote: >> >>>On 22/05/2020, Igor Mammedow wrote: >>>>On Thu, 21 May 2020 18:02:07 +0200 >>>>Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>>>> On 13/05/20 17:34, Igor Mammedov wrote: >>>>> > I'd rather avoid using random IRQ numbers (considering we are >>>>> > dealing with black-box here). So if it's really necessary to have >>>>> > IRQ described here, I'd suggest to implement them in device model >>>>> > so they would be reserved and QEMU would error out in a sane way if >>>>> > IRQ conflict is detected. >>>>> >>>>> We don't generally detect ISA IRQ conflicts though, do we? >>>> >>>>that I don't know that's why I'm not suggesting how to do it. >>>>The point is hard-coding in AML random IRQs is not right thing to do, >>>>(especially with the lack of 'any' spec), as minimum AML should pull >>>>it from device model and that probably should be configurable and set >>>>by board. >>>> >>>>Other thing is: >>>>I haven't looked at VMBus device model in detail, but DSDT part aren't >>>>matching device though (device model is not ISA device hence AML part >>>>shouldn't be on in ISA scope), where to put it is open question. >>>>There were other issues with AML code, I've commented on, so I was >>>>waiting on respin with comments addressed. >>>>I don't think that this patch is good enough for merging. >>>> >>>> >>> >>>But it seems like the current patch does match what's Microsoft HyperV >>>is publishing in it's APCI tables. >>> >>>I dont think it's correct for us to "fix" Microsoft emulation even if >>>it's wrong, since that's what Windows probably expects to see... >>> >>>I tried looking where Microsoft uses the ACPI tables to identify the >>>VMBus but without much luck in order to understand how flexible a change >>>would be for the OS to still detect the VMBus device, but in general >>>I think "correcting" something that is emulated 1:1 because there is no >>>spec is the right way. >> >>I'd agree, if removing nonsense would break VMBus detection (does it?). >>if something is that doesn't make sense but has to stay because it is need >>to make windows happy, that's fine , just add annotate is with comment, >>so it won't confuse anyone why that code exists there later on. >> >>I suggest to: >>1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong >>2. drop one IRQ, newer hyper-v seems to be doing fine with only one >>3. it's not ISA device, I'd suggest to move into _SB scope >>4. I don't know much about IRQs but >> git grep DEFINE_PROP_ | grep -i iqr >> yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns >> IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). >> So I'd leave it upto Paolo or someone else to decide/comment on. >> > >Sounds like a plan, I'll try to come up with the test results >(at least for Windows 10 guest which is what I have setup) and update >this thread with the results. > >-- Jon. > >>> >>>>> >>>>> Paolo >>>>> >>>> >>> >> Hi guys, Sorry for the delay... So first ill clarify what was the test, the test was to see the device "Microsoft Hyper-V Virtual Machine Bus" in Windows Device Manager under "System devices" with a state of "working properly". It seems like it's ok to drop all the _PS* and _STA. It seems to be functioning with single IRQ as well, it is worth noting that even when i dropped the entire _CRS (so no IRQs resources are required, the device was still showing that it's functioning, but I suspect this might affect the child devices like hv-net and hv-scsi). With that said I did run into a small issue I set-up Win10 1903 (aka 19H1) and it seems like VMBus now requires to have the following features enabled: HV_VP_RUNTIME_AVAILABLE HV_TIME_REF_COUNT_AVAILABLE HV_SYNIC_AVAILABLE HV_SYNTIMERS_AVAILABLE HV_APIC_ACCESS_AVAILABLE HV_HYPERCALL_AVAILABLE HV_VP_INDEX_AVAILABLE So notice that previously only SYNIC and VPINDEX was needed, now you need the whole thing so you need to run qemu with something like -cpu host,hv-relaxed,hv_spinlocks=0x1fff,hv_time,hv-vapic,hv-vpindex,hv-synic,hv-runtime,hv-stimer The validation was done in winhv!WinHvpCheckPartitionPrivileges . Paolo I noticed you have done a PULL request, would you like to wait on it and we will submit a version with a single IRQ (selectable by user property) and go with Igor's suggestion dropping _PS* and _STA (though like I said before I prefer to mimic the original HyperV with it's bugs, but I'll leave this decision to you). Also today VMBus only verifies SYNIC is enabled I'm not sure how but I wonder if we want to some how exports from the CPU which other HV features are enabled so we can verify all the required ones are set, would appreciate if you have any suggestions here. Cheers, -- Jon.
On 14/06/2020, Jon Doron wrote: >On 28/05/2020, Jon Doron wrote: >>On 28/05/2020, Igor Mammedov wrote: >>>On Thu, 28 May 2020 08:26:42 +0300 >>>Jon Doron <arilou@gmail.com> wrote: >>> >>>>On 22/05/2020, Igor Mammedow wrote: >>>>>On Thu, 21 May 2020 18:02:07 +0200 >>>>>Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>> >>>>>>On 13/05/20 17:34, Igor Mammedov wrote: >>>>>>> I'd rather avoid using random IRQ numbers (considering we are >>>>>>> dealing with black-box here). So if it's really necessary to have >>>>>>> IRQ described here, I'd suggest to implement them in device model >>>>>>> so they would be reserved and QEMU would error out in a sane way if >>>>>>> IRQ conflict is detected. >>>>>> >>>>>>We don't generally detect ISA IRQ conflicts though, do we? >>>>> >>>>>that I don't know that's why I'm not suggesting how to do it. >>>>>The point is hard-coding in AML random IRQs is not right thing to do, >>>>>(especially with the lack of 'any' spec), as minimum AML should pull >>>>>it from device model and that probably should be configurable and set >>>>>by board. >>>>> >>>>>Other thing is: >>>>>I haven't looked at VMBus device model in detail, but DSDT part aren't >>>>>matching device though (device model is not ISA device hence AML part >>>>>shouldn't be on in ISA scope), where to put it is open question. >>>>>There were other issues with AML code, I've commented on, so I was >>>>>waiting on respin with comments addressed. >>>>>I don't think that this patch is good enough for merging. >>>>> >>>>> >>>> >>>>But it seems like the current patch does match what's Microsoft HyperV >>>>is publishing in it's APCI tables. >>>> >>>>I dont think it's correct for us to "fix" Microsoft emulation even if >>>>it's wrong, since that's what Windows probably expects to see... >>>> >>>>I tried looking where Microsoft uses the ACPI tables to identify the >>>>VMBus but without much luck in order to understand how flexible a change >>>>would be for the OS to still detect the VMBus device, but in general >>>>I think "correcting" something that is emulated 1:1 because there is no >>>>spec is the right way. >>> >>>I'd agree, if removing nonsense would break VMBus detection (does it?). >>>if something is that doesn't make sense but has to stay because it is need >>>to make windows happy, that's fine , just add annotate is with comment, >>>so it won't confuse anyone why that code exists there later on. >>> >>>I suggest to: >>>1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong >>>2. drop one IRQ, newer hyper-v seems to be doing fine with only one >>>3. it's not ISA device, I'd suggest to move into _SB scope >>>4. I don't know much about IRQs but >>> git grep DEFINE_PROP_ | grep -i iqr >>> yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns >>> IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). >>> So I'd leave it upto Paolo or someone else to decide/comment on. >>> >> >>Sounds like a plan, I'll try to come up with the test results >>(at least for Windows 10 guest which is what I have setup) and update >>this thread with the results. >> >>-- Jon. >> >>>> >>>>>> >>>>>>Paolo >>>>>> >>>>> >>>> >>> >Hi guys, > >Sorry for the delay... > >So first ill clarify what was the test, the test was to see the device >"Microsoft Hyper-V Virtual Machine Bus" in Windows Device Manager under >"System devices" with a state of "working properly". > >It seems like it's ok to drop all the _PS* and _STA. > >It seems to be functioning with single IRQ as well, it is worth noting >that even when i dropped the entire _CRS (so no IRQs resources are >required, the device was still showing that it's functioning, but I >suspect this might affect the child devices like hv-net and hv-scsi). > >With that said I did run into a small issue I set-up Win10 1903 (aka >19H1) and it seems like VMBus now requires to have the following >features enabled: >HV_VP_RUNTIME_AVAILABLE >HV_TIME_REF_COUNT_AVAILABLE >HV_SYNIC_AVAILABLE >HV_SYNTIMERS_AVAILABLE >HV_APIC_ACCESS_AVAILABLE >HV_HYPERCALL_AVAILABLE >HV_VP_INDEX_AVAILABLE > >So notice that previously only SYNIC and VPINDEX was needed, now you >need the whole thing so you need to run qemu with something like >-cpu host,hv-relaxed,hv_spinlocks=0x1fff,hv_time,hv-vapic,hv-vpindex,hv-synic,hv-runtime,hv-stimer > >The validation was done in winhv!WinHvpCheckPartitionPrivileges . > >Paolo I noticed you have done a PULL request, would you like to wait >on it and we will submit a version with a single IRQ (selectable by >user property) and go with Igor's suggestion dropping _PS* and _STA >(though like I said before I prefer to mimic the original HyperV with >it's bugs, but I'll leave this decision to you). > >Also today VMBus only verifies SYNIC is enabled I'm not sure how but I >wonder if we want to some how exports from the CPU which other HV >features are enabled so we can verify all the required ones are set, >would appreciate if you have any suggestions here. > >Cheers, >-- Jon. I got the latest DSDT from one of the latest builds 19041 Device (\_SB.VMOD.VMBS) { Name (STA, 0x0F) Name (_ADR, Zero) // _ADR: Address Name (_DDN, "VMBUS") // _DDN: DOS Device Name Name (_HID, "VMBus") // _HID: Hardware ID Name (_UID, Zero) // _UID: Unique ID Method (_DIS, 0, NotSerialized) // _DIS: Disable Device { STA &= 0x0D } Method (_PS0, 0, NotSerialized) // _PS0: Power State 0 { STA |= 0x0F } Method (_STA, 0, NotSerialized) // _STA: Status { Return (STA) /* \_SB_.VMOD.VMBS.STA_ */ } Name (_PS3, Zero) // _PS3: Power State 3 Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { IRQ (Edge, ActiveHigh, Exclusive, ) {5} }) } So things looks the same...
Hi Jon, On 14.06.2020 16:11, Jon Doron wrote: > On 28/05/2020, Jon Doron wrote: >> On 28/05/2020, Igor Mammedov wrote: >>> On Thu, 28 May 2020 08:26:42 +0300 >>> Jon Doron <arilou@gmail.com> wrote: >>> >>>> On 22/05/2020, Igor Mammedow wrote: >>>>> On Thu, 21 May 2020 18:02:07 +0200 >>>>> Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>> >>>>>> On 13/05/20 17:34, Igor Mammedov wrote: >>>>>> > I'd rather avoid using random IRQ numbers (considering we are >>>>>> > dealing with black-box here). So if it's really necessary to have >>>>>> > IRQ described here, I'd suggest to implement them in device model >>>>>> > so they would be reserved and QEMU would error out in a sane way if >>>>>> > IRQ conflict is detected. >>>>>> >>>>>> We don't generally detect ISA IRQ conflicts though, do we? >>>>> >>>>> that I don't know that's why I'm not suggesting how to do it. >>>>> The point is hard-coding in AML random IRQs is not right thing to do, >>>>> (especially with the lack of 'any' spec), as minimum AML should pull >>>>> it from device model and that probably should be configurable and set >>>>> by board. >>>>> >>>>> Other thing is: >>>>> I haven't looked at VMBus device model in detail, but DSDT part aren't >>>>> matching device though (device model is not ISA device hence AML part >>>>> shouldn't be on in ISA scope), where to put it is open question. >>>>> There were other issues with AML code, I've commented on, so I was >>>>> waiting on respin with comments addressed. >>>>> I don't think that this patch is good enough for merging. >>>>> >>>>> >>>> >>>> But it seems like the current patch does match what's Microsoft HyperV >>>> is publishing in it's APCI tables. >>>> >>>> I dont think it's correct for us to "fix" Microsoft emulation even if >>>> it's wrong, since that's what Windows probably expects to see... >>>> >>>> I tried looking where Microsoft uses the ACPI tables to identify the >>>> VMBus but without much luck in order to understand how flexible a change >>>> would be for the OS to still detect the VMBus device, but in general >>>> I think "correcting" something that is emulated 1:1 because there is no >>>> spec is the right way. >>> >>> I'd agree, if removing nonsense would break VMBus detection (does it?). >>> if something is that doesn't make sense but has to stay because it is need >>> to make windows happy, that's fine , just add annotate is with comment, >>> so it won't confuse anyone why that code exists there later on. >>> >>> I suggest to: >>> 1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong >>> 2. drop one IRQ, newer hyper-v seems to be doing fine with only one >>> 3. it's not ISA device, I'd suggest to move into _SB scope >>> 4. I don't know much about IRQs but >>> git grep DEFINE_PROP_ | grep -i iqr >>> yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns >>> IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). >>> So I'd leave it upto Paolo or someone else to decide/comment on. >>> >> >> Sounds like a plan, I'll try to come up with the test results >> (at least for Windows 10 guest which is what I have setup) and update >> this thread with the results. >> >> -- Jon. >> >>>> >>>>>> >>>>>> Paolo >>>>>> >>>>> >>>> >>> > Hi guys, > > Sorry for the delay... > > So first ill clarify what was the test, the test was to see the device > "Microsoft Hyper-V Virtual Machine Bus" in Windows Device Manager under > "System devices" with a state of "working properly". > > It seems like it's ok to drop all the _PS* and _STA. > > It seems to be functioning with single IRQ as well, it is worth noting that even when i dropped the entire _CRS (so no IRQs resources are required, the device was still showing that it's functioning, but I suspect this might affect the child devices like hv-net and hv-scsi). I guess you tested a single Windows version, correct? It may be that requirements differ between Windows versions, just as you say below about the required enlightenments. > With that said I did run into a small issue I set-up Win10 1903 (aka 19H1) and it seems like VMBus now requires to have the following features enabled: > HV_VP_RUNTIME_AVAILABLE > HV_TIME_REF_COUNT_AVAILABLE > HV_SYNIC_AVAILABLE > HV_SYNTIMERS_AVAILABLE > HV_APIC_ACCESS_AVAILABLE > HV_HYPERCALL_AVAILABLE > HV_VP_INDEX_AVAILABLE > > So notice that previously only SYNIC and VPINDEX was needed, now you need the whole thing so you need to run qemu with something like > -cpu host,hv-relaxed,hv_spinlocks=0x1fff,hv_time,hv-vapic,hv-vpindex,hv-synic,hv-runtime,hv-stimer > > The validation was done in winhv!WinHvpCheckPartitionPrivileges . > > Paolo I noticed you have done a PULL request, would you like to wait on it and we will submit a version with a single IRQ (selectable by user property) and go with Igor's suggestion dropping _PS* and _STA (though like I said before I prefer to mimic the original HyperV with it's bugs, but I'll leave this decision to you). The code is already in the upstream QEMU tree, it's a known-working code, so I think it is better to simply work incrementally on further improving the current version rather than backing it out and merging it again later. This way it will (hopefully) get some wider testing sooner. Not to mention that it is less likely for some other QEMU change to accidentally break it. > Also today VMBus only verifies SYNIC is enabled I'm not sure how but I wonder if we want to some how exports from the CPU which other HV features are enabled so we can verify all the required ones are set, would appreciate if you have any suggestions here. > > Cheers, > -- Jon. Thanks, Maciej
On 14/06/2020, Maciej S. Szmigiero wrote: >Hi Jon, > >On 14.06.2020 16:11, Jon Doron wrote: >> On 28/05/2020, Jon Doron wrote: >>> On 28/05/2020, Igor Mammedov wrote: >>>> On Thu, 28 May 2020 08:26:42 +0300 >>>> Jon Doron <arilou@gmail.com> wrote: >>>> >>>>> On 22/05/2020, Igor Mammedow wrote: >>>>>> On Thu, 21 May 2020 18:02:07 +0200 >>>>>> Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>>> >>>>>>> On 13/05/20 17:34, Igor Mammedov wrote: >>>>>>> > I'd rather avoid using random IRQ numbers (considering we are >>>>>>> > dealing with black-box here). So if it's really necessary to have >>>>>>> > IRQ described here, I'd suggest to implement them in device model >>>>>>> > so they would be reserved and QEMU would error out in a sane way if >>>>>>> > IRQ conflict is detected. >>>>>>> >>>>>>> We don't generally detect ISA IRQ conflicts though, do we? >>>>>> >>>>>> that I don't know that's why I'm not suggesting how to do it. >>>>>> The point is hard-coding in AML random IRQs is not right thing to do, >>>>>> (especially with the lack of 'any' spec), as minimum AML should pull >>>>>> it from device model and that probably should be configurable and set >>>>>> by board. >>>>>> >>>>>> Other thing is: >>>>>> I haven't looked at VMBus device model in detail, but DSDT part aren't >>>>>> matching device though (device model is not ISA device hence AML part >>>>>> shouldn't be on in ISA scope), where to put it is open question. >>>>>> There were other issues with AML code, I've commented on, so I was >>>>>> waiting on respin with comments addressed. >>>>>> I don't think that this patch is good enough for merging. >>>>>> >>>>>> >>>>> >>>>> But it seems like the current patch does match what's Microsoft HyperV >>>>> is publishing in it's APCI tables. >>>>> >>>>> I dont think it's correct for us to "fix" Microsoft emulation even if >>>>> it's wrong, since that's what Windows probably expects to see... >>>>> >>>>> I tried looking where Microsoft uses the ACPI tables to identify the >>>>> VMBus but without much luck in order to understand how flexible a change >>>>> would be for the OS to still detect the VMBus device, but in general >>>>> I think "correcting" something that is emulated 1:1 because there is no >>>>> spec is the right way. >>>> >>>> I'd agree, if removing nonsense would break VMBus detection (does it?). >>>> if something is that doesn't make sense but has to stay because it is need >>>> to make windows happy, that's fine , just add annotate is with comment, >>>> so it won't confuse anyone why that code exists there later on. >>>> >>>> I suggest to: >>>> 1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong >>>> 2. drop one IRQ, newer hyper-v seems to be doing fine with only one >>>> 3. it's not ISA device, I'd suggest to move into _SB scope >>>> 4. I don't know much about IRQs but >>>> git grep DEFINE_PROP_ | grep -i iqr >>>> yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns >>>> IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). >>>> So I'd leave it upto Paolo or someone else to decide/comment on. >>>> >>> >>> Sounds like a plan, I'll try to come up with the test results >>> (at least for Windows 10 guest which is what I have setup) and update >>> this thread with the results. >>> >>> -- Jon. >>> >>>>> >>>>>>> >>>>>>> Paolo >>>>>>> >>>>>> >>>>> >>>> >> Hi guys, >> >> Sorry for the delay... >> >> So first ill clarify what was the test, the test was to see the device >> "Microsoft Hyper-V Virtual Machine Bus" in Windows Device Manager under >> "System devices" with a state of "working properly". >> >> It seems like it's ok to drop all the _PS* and _STA. >> >> It seems to be functioning with single IRQ as well, it is worth noting that even when i dropped the entire _CRS (so no IRQs resources are required, the device was still showing that it's functioning, but I suspect this might affect the child devices like hv-net and hv-scsi). > >I guess you tested a single Windows version, correct? >It may be that requirements differ between Windows versions, just as you >say below about the required enlightenments. > Yes I have tested only a single version, but from the looks of it there is only a single IRQ in the HyperV APCI table that I have looked at, and I believe the one you have pasted as well in the past, so that change sounds reasonable to me. As for the enlightenments dont you prefer to have all those as mandatory for VMBus in order not to run into the issue I have encountered? >> With that said I did run into a small issue I set-up Win10 1903 (aka 19H1) and it seems like VMBus now requires to have the following features enabled: >> HV_VP_RUNTIME_AVAILABLE >> HV_TIME_REF_COUNT_AVAILABLE >> HV_SYNIC_AVAILABLE >> HV_SYNTIMERS_AVAILABLE >> HV_APIC_ACCESS_AVAILABLE >> HV_HYPERCALL_AVAILABLE >> HV_VP_INDEX_AVAILABLE >> >> So notice that previously only SYNIC and VPINDEX was needed, now you need the whole thing so you need to run qemu with something like >> -cpu host,hv-relaxed,hv_spinlocks=0x1fff,hv_time,hv-vapic,hv-vpindex,hv-synic,hv-runtime,hv-stimer >> >> The validation was done in winhv!WinHvpCheckPartitionPrivileges . >> >> Paolo I noticed you have done a PULL request, would you like to wait on it and we will submit a version with a single IRQ (selectable by user property) and go with Igor's suggestion dropping _PS* and _STA (though like I said before I prefer to mimic the original HyperV with it's bugs, but I'll leave this decision to you). > >The code is already in the upstream QEMU tree, it's a known-working code, >so I think it is better to simply work incrementally on further improving >the current version rather than backing it out and merging it again later. > >This way it will (hopefully) get some wider testing sooner. > >Not to mention that it is less likely for some other QEMU change to >accidentally break it. > That's great I agree incrementally is the right way here, should we open another Thread to discuss all these changes or do you prefer we will keep going on this one? >> Also today VMBus only verifies SYNIC is enabled I'm not sure how but I wonder if we want to some how exports from the CPU which other HV features are enabled so we can verify all the required ones are set, would appreciate if you have any suggestions here. >> >> Cheers, >> -- Jon. > >Thanks, >Maciej Thanks, -- Jon.
On 15.06.2020 04:40, Jon Doron wrote: > On 14/06/2020, Maciej S. Szmigiero wrote: >> Hi Jon, >> >> On 14.06.2020 16:11, Jon Doron wrote: >>> On 28/05/2020, Jon Doron wrote: >>>> On 28/05/2020, Igor Mammedov wrote: >>>>> On Thu, 28 May 2020 08:26:42 +0300 >>>>> Jon Doron <arilou@gmail.com> wrote: >>>>> >>>>>> On 22/05/2020, Igor Mammedow wrote: >>>>>>> On Thu, 21 May 2020 18:02:07 +0200 >>>>>>> Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>>>> >>>>>>>> On 13/05/20 17:34, Igor Mammedov wrote: >>>>>>>> > I'd rather avoid using random IRQ numbers (considering we are >>>>>>>> > dealing with black-box here). So if it's really necessary to have >>>>>>>> > IRQ described here, I'd suggest to implement them in device model >>>>>>>> > so they would be reserved and QEMU would error out in a sane way if >>>>>>>> > IRQ conflict is detected. >>>>>>>> >>>>>>>> We don't generally detect ISA IRQ conflicts though, do we? >>>>>>> >>>>>>> that I don't know that's why I'm not suggesting how to do it. >>>>>>> The point is hard-coding in AML random IRQs is not right thing to do, >>>>>>> (especially with the lack of 'any' spec), as minimum AML should pull >>>>>>> it from device model and that probably should be configurable and set >>>>>>> by board. >>>>>>> >>>>>>> Other thing is: >>>>>>> I haven't looked at VMBus device model in detail, but DSDT part aren't >>>>>>> matching device though (device model is not ISA device hence AML part >>>>>>> shouldn't be on in ISA scope), where to put it is open question. >>>>>>> There were other issues with AML code, I've commented on, so I was >>>>>>> waiting on respin with comments addressed. >>>>>>> I don't think that this patch is good enough for merging. >>>>>>> >>>>>>> >>>>>> >>>>>> But it seems like the current patch does match what's Microsoft HyperV >>>>>> is publishing in it's APCI tables. >>>>>> >>>>>> I dont think it's correct for us to "fix" Microsoft emulation even if >>>>>> it's wrong, since that's what Windows probably expects to see... >>>>>> >>>>>> I tried looking where Microsoft uses the ACPI tables to identify the >>>>>> VMBus but without much luck in order to understand how flexible a change >>>>>> would be for the OS to still detect the VMBus device, but in general >>>>>> I think "correcting" something that is emulated 1:1 because there is no >>>>>> spec is the right way. >>>>> >>>>> I'd agree, if removing nonsense would break VMBus detection (does it?). >>>>> if something is that doesn't make sense but has to stay because it is need >>>>> to make windows happy, that's fine , just add annotate is with comment, >>>>> so it won't confuse anyone why that code exists there later on. >>>>> >>>>> I suggest to: >>>>> 1. try dropping _PS* & _STA as it doesn't actually does anything and _PS3 is plain wrong >>>>> 2. drop one IRQ, newer hyper-v seems to be doing fine with only one >>>>> 3. it's not ISA device, I'd suggest to move into _SB scope >>>>> 4. I don't know much about IRQs but >>>>> git grep DEFINE_PROP_ | grep -i iqr >>>>> yields nothing so I'm not sure if it's acceptable. Typically it's board that assigns >>>>> IRQ and not device, for Sysbus devices (see: sysbus_init_irq/sysbus_connect_irq). >>>>> So I'd leave it upto Paolo or someone else to decide/comment on. >>>>> >>>> >>>> Sounds like a plan, I'll try to come up with the test results >>>> (at least for Windows 10 guest which is what I have setup) and update >>>> this thread with the results. >>>> >>>> -- Jon. >>>> >>>>>> >>>>>>>> >>>>>>>> Paolo >>>>>>>> >>>>>>> >>>>>> >>>>> >>> Hi guys, >>> >>> Sorry for the delay... >>> >>> So first ill clarify what was the test, the test was to see the device >>> "Microsoft Hyper-V Virtual Machine Bus" in Windows Device Manager under >>> "System devices" with a state of "working properly". >>> >>> It seems like it's ok to drop all the _PS* and _STA. >>> >>> It seems to be functioning with single IRQ as well, it is worth noting that even when i dropped the entire _CRS (so no IRQs resources are required, the device was still showing that it's functioning, but I suspect this might affect the child devices like hv-net and hv-scsi). >> >> I guess you tested a single Windows version, correct? >> It may be that requirements differ between Windows versions, just as you >> say below about the required enlightenments. >> > > Yes I have tested only a single version, but from the looks of it there is only a single IRQ in the HyperV APCI table that I have looked at, and I believe the one you have pasted as well in the past, so that change sounds reasonable to me. I meant also other changes, like dropping of some ACPI methods. > As for the enlightenments dont you prefer to have all those as mandatory for VMBus in order not to run into the issue I have encountered? Well, if somebody wants to run only older guests they will be content with just a limited set of enlightenments, right? But I don't know what's the QEMU project policy is here. >>> With that said I did run into a small issue I set-up Win10 1903 (aka 19H1) and it seems like VMBus now requires to have the following features enabled: >>> HV_VP_RUNTIME_AVAILABLE >>> HV_TIME_REF_COUNT_AVAILABLE >>> HV_SYNIC_AVAILABLE >>> HV_SYNTIMERS_AVAILABLE >>> HV_APIC_ACCESS_AVAILABLE >>> HV_HYPERCALL_AVAILABLE >>> HV_VP_INDEX_AVAILABLE >>> >>> So notice that previously only SYNIC and VPINDEX was needed, now you need the whole thing so you need to run qemu with something like >>> -cpu host,hv-relaxed,hv_spinlocks=0x1fff,hv_time,hv-vapic,hv-vpindex,hv-synic,hv-runtime,hv-stimer >>> >>> The validation was done in winhv!WinHvpCheckPartitionPrivileges . >>> >>> Paolo I noticed you have done a PULL request, would you like to wait on it and we will submit a version with a single IRQ (selectable by user property) and go with Igor's suggestion dropping _PS* and _STA (though like I said before I prefer to mimic the original HyperV with it's bugs, but I'll leave this decision to you). >> >> The code is already in the upstream QEMU tree, it's a known-working code, >> so I think it is better to simply work incrementally on further improving >> the current version rather than backing it out and merging it again later. >> >> This way it will (hopefully) get some wider testing sooner. >> >> Not to mention that it is less likely for some other QEMU change to >> accidentally break it. >> > > That's great I agree incrementally is the right way here, should we open another Thread to discuss all these changes or do you prefer we will keep going on this one? If that's going to be an incremental change it is probably worth its own email thread. Otherwise the existing one will grow without bounds. >>> Also today VMBus only verifies SYNIC is enabled I'm not sure how but I wonder if we want to some how exports from the CPU which other HV features are enabled so we can verify all the required ones are set, would appreciate if you have any suggestions here. >>> > > Thanks, > -- Jon. Thanks, Maciej
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); + + aml_append(dev, aml_name_decl("_PS3", aml_int(0x0))); + + crs = aml_resource_template(); + aml_append(crs, aml_irq_no_flags(vmbus_bridge->irq0)); + /* FIXME: newer HyperV gets by with only one 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)); + } + 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;