| Message ID | 20210722105945.2080428-3-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: pcihp: fix hotplug when bridge is wired to function > 0 | expand |
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>
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
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
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 >
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 > >
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 > > > >
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
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
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 :|
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)); } }
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(-)