[PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0

[PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0

[Igor Mammedov]

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

[PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices()

[Igor Mammedov]

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

Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices()

[Michael S. Tsirkin]

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

Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices()

[Igor Mammedov]

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  
> 

[PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Igor Mammedov]

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

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Laurent Vivier]

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>

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Michael S. Tsirkin]

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

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Laurent Vivier]

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

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Igor Mammedov]

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  
> 

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Michael S. Tsirkin]

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  
> > 

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Igor Mammedov]

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    
> > >   
> 

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Daniel P. Berrangé]

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 :|

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Michael S. Tsirkin]

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 :|

Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges

[Daniel P. Berrangé]

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 :|

Re: [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0

[Michael S. Tsirkin]

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