* [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
@ 2022-09-05 7:25 Ani Sinha
2022-09-05 10:53 ` Michael S. Tsirkin
2022-09-05 15:52 ` Igor Mammedov
0 siblings, 2 replies; 8+ messages in thread
From: Ani Sinha @ 2022-09-05 7:25 UTC (permalink / raw)
To: Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Marcel Apfelbaum
Cc: qemu-devel, kkostiuk, yvugenfi, yiwei, ybendito, jusual
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));
+ 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));
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-05 7:25 [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled Ani Sinha
@ 2022-09-05 10:53 ` Michael S. Tsirkin
2022-09-05 12:57 ` Ani Sinha
2022-09-05 15:52 ` Igor Mammedov
1 sibling, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2022-09-05 10:53 UTC (permalink / raw)
To: Ani Sinha
Cc: Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
Marcel Apfelbaum, qemu-devel, kkostiuk, yvugenfi, yiwei, ybendito,
jusual
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-05 10:53 ` Michael S. Tsirkin
@ 2022-09-05 12:57 ` Ani Sinha
0 siblings, 0 replies; 8+ messages in thread
From: Ani Sinha @ 2022-09-05 12:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Ani Sinha, Igor Mammedov, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Marcel Apfelbaum, qemu-devel, kkostiuk, yvugenfi,
yiwei, ybendito, jusual
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 ...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-05 7:25 [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled Ani Sinha
2022-09-05 10:53 ` Michael S. Tsirkin
@ 2022-09-05 15:52 ` Igor Mammedov
2022-09-05 16:50 ` Ani Sinha
1 sibling, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2022-09-05 15:52 UTC (permalink / raw)
To: Ani Sinha
Cc: Michael S. Tsirkin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Marcel Apfelbaum, qemu-devel, kkostiuk, yvugenfi,
yiwei, ybendito, jusual
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));
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-05 15:52 ` Igor Mammedov
@ 2022-09-05 16:50 ` Ani Sinha
2022-09-05 16:55 ` Ani Sinha
0 siblings, 1 reply; 8+ messages in thread
From: Ani Sinha @ 2022-09-05 16:50 UTC (permalink / raw)
To: Igor Mammedov
Cc: Ani Sinha, Michael S. Tsirkin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Marcel Apfelbaum, qemu-devel, kkostiuk, yvugenfi,
yiwei, ybendito, jusual
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));
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-05 16:50 ` Ani Sinha
@ 2022-09-05 16:55 ` Ani Sinha
2022-09-06 7:39 ` Igor Mammedov
0 siblings, 1 reply; 8+ messages in thread
From: Ani Sinha @ 2022-09-05 16:55 UTC (permalink / raw)
To: Ani Sinha
Cc: Igor Mammedov, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Marcel Apfelbaum, qemu-devel,
kkostiuk, yvugenfi, yiwei, ybendito, jusual
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-05 16:55 ` Ani Sinha
@ 2022-09-06 7:39 ` Igor Mammedov
2022-09-06 7:45 ` Ani Sinha
0 siblings, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2022-09-06 7:39 UTC (permalink / raw)
To: Ani Sinha
Cc: Michael S. Tsirkin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Marcel Apfelbaum, qemu-devel, kkostiuk, yvugenfi,
yiwei, ybendito, jusual
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled
2022-09-06 7:39 ` Igor Mammedov
@ 2022-09-06 7:45 ` Ani Sinha
0 siblings, 0 replies; 8+ messages in thread
From: Ani Sinha @ 2022-09-06 7:45 UTC (permalink / raw)
To: Igor Mammedov
Cc: Ani Sinha, Michael S. Tsirkin, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Marcel Apfelbaum, qemu-devel, kkostiuk, yvugenfi,
yiwei, ybendito, jusual
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.
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-06 7:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-05 7:25 [RFC PATCH] hw/acpi: do not let OSPM set pcie native hotplug when acpi hotplug is enabled Ani Sinha
2022-09-05 10:53 ` Michael S. Tsirkin
2022-09-05 12:57 ` Ani Sinha
2022-09-05 15:52 ` Igor Mammedov
2022-09-05 16:50 ` Ani Sinha
2022-09-05 16:55 ` Ani Sinha
2022-09-06 7:39 ` Igor Mammedov
2022-09-06 7:45 ` Ani Sinha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).