| Message ID | 20220905072531.8059-1-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled | expand |
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 ... > -- > 2.34.1
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 ...
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));
On Mon, 5 Sep 2022, Igor Mammedov wrote: > 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. The failure is only returned when the OS requests/probes native hotplug capability in CWD1. Also maybe I was mistaken but in the offline thread, I thought MST suggested we left the native hotplug bit as is without ever masking it. I guess he was coming from the fact that experiments indicated that when the native hotplug is off in OSC, Windows did not detect ATS. > 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). Yeah I also think this is a Windows bug. ATS and native hotplug does not seem to be related in anyway. > > > + /* > > + * 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)); > >
On Mon, 5 Sep 2022, Ani Sinha wrote: > > > > > > > > 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. > > The failure is only returned when the OS requests/probes native hotplug > capability in CWD1. I meant CWD3.
On Mon, 5 Sep 2022 22:25:25 +0530 (IST) Ani Sinha <ani@anisinha.ca> wrote: > On Mon, 5 Sep 2022, Ani Sinha wrote: > > > > > > > > > > > > > > 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. > > > > The failure is only returned when the OS requests/probes native hotplug > > capability in CWD1. > > I meant CWD3. For OSPM to know which features are denied by platform, the later must mask them in returned bitmask, how otherwise you would see above behavior. (or alternatively OSPM might ignore _OSC results and resort to workarounds/probing as result enabling native hotplug in which case you would see ATS detected). To verify which hotplug is used, you can just trace acpi_pci_* in QEMU and observe if it's used for unplug or not.
On Tue, 6 Sep 2022, Igor Mammedov wrote: > On Mon, 5 Sep 2022 22:25:25 +0530 (IST) > Ani Sinha <ani@anisinha.ca> wrote: > > > On Mon, 5 Sep 2022, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > 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. > > > > > > The failure is only returned when the OS requests/probes native hotplug > > > capability in CWD1. > > > > I meant CWD3. > For OSPM to know which features are denied by platform, the later must mask > them in returned bitmask, how otherwise you would see above behavior. In that case, mst's idea does not work, sadly. > (or alternatively OSPM might ignore _OSC results and resort to workarounds/probing > as result enabling native hotplug in which case you would see ATS > detected). > > To verify which hotplug is used, you can just trace acpi_pci_* in QEMU and observe > if it's used for unplug or not. > >
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)); + 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));
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(-)