* [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 @ 2021-07-22 10:59 Igor Mammedov 2021-07-22 10:59 ` [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() Igor Mammedov ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Igor Mammedov @ 2021-07-22 10:59 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, jusual, mst For full description see 2/2. Tested hotplug on Q35 (see 2/2 for reproducer) and PC (with pci-bridge) machines Igor Mammedov (2): acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() acpi: x86: pcihp: add support hotplug on multifunction bridges hw/i386/acpi-build.c | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() 2021-07-22 10:59 [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Igor Mammedov @ 2021-07-22 10:59 ` Igor Mammedov 2021-07-22 17:56 ` Michael S. Tsirkin 2021-07-22 10:59 ` [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges Igor Mammedov 2021-07-22 17:56 ` [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Michael S. Tsirkin 2 siblings, 1 reply; 15+ messages in thread From: Igor Mammedov @ 2021-07-22 10:59 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, jusual, mst Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/acpi-build.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 17836149fe..b40e284b72 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, Aml *dev, *notify_method = NULL, *method; QObject *bsel; PCIBus *sec; - int i; + int devfn; bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); if (bsel) { @@ -384,11 +384,11 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); } - for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) { + for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { DeviceClass *dc; PCIDeviceClass *pc; - PCIDevice *pdev = bus->devices[i]; - int slot = PCI_SLOT(i); + PCIDevice *pdev = bus->devices[devfn]; + int slot = PCI_SLOT(devfn); bool hotplug_enabled_dev; bool bridge_in_acpi; bool cold_plugged_bridge; @@ -525,13 +525,12 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, /* Notify about child bus events in any case */ if (pcihp_bridge_en) { QLIST_FOREACH(sec, &bus->child, sibling) { - int32_t devfn = sec->parent_dev->devfn; - if (pci_bus_is_root(sec)) { continue; } - aml_append(method, aml_name("^S%.02X.PCNT", devfn)); + aml_append(method, aml_name("^S%.02X.PCNT", + sec->parent_dev->devfn)); } } -- 2.27.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() 2021-07-22 10:59 ` [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() Igor Mammedov @ 2021-07-22 17:56 ` Michael S. Tsirkin 2021-07-23 7:34 ` Igor Mammedov 0 siblings, 1 reply; 15+ messages in thread From: Michael S. Tsirkin @ 2021-07-22 17:56 UTC (permalink / raw) To: Igor Mammedov; +Cc: peter.maydell, jusual, qemu-devel I would add a description: we want to scan all functions not just function 0 to describe hotplug into bridges at function != 0. in preparation for this, refactor code to not skip functions != 0. On Thu, Jul 22, 2021 at 06:59:44AM -0400, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/acpi-build.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 17836149fe..b40e284b72 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > Aml *dev, *notify_method = NULL, *method; > QObject *bsel; > PCIBus *sec; > - int i; > + int devfn; > > bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); > if (bsel) { > @@ -384,11 +384,11 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); > } > > - for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) { > + for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { > DeviceClass *dc; > PCIDeviceClass *pc; > - PCIDevice *pdev = bus->devices[i]; > - int slot = PCI_SLOT(i); > + PCIDevice *pdev = bus->devices[devfn]; > + int slot = PCI_SLOT(devfn); > bool hotplug_enabled_dev; > bool bridge_in_acpi; > bool cold_plugged_bridge; I am a bit puzzled about why this is equivalent. so we used to scan just function 0 on each slot. now we are scanning them all. won't this generate a different AML code? in fact duplicate descriptions? I suspect you need to move the check for slot == 0 from the next patch to this one otherwise bisect will be broken. Or just squash this part into next patch. up to you. > @@ -525,13 +525,12 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > /* Notify about child bus events in any case */ > if (pcihp_bridge_en) { > QLIST_FOREACH(sec, &bus->child, sibling) { > - int32_t devfn = sec->parent_dev->devfn; > - > if (pci_bus_is_root(sec)) { > continue; > } > > - aml_append(method, aml_name("^S%.02X.PCNT", devfn)); > + aml_append(method, aml_name("^S%.02X.PCNT", > + sec->parent_dev->devfn)); > } > } > this is a refactor, sure. > -- > 2.27.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() 2021-07-22 17:56 ` Michael S. Tsirkin @ 2021-07-23 7:34 ` Igor Mammedov 0 siblings, 0 replies; 15+ messages in thread From: Igor Mammedov @ 2021-07-23 7:34 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: peter.maydell, jusual, qemu-devel On Thu, 22 Jul 2021 13:56:18 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > I would add a description: we want to scan all functions > not just function 0 to describe hotplug into bridges > at function != 0. in preparation for this, refactor code to not > skip functions != 0. > > On Thu, Jul 22, 2021 at 06:59:44AM -0400, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/acpi-build.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 17836149fe..b40e284b72 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > Aml *dev, *notify_method = NULL, *method; > > QObject *bsel; > > PCIBus *sec; > > - int i; > > + int devfn; > > > > bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL); > > if (bsel) { > > @@ -384,11 +384,11 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED); > > } > > > > - for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) { > > + for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { > > DeviceClass *dc; > > PCIDeviceClass *pc; > > - PCIDevice *pdev = bus->devices[i]; > > - int slot = PCI_SLOT(i); > > + PCIDevice *pdev = bus->devices[devfn]; > > + int slot = PCI_SLOT(devfn); > > bool hotplug_enabled_dev; > > bool bridge_in_acpi; > > bool cold_plugged_bridge; > > I am a bit puzzled about why this is equivalent. so we used to scan just > function 0 on each slot. now we are scanning them all. > won't this generate a different AML code? in fact duplicate > descriptions? > I suspect you need to move the check for slot == 0 from the next patch > to this one otherwise bisect will be broken. this was my mistake when splitting it into separate patch, I shouldn't have touched += PCI_FUNC_MAX here, point of the patch was to s/i/devfn/m so it won't distract from what the next patch is doing. > Or just squash this part into next patch. up to you. ok I'll squash it into next. > > > @@ -525,13 +525,12 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > /* Notify about child bus events in any case */ > > if (pcihp_bridge_en) { > > QLIST_FOREACH(sec, &bus->child, sibling) { > > - int32_t devfn = sec->parent_dev->devfn; > > - > > if (pci_bus_is_root(sec)) { > > continue; > > } > > > > - aml_append(method, aml_name("^S%.02X.PCNT", devfn)); > > + aml_append(method, aml_name("^S%.02X.PCNT", > > + sec->parent_dev->devfn)); > > } > > } > > > > > this is a refactor, sure. > > > -- > > 2.27.0 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 10:59 [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Igor Mammedov 2021-07-22 10:59 ` [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() Igor Mammedov @ 2021-07-22 10:59 ` Igor Mammedov 2021-07-22 12:38 ` Laurent Vivier ` (2 more replies) 2021-07-22 17:56 ` [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Michael S. Tsirkin 2 siblings, 3 replies; 15+ messages in thread From: Igor Mammedov @ 2021-07-22 10:59 UTC (permalink / raw) To: qemu-devel; +Cc: Laurent Vivier, peter.maydell, jusual, mst Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) switched PCI hotplug from native to ACPI one by default. That however breaks ihotplug on following CLI that used to work: -nodefaults -machine q35 \ -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 where PCI device is hotplugged to pcie-root-port-1 with error on guest side: ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) cause is that QEMU's ACPI hotplug never supported functions other then 0 and due to bug it was generating notification entries for not described functions. Technically there is no reason not to describe cold-plugged bridges (root ports) on functions other then 0, as they similaraly to bridge on function 0 are unpluggable. Fix consists of describing cold-plugged bridges[root ports] on functions other than 0. Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reported-by: Laurent Vivier <lvivier@redhat.com> --- hw/i386/acpi-build.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index b40e284b72..77cb7a338a 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -364,7 +364,7 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) int32_t devfn = PCI_DEVFN(slot, 0); if_ctx = aml_if(aml_and(aml_arg(0), aml_int(0x1U << slot), NULL)); - aml_append(if_ctx, aml_notify(aml_name("S%.02X", devfn), aml_arg(1))); + aml_append(if_ctx, aml_notify(aml_name("^S%.03X", devfn), aml_arg(1))); aml_append(method, if_ctx); } @@ -389,18 +389,26 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, PCIDeviceClass *pc; PCIDevice *pdev = bus->devices[devfn]; int slot = PCI_SLOT(devfn); + int func = PCI_FUNC(devfn); + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ + int adr = slot << 16 | func; bool hotplug_enabled_dev; bool bridge_in_acpi; bool cold_plugged_bridge; if (!pdev) { - if (bsel) { /* add hotplug slots for non present devices */ + /* + * add hotplug slots for non present devices. + * hotplug is supported only for non-multifunction device + * so generate device description only for function 0 + */ + if (bsel && !PCI_FUNC(devfn)) { if (pci_bus_is_express(bus) && slot > 0) { break; } - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); + dev = aml_device("S%.03X", devfn); aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); aml_append(method, aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, continue; } + /* + * allow describing coldplugged bridges in ACPI even if they are not + * on function 0, as they are not unpluggale, for all other devices + * generate description only for function 0 per slot + */ + if (PCI_FUNC(devfn) && !bridge_in_acpi) { + continue; + } + /* start to compose PCI slot descriptor */ - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); + dev = aml_device("S%.03X", devfn); + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); if (bsel) { /* @@ -529,7 +546,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, continue; } - aml_append(method, aml_name("^S%.02X.PCNT", + aml_append(method, aml_name("^S%.03X.PCNT", sec->parent_dev->devfn)); } } -- 2.27.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 10:59 ` [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges Igor Mammedov @ 2021-07-22 12:38 ` Laurent Vivier 2021-07-22 17:49 ` Michael S. Tsirkin 2021-07-23 8:50 ` Daniel P. Berrangé 2 siblings, 0 replies; 15+ messages in thread From: Laurent Vivier @ 2021-07-22 12:38 UTC (permalink / raw) To: Igor Mammedov, qemu-devel; +Cc: peter.maydell, jusual, mst On 22/07/2021 12:59, Igor Mammedov wrote: > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > switched PCI hotplug from native to ACPI one by default. > > That however breaks ihotplug on following CLI that used to work: > -nodefaults -machine q35 \ > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > and due to bug it was generating notification entries for not described > functions. > > Technically there is no reason not to describe cold-plugged bridges > (root ports) on functions other then 0, as they similaraly to bridge > on function 0 are unpluggable. > > Fix consists of describing cold-plugged bridges[root ports] on functions > other than 0. > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reported-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/i386/acpi-build.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > Tested-by: Laurent Vivier <lvivier@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 10:59 ` [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges Igor Mammedov 2021-07-22 12:38 ` Laurent Vivier @ 2021-07-22 17:49 ` Michael S. Tsirkin 2021-07-22 18:13 ` Laurent Vivier ` (2 more replies) 2021-07-23 8:50 ` Daniel P. Berrangé 2 siblings, 3 replies; 15+ messages in thread From: Michael S. Tsirkin @ 2021-07-22 17:49 UTC (permalink / raw) To: Igor Mammedov; +Cc: Laurent Vivier, peter.maydell, jusual, qemu-devel On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > switched PCI hotplug from native to ACPI one by default. > > That however breaks ihotplug on following CLI that used to work: s/ihotplug/hotplug/ ? > -nodefaults -machine q35 \ > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > and due to bug it was generating notification entries for not described > functions. > > Technically there is no reason not to describe cold-plugged bridges > (root ports) on functions other then 0, as they similaraly to bridge > on function 0 are unpluggable. > > Fix consists of describing cold-plugged bridges[root ports] on functions > other than 0. I would add: since we need to describe multifunction devices > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 use short hash and include subject within ("subject here") please > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reported-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/i386/acpi-build.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index b40e284b72..77cb7a338a 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -364,7 +364,7 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) > int32_t devfn = PCI_DEVFN(slot, 0); > > if_ctx = aml_if(aml_and(aml_arg(0), aml_int(0x1U << slot), NULL)); > - aml_append(if_ctx, aml_notify(aml_name("S%.02X", devfn), aml_arg(1))); > + aml_append(if_ctx, aml_notify(aml_name("^S%.03X", devfn), aml_arg(1))); > aml_append(method, if_ctx); > } > why are you adding a parent prefix here? anything wrong with relying on search rules? Seems like an unrelated change. Also there's always 8 bit in devfn. Why do we need an extra 0? > @@ -389,18 +389,26 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > PCIDeviceClass *pc; > PCIDevice *pdev = bus->devices[devfn]; > int slot = PCI_SLOT(devfn); > + int func = PCI_FUNC(devfn); > + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ > + int adr = slot << 16 | func; > bool hotplug_enabled_dev; > bool bridge_in_acpi; > bool cold_plugged_bridge; > > if (!pdev) { > - if (bsel) { /* add hotplug slots for non present devices */ > + /* > + * add hotplug slots for non present devices. > + * hotplug is supported only for non-multifunction device > + * so generate device description only for function 0 > + */ > + if (bsel && !PCI_FUNC(devfn)) { replace with func here and elsewhere? > if (pci_bus_is_express(bus) && slot > 0) { > break; > } > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > + dev = aml_device("S%.03X", devfn); > aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > aml_append(method, > aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) > @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > continue; > } > > + /* > + * allow describing coldplugged bridges in ACPI even if they are not > + * on function 0, as they are not unpluggale, for all other devices unpluggable > + * generate description only for function 0 per slot > + */ > + if (PCI_FUNC(devfn) && !bridge_in_acpi) { > + continue; > + } > + > /* start to compose PCI slot descriptor */ > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > + dev = aml_device("S%.03X", devfn); > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > if (bsel) { > /* > @@ -529,7 +546,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > continue; > } > > - aml_append(method, aml_name("^S%.02X.PCNT", > + aml_append(method, aml_name("^S%.03X.PCNT", > sec->parent_dev->devfn)); > } > } > -- > 2.27.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 17:49 ` Michael S. Tsirkin @ 2021-07-22 18:13 ` Laurent Vivier 2021-07-23 7:47 ` Igor Mammedov 2021-07-23 8:48 ` Daniel P. Berrangé 2 siblings, 0 replies; 15+ messages in thread From: Laurent Vivier @ 2021-07-22 18:13 UTC (permalink / raw) To: Igor Mammedov; +Cc: peter.maydell, jusual, qemu-devel, Michael S. Tsirkin On 22/07/2021 19:49, Michael S. Tsirkin wrote: > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: ... >> >> Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > > use short hash and include subject within ("subject here") please Tips: some people use .gitconfig [pretty] fixes = Fixes: %h (\"%s\")%nCc: %aE [alias] showfix = log -1 --format=fixes $ git showfix 17858a169508609ca9063c544833e5a1adeb7b52 Fixes: 17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") Cc: jusual@redhat.com Thanks, Laurent ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 17:49 ` Michael S. Tsirkin 2021-07-22 18:13 ` Laurent Vivier @ 2021-07-23 7:47 ` Igor Mammedov 2021-07-23 8:05 ` Michael S. Tsirkin 2021-07-23 8:48 ` Daniel P. Berrangé 2 siblings, 1 reply; 15+ messages in thread From: Igor Mammedov @ 2021-07-23 7:47 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Laurent Vivier, peter.maydell, jusual, qemu-devel On Thu, 22 Jul 2021 13:49:34 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > > switched PCI hotplug from native to ACPI one by default. > > > > That however breaks ihotplug on following CLI that used to work: > > s/ihotplug/hotplug/ ? > > > -nodefaults -machine q35 \ > > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > > and due to bug it was generating notification entries for not described > > functions. > > > > Technically there is no reason not to describe cold-plugged bridges > > (root ports) on functions other then 0, as they similaraly to bridge > > on function 0 are unpluggable. > > > > Fix consists of describing cold-plugged bridges[root ports] on functions > > other than 0. > > > I would add: since we need to describe multifunction devices ok > > > > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > > use short hash and include subject within ("subject here") please > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reported-by: Laurent Vivier <lvivier@redhat.com> > > --- > > hw/i386/acpi-build.c | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index b40e284b72..77cb7a338a 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -364,7 +364,7 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) > > int32_t devfn = PCI_DEVFN(slot, 0); > > > > if_ctx = aml_if(aml_and(aml_arg(0), aml_int(0x1U << slot), NULL)); > > - aml_append(if_ctx, aml_notify(aml_name("S%.02X", devfn), aml_arg(1))); > > + aml_append(if_ctx, aml_notify(aml_name("^S%.03X", devfn), aml_arg(1))); > > aml_append(method, if_ctx); > > } > > > > why are you adding a parent prefix here? anything wrong with > relying on search rules? > Seems like an unrelated change. it's remnants from time I was trying to figure out why functions weren't visible, I'll drop it. > Also there's always 8 bit in devfn. Why do we need an extra 0? 2 digits hold slot value, like it used to be and the last one to hold function. Basically like it's shown in PCI.FW spec example. but if you prefer to print raw devfn I can change to that, it should work as well. > > @@ -389,18 +389,26 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > PCIDeviceClass *pc; > > PCIDevice *pdev = bus->devices[devfn]; > > int slot = PCI_SLOT(devfn); > > + int func = PCI_FUNC(devfn); > > + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ > > + int adr = slot << 16 | func; > > bool hotplug_enabled_dev; > > bool bridge_in_acpi; > > bool cold_plugged_bridge; > > > > if (!pdev) { > > - if (bsel) { /* add hotplug slots for non present devices */ > > + /* > > + * add hotplug slots for non present devices. > > + * hotplug is supported only for non-multifunction device > > + * so generate device description only for function 0 > > + */ > > + if (bsel && !PCI_FUNC(devfn)) { > > replace with func here and elsewhere? ok > > > if (pci_bus_is_express(bus) && slot > 0) { > > break; > > } > > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > > + dev = aml_device("S%.03X", devfn); > > aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > > aml_append(method, > > aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) > > @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > continue; > > } > > > > + /* > > + * allow describing coldplugged bridges in ACPI even if they are not > > + * on function 0, as they are not unpluggale, for all other devices > > > unpluggable > > > + * generate description only for function 0 per slot > > + */ > > + if (PCI_FUNC(devfn) && !bridge_in_acpi) { > > + continue; > > + } > > + > > /* start to compose PCI slot descriptor */ > > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > > + dev = aml_device("S%.03X", devfn); > > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > > > if (bsel) { > > /* > > @@ -529,7 +546,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > continue; > > } > > > > - aml_append(method, aml_name("^S%.02X.PCNT", > > + aml_append(method, aml_name("^S%.03X.PCNT", > > sec->parent_dev->devfn)); > > } > > } > > -- > > 2.27.0 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-23 7:47 ` Igor Mammedov @ 2021-07-23 8:05 ` Michael S. Tsirkin 2021-07-23 8:33 ` Igor Mammedov 0 siblings, 1 reply; 15+ messages in thread From: Michael S. Tsirkin @ 2021-07-23 8:05 UTC (permalink / raw) To: Igor Mammedov; +Cc: Laurent Vivier, peter.maydell, jusual, qemu-devel On Fri, Jul 23, 2021 at 09:47:26AM +0200, Igor Mammedov wrote: > On Thu, 22 Jul 2021 13:49:34 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > > > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > > > switched PCI hotplug from native to ACPI one by default. > > > > > > That however breaks ihotplug on following CLI that used to work: > > > > s/ihotplug/hotplug/ ? > > > > > -nodefaults -machine q35 \ > > > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > > > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > > > > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > > > > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > > > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > > > > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > > > and due to bug it was generating notification entries for not described > > > functions. > > > > > > Technically there is no reason not to describe cold-plugged bridges > > > (root ports) on functions other then 0, as they similaraly to bridge > > > on function 0 are unpluggable. > > > > > > Fix consists of describing cold-plugged bridges[root ports] on functions > > > other than 0. > > > > > > I would add: since we need to describe multifunction devices > > ok > > > > > > > > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > > > > use short hash and include subject within ("subject here") please > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Reported-by: Laurent Vivier <lvivier@redhat.com> > > > --- > > > hw/i386/acpi-build.c | 31 ++++++++++++++++++++++++------- > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index b40e284b72..77cb7a338a 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -364,7 +364,7 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) > > > int32_t devfn = PCI_DEVFN(slot, 0); > > > > > > if_ctx = aml_if(aml_and(aml_arg(0), aml_int(0x1U << slot), NULL)); > > > - aml_append(if_ctx, aml_notify(aml_name("S%.02X", devfn), aml_arg(1))); > > > + aml_append(if_ctx, aml_notify(aml_name("^S%.03X", devfn), aml_arg(1))); > > > aml_append(method, if_ctx); > > > } > > > > > > > why are you adding a parent prefix here? anything wrong with > > relying on search rules? > > Seems like an unrelated change. > it's remnants from time I was trying to figure out > why functions weren't visible, I'll drop it. > > > > Also there's always 8 bit in devfn. Why do we need an extra 0? > 2 digits hold slot value, like it used to be and > the last one to hold function. Basically like it's shown in PCI.FW spec example. > > but if you prefer to print raw devfn I can change to that, > it should work as well. will make the patch smaller, won't it? we can always change it separately to match spec examples more closely. > > > @@ -389,18 +389,26 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > PCIDeviceClass *pc; > > > PCIDevice *pdev = bus->devices[devfn]; > > > int slot = PCI_SLOT(devfn); > > > + int func = PCI_FUNC(devfn); > > > + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ > > > + int adr = slot << 16 | func; > > > bool hotplug_enabled_dev; > > > bool bridge_in_acpi; > > > bool cold_plugged_bridge; > > > > > > if (!pdev) { > > > - if (bsel) { /* add hotplug slots for non present devices */ > > > + /* > > > + * add hotplug slots for non present devices. > > > + * hotplug is supported only for non-multifunction device > > > + * so generate device description only for function 0 > > > + */ > > > + if (bsel && !PCI_FUNC(devfn)) { > > > > replace with func here and elsewhere? > > ok > > > > > > if (pci_bus_is_express(bus) && slot > 0) { > > > break; > > > } > > > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > > > + dev = aml_device("S%.03X", devfn); > > > aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > > > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > > > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > > method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > > > aml_append(method, > > > aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) > > > @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > continue; > > > } > > > > > > + /* > > > + * allow describing coldplugged bridges in ACPI even if they are not > > > + * on function 0, as they are not unpluggale, for all other devices > > > > > > unpluggable > > > > > + * generate description only for function 0 per slot > > > + */ > > > + if (PCI_FUNC(devfn) && !bridge_in_acpi) { > > > + continue; > > > + } > > > + > > > /* start to compose PCI slot descriptor */ > > > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > > > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > > > + dev = aml_device("S%.03X", devfn); > > > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > > > > > if (bsel) { > > > /* > > > @@ -529,7 +546,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > continue; > > > } > > > > > > - aml_append(method, aml_name("^S%.02X.PCNT", > > > + aml_append(method, aml_name("^S%.03X.PCNT", > > > sec->parent_dev->devfn)); > > > } > > > } > > > -- > > > 2.27.0 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-23 8:05 ` Michael S. Tsirkin @ 2021-07-23 8:33 ` Igor Mammedov 0 siblings, 0 replies; 15+ messages in thread From: Igor Mammedov @ 2021-07-23 8:33 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Laurent Vivier, peter.maydell, jusual, qemu-devel On Fri, 23 Jul 2021 04:05:18 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jul 23, 2021 at 09:47:26AM +0200, Igor Mammedov wrote: > > On Thu, 22 Jul 2021 13:49:34 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > > > > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > > > > switched PCI hotplug from native to ACPI one by default. > > > > > > > > That however breaks ihotplug on following CLI that used to work: > > > > > > s/ihotplug/hotplug/ ? > > > > > > > -nodefaults -machine q35 \ > > > > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > > > > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > > > > > > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > > > > > > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > > > > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > > > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > > > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > > > > > > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > > > > and due to bug it was generating notification entries for not described > > > > functions. > > > > > > > > Technically there is no reason not to describe cold-plugged bridges > > > > (root ports) on functions other then 0, as they similaraly to bridge > > > > on function 0 are unpluggable. > > > > > > > > Fix consists of describing cold-plugged bridges[root ports] on functions > > > > other than 0. > > > > > > > > > I would add: since we need to describe multifunction devices > > > > ok > > > > > > > > > > > > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > > > > > > use short hash and include subject within ("subject here") please > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Reported-by: Laurent Vivier <lvivier@redhat.com> > > > > --- > > > > hw/i386/acpi-build.c | 31 ++++++++++++++++++++++++------- > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > index b40e284b72..77cb7a338a 100644 > > > > --- a/hw/i386/acpi-build.c > > > > +++ b/hw/i386/acpi-build.c > > > > @@ -364,7 +364,7 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot) > > > > int32_t devfn = PCI_DEVFN(slot, 0); > > > > > > > > if_ctx = aml_if(aml_and(aml_arg(0), aml_int(0x1U << slot), NULL)); > > > > - aml_append(if_ctx, aml_notify(aml_name("S%.02X", devfn), aml_arg(1))); > > > > + aml_append(if_ctx, aml_notify(aml_name("^S%.03X", devfn), aml_arg(1))); > > > > aml_append(method, if_ctx); > > > > } > > > > > > > > > > why are you adding a parent prefix here? anything wrong with > > > relying on search rules? > > > Seems like an unrelated change. > > it's remnants from time I was trying to figure out > > why functions weren't visible, I'll drop it. > > > > > > > Also there's always 8 bit in devfn. Why do we need an extra 0? > > 2 digits hold slot value, like it used to be and > > the last one to hold function. Basically like it's shown in PCI.FW spec example. > > > > but if you prefer to print raw devfn I can change to that, > > it should work as well. > > will make the patch smaller, won't it? we can always change it > separately to match spec examples more closely. sure, done > > > > > @@ -389,18 +389,26 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > > PCIDeviceClass *pc; > > > > PCIDevice *pdev = bus->devices[devfn]; > > > > int slot = PCI_SLOT(devfn); > > > > + int func = PCI_FUNC(devfn); > > > > + /* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */ > > > > + int adr = slot << 16 | func; > > > > bool hotplug_enabled_dev; > > > > bool bridge_in_acpi; > > > > bool cold_plugged_bridge; > > > > > > > > if (!pdev) { > > > > - if (bsel) { /* add hotplug slots for non present devices */ > > > > + /* > > > > + * add hotplug slots for non present devices. > > > > + * hotplug is supported only for non-multifunction device > > > > + * so generate device description only for function 0 > > > > + */ > > > > + if (bsel && !PCI_FUNC(devfn)) { > > > > > > replace with func here and elsewhere? > > > > ok > > > > > > > > > if (pci_bus_is_express(bus) && slot > 0) { > > > > break; > > > > } > > > > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > > > > + dev = aml_device("S%.03X", devfn); > > > > aml_append(dev, aml_name_decl("_SUN", aml_int(slot))); > > > > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > > > > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > > > method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > > > > aml_append(method, > > > > aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN")) > > > > @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > > continue; > > > > } > > > > > > > > + /* > > > > + * allow describing coldplugged bridges in ACPI even if they are not > > > > + * on function 0, as they are not unpluggale, for all other devices > > > > > > > > > unpluggable > > > > > > > + * generate description only for function 0 per slot > > > > + */ > > > > + if (PCI_FUNC(devfn) && !bridge_in_acpi) { > > > > + continue; > > > > + } > > > > + > > > > /* start to compose PCI slot descriptor */ > > > > - dev = aml_device("S%.02X", PCI_DEVFN(slot, 0)); > > > > - aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16))); > > > > + dev = aml_device("S%.03X", devfn); > > > > + aml_append(dev, aml_name_decl("_ADR", aml_int(adr))); > > > > > > > > if (bsel) { > > > > /* > > > > @@ -529,7 +546,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > > continue; > > > > } > > > > > > > > - aml_append(method, aml_name("^S%.02X.PCNT", > > > > + aml_append(method, aml_name("^S%.03X.PCNT", > > > > sec->parent_dev->devfn)); > > > > } > > > > } > > > > -- > > > > 2.27.0 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 17:49 ` Michael S. Tsirkin 2021-07-22 18:13 ` Laurent Vivier 2021-07-23 7:47 ` Igor Mammedov @ 2021-07-23 8:48 ` Daniel P. Berrangé 2021-07-23 9:34 ` Michael S. Tsirkin 2 siblings, 1 reply; 15+ messages in thread From: Daniel P. Berrangé @ 2021-07-23 8:48 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Laurent Vivier, Igor Mammedov, jusual, qemu-devel, peter.maydell On Thu, Jul 22, 2021 at 01:49:34PM -0400, Michael S. Tsirkin wrote: > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > > switched PCI hotplug from native to ACPI one by default. > > > > That however breaks ihotplug on following CLI that used to work: > > s/ihotplug/hotplug/ ? > > > -nodefaults -machine q35 \ > > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > > and due to bug it was generating notification entries for not described > > functions. > > > > Technically there is no reason not to describe cold-plugged bridges > > (root ports) on functions other then 0, as they similaraly to bridge > > on function 0 are unpluggable. > > > > Fix consists of describing cold-plugged bridges[root ports] on functions > > other than 0. > > > I would add: since we need to describe multifunction devices > > > > > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > > use short hash and include subject within ("subject here") please Using short hashes isn't a good idea in commits IMHO. A git short hash is only guaranteed unique at the time it is generated. In future the repo might gain commits that result in a clashing short hash. Using the full hash is good. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-23 8:48 ` Daniel P. Berrangé @ 2021-07-23 9:34 ` Michael S. Tsirkin 0 siblings, 0 replies; 15+ messages in thread From: Michael S. Tsirkin @ 2021-07-23 9:34 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Laurent Vivier, Igor Mammedov, jusual, qemu-devel, peter.maydell On Fri, Jul 23, 2021 at 09:48:23AM +0100, Daniel P. Berrangé wrote: > On Thu, Jul 22, 2021 at 01:49:34PM -0400, Michael S. Tsirkin wrote: > > On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > > > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > > > switched PCI hotplug from native to ACPI one by default. > > > > > > That however breaks ihotplug on following CLI that used to work: > > > > s/ihotplug/hotplug/ ? > > > > > -nodefaults -machine q35 \ > > > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > > > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > > > > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > > > > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > > > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > > > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > > > > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > > > and due to bug it was generating notification entries for not described > > > functions. > > > > > > Technically there is no reason not to describe cold-plugged bridges > > > (root ports) on functions other then 0, as they similaraly to bridge > > > on function 0 are unpluggable. > > > > > > Fix consists of describing cold-plugged bridges[root ports] on functions > > > other than 0. > > > > > > I would add: since we need to describe multifunction devices > > > > > > > > > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > > > > use short hash and include subject within ("subject here") please > > Using short hashes isn't a good idea in commits IMHO. A git short > hash is only guaranteed unique at the time it is generated. In future > the repo might gain commits that result in a clashing short hash. > Using the full hash is good. > > > Regards, > Daniel It's a good point but it became a standard practice at this point. At least with the subject it's unlikely to be ambiguous too often. > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges 2021-07-22 10:59 ` [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges Igor Mammedov 2021-07-22 12:38 ` Laurent Vivier 2021-07-22 17:49 ` Michael S. Tsirkin @ 2021-07-23 8:50 ` Daniel P. Berrangé 2 siblings, 0 replies; 15+ messages in thread From: Daniel P. Berrangé @ 2021-07-23 8:50 UTC (permalink / raw) To: Igor Mammedov; +Cc: Laurent Vivier, peter.maydell, jusual, qemu-devel, mst On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote: > Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35) > switched PCI hotplug from native to ACPI one by default. > > That however breaks ihotplug on following CLI that used to work: s/ihotplug/hotplug/ > -nodefaults -machine q35 \ > -device pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1 \ > -device pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2 > > where PCI device is hotplugged to pcie-root-port-1 with error on guest side: > > ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND (20201113/psargs-330) > ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND) (20201113/psparse-531) > ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01] (20201113/evgpe-515) > > cause is that QEMU's ACPI hotplug never supported functions other then 0 > and due to bug it was generating notification entries for not described > functions. > > Technically there is no reason not to describe cold-plugged bridges > (root ports) on functions other then 0, as they similaraly to bridge s/similaraly/similarly/ > on function 0 are unpluggable. > > Fix consists of describing cold-plugged bridges[root ports] on functions > other than 0. > > Fixes: 17858a169508609ca9063c544833e5a1adeb7b52 > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reported-by: Laurent Vivier <lvivier@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 2021-07-22 10:59 [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Igor Mammedov 2021-07-22 10:59 ` [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() Igor Mammedov 2021-07-22 10:59 ` [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges Igor Mammedov @ 2021-07-22 17:56 ` Michael S. Tsirkin 2 siblings, 0 replies; 15+ messages in thread From: Michael S. Tsirkin @ 2021-07-22 17:56 UTC (permalink / raw) To: Igor Mammedov; +Cc: peter.maydell, jusual, qemu-devel On Thu, Jul 22, 2021 at 06:59:43AM -0400, Igor Mammedov wrote: > For full description see 2/2. > Tested hotplug on Q35 (see 2/2 for reproducer) and PC (with pci-bridge) machines > > Igor Mammedov (2): > acpi: x86: pcihp: cleanup devfn usage in > build_append_pci_bus_devices() > acpi: x86: pcihp: add support hotplug on multifunction bridges > > hw/i386/acpi-build.c | 42 +++++++++++++++++++++++++++++------------- > 1 file changed, 29 insertions(+), 13 deletions(-) > In fact I think this fixes a bug in acpi hotplug on pc too. have some questions though, posted. Thanks! > 2.27.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-23 9:35 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-22 10:59 [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Igor Mammedov 2021-07-22 10:59 ` [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices() Igor Mammedov 2021-07-22 17:56 ` Michael S. Tsirkin 2021-07-23 7:34 ` Igor Mammedov 2021-07-22 10:59 ` [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges Igor Mammedov 2021-07-22 12:38 ` Laurent Vivier 2021-07-22 17:49 ` Michael S. Tsirkin 2021-07-22 18:13 ` Laurent Vivier 2021-07-23 7:47 ` Igor Mammedov 2021-07-23 8:05 ` Michael S. Tsirkin 2021-07-23 8:33 ` Igor Mammedov 2021-07-23 8:48 ` Daniel P. Berrangé 2021-07-23 9:34 ` Michael S. Tsirkin 2021-07-23 8:50 ` Daniel P. Berrangé 2021-07-22 17:56 ` [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0 Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.