Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled

[Ani Sinha]

On Mon, 5 Sep 2022, Michael S. Tsirkin wrote:

> On Mon, Sep 05, 2022 at 12:55:31PM +0530, Ani Sinha wrote:
> > Possible fix for https://bugzilla.redhat.com/show_bug.cgi?id=2089545
> >
> > Change in AML:
> >
> > @@ -47,33 +47,39 @@
> >      Scope (_SB)
> >      {
> >          Device (PCI0)
> >          {
> >              Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // 
> > _HID: Hardware ID
> >              Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: 
> > Compatible ID
> >              Name (_ADR, Zero)  // _ADR: Address
> >              Name (_UID, Zero)  // _UID: Unique ID
> >              Method (_OSC, 4, NotSerialized)  // _OSC: Operating System 
> > Capabilities
> >              {
> >                  CreateDWordField (Arg3, Zero, CDW1)
> >                  If ((Arg0 == ToUUID 
> > ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> >                  {
> >                      CreateDWordField (Arg3, 0x04, CDW2)
> >                      CreateDWordField (Arg3, 0x08, CDW3)
> >                      Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> > -                    Local0 &= 0x1E
> > +                    Local0 &= 0x1F
> > +                    Local1 = (CDW3 & One)
> > +                    If ((One == Local1))
> > +                    {
> > +                        CDW1 |= 0x12
> > +                    }
> > +
> >                      If ((Arg1 != One))
> >                      {
> >                          CDW1 |= 0x08
> >                      }
> >
> >                      If ((CDW3 != Local0))
> >                      {
> >                          CDW1 |= 0x10
> >                      }
> >
> >                      CDW3 = Local0
> >                  }
> >                  Else
> >                  {
> >                      CDW1 |= 0x04
> >                  }
> > **
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/i386/acpi-build.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 0355bd3dda..3dc9379f27 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1348,10 +1348,12 @@ static Aml *build_q35_osc_method(bool 
> > enable_native_pcie_hotplug)
> >  {
> >      Aml *if_ctx;
> >      Aml *if_ctx2;
> > +    Aml *if_ctx3;
> >      Aml *else_ctx;
> >      Aml *method;
> >      Aml *a_cwd1 = aml_name("CDW1");
> >      Aml *a_ctrl = aml_local(0);
> > +    Aml *a_pcie_nhp_ctl = aml_local(1);
> >
> >      method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> >      aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), 
> > "CDW1"));
> > @@ -1366,11 +1368,26 @@ static Aml *build_q35_osc_method(bool 
> > enable_native_pcie_hotplug)
> >      /*
> >       * Always allow native PME, AER (no dependencies)
> >       * Allow SHPC (PCI bridges can have SHPC controller)
> > -     * Disable PCIe Native Hot-plug if ACPI PCI Hot-plug is enabled.
> >       */
> > -    aml_append(if_ctx, aml_and(a_ctrl,
> > -        aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)), a_ctrl));
> > +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> >
> > +    /*
> > +     * if ACPI PCI Hot-plug is enabled, do not let OSPM set OSC PCIE
> > +     * Native hotplug ctrl bit.
> > +     */
> > +    if (!enable_native_pcie_hotplug) {
> > +        /* check if the ACPI native hotplug bit is set by the OS in DWORD3 
> > */
> > +        aml_append(if_ctx, aml_and(aml_name("CDW3"),
> > +                                   aml_int(0x01), a_pcie_nhp_ctl));
> > +        if_ctx3 = aml_if(aml_equal(aml_int(1), a_pcie_nhp_ctl));
> > +        /*
> > +         * ACPI spec 5.1, section 6.2.11
> > +         * bit 1 in first DWORD - _OSC failure
> > +         * bit 4 in first DWORD - capabilities masked
> > +         */
> > +        aml_append(if_ctx3, aml_or(a_cwd1, aml_int(0x12), a_cwd1));
>
>
> 0x12 ->
>
>  ( 0x1 << 4 ) /* _OSC failure */ | ( 0x1 << 1) /* capabilities masked */
>
>
> > +        aml_append(if_ctx, if_ctx3);
> > +    }
> >      if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> >      /* Unknown revision */
> >      aml_append(if_ctx2, aml_or(a_cwd1, aml_int(0x08), a_cwd1));
>
> Hmm wait a sec
>
>
>     if_ctx2 = aml_if(aml_lnot(aml_equal(aml_name("CDW3"), a_ctrl)));
>     /* Capabilities bits were masked */
>     aml_append(if_ctx2, aml_or(a_cwd1, aml_int(0x10), a_cwd1));
>     aml_append(if_ctx, if_ctx2);
>
>
> this one seems subtly different ...

I guess what this code is trying to say is that if the requested
capabilities by the driver is not the same as the one we are returning, we
are masking some of the capabilities. This seems to be a superset of my
change above and does not break it. Unless I am reading this wrong ...