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

[Igor Mammedov]

On Mon,  5 Sep 2022 12:55:31 +0530
Ani Sinha <ani@anisinha.ca> 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));

that makes us not actually mask any capabilities since you forgot to mask
bit 1 later under if_ctx3 context.

So OSPM will see a permanent failure (_OSC failure bit in CWD1)
and will have no idea that PCI Hotplug is not supported since we return CWD3
with this bit still set whoever much it tries to negotiate.

So if it boots at all, guest will probably not use any requested features
since _OSC failed to confirm any without error.

And even if we clear hotplug bit it still doesn't help.

some more testing shows that ATS cap doesn't depended hard on native hotplug
(i.e. run QEMU with native hotplug enabled but disable hotplug on root-port in 
question)
To me it still looks like a bug in Windows' acpi hotplug impl
(or perhaps it's no more properly maintained, so it doesn't account for new 
features).

> +    /*
> +     * 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));
> +        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));