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  
>