From: David Woodhouse <dwmw@amazon.co.uk>

The emulated PIIX3 uses a nybble for the status of each PCI function,
so the status for e.g. slot 0 functions 0 and 1 respectively can be
read as (\_GPE.PH00 & 0x0F), and (\_GPE.PH00 >> 0x04).

The AML that Xen gives to a guest gets the operand order for the odd-
numbered functions the wrong way round, returning (0x04 >> \_GPE.PH00)
instead.

As far as I can tell, this was the wrong way round in Xen from the
moment that PCI hotplug was first introduced in commit 83d82e6f35a8:

+                    ShiftRight (0x4, \_GPE.PH00, Local1)
+                    Return (Local1) /* IN status as the _STA */

Or maybe there's bizarre AML operand ordering going on there, like
Intel's wrong-way-round assembler, and it only broke later when it was
changed to being generated?

Either way, it's definitely wrong now, and instrumenting a Linux guest
shows that it correctly sees _STA being 0x00 in function 0 of an empty
slot, but then the loop in acpiphp_glue.c::get_slot_status() goes on to
look at function 1 and sees that _STA evaluates to 0x04. Thus reporting
an adapter is present in every slot in /sys/bus/pci/slots/*

Quite why Linux wants to look for function 1 being physically present
when function 0 isn't... I don't want to think about right now.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Fixes: 83d82e6f35a8 ("hvmloader: pass-through: multi-function PCI hot-plug")
---
Utterly untested in Xen. Tested the same change in a different
environment which is using precisely the *same* AML for guest
compatibility.

diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 1176da80ef..1d27809116 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -431,7 +431,7 @@ int main(int argc, char **argv)
                 stmt("Store", "0x89, \\_GPE.DPT2");
             }
             if ( slot & 1 )
-                stmt("ShiftRight", "0x4, \\_GPE.PH%02X, Local1", slot & ~1);
+                stmt("ShiftRight", "\\_GPE.PH%02X, 0x04, Local1", slot & ~1);
             else
                 stmt("And", "\\_GPE.PH%02X, 0x0f, Local1", slot & ~1);
             stmt("Return", "Local1"); /* IN status as the _STA */

On 17.03.2023 11:32, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The emulated PIIX3 uses a nybble for the status of each PCI function,
> so the status for e.g. slot 0 functions 0 and 1 respectively can be
> read as (\_GPE.PH00 & 0x0F), and (\_GPE.PH00 >> 0x04).
> 
> The AML that Xen gives to a guest gets the operand order for the odd-
> numbered functions the wrong way round, returning (0x04 >> \_GPE.PH00)
> instead.
> 
> As far as I can tell, this was the wrong way round in Xen from the
> moment that PCI hotplug was first introduced in commit 83d82e6f35a8:
> 
> +                    ShiftRight (0x4, \_GPE.PH00, Local1)
> +                    Return (Local1) /* IN status as the _STA */
> 
> Or maybe there's bizarre AML operand ordering going on there, like
> Intel's wrong-way-round assembler, and it only broke later when it was
> changed to being generated?
> 
> Either way, it's definitely wrong now, and instrumenting a Linux guest
> shows that it correctly sees _STA being 0x00 in function 0 of an empty
> slot, but then the loop in acpiphp_glue.c::get_slot_status() goes on to
> look at function 1 and sees that _STA evaluates to 0x04. Thus reporting
> an adapter is present in every slot in /sys/bus/pci/slots/*
> 
> Quite why Linux wants to look for function 1 being physically present
> when function 0 isn't... I don't want to think about right now.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Fixes: 83d82e6f35a8 ("hvmloader: pass-through: multi-function PCI hot-plug")

Reviewed-by: Jan Beulich <jbeulich@suse.com>

On 17/03/2023 10:32, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The emulated PIIX3 uses a nybble for the status of each PCI function,
> so the status for e.g. slot 0 functions 0 and 1 respectively can be
> read as (\_GPE.PH00 & 0x0F), and (\_GPE.PH00 >> 0x04).
> 
> The AML that Xen gives to a guest gets the operand order for the odd-
> numbered functions the wrong way round, returning (0x04 >> \_GPE.PH00)
> instead.
> 
> As far as I can tell, this was the wrong way round in Xen from the
> moment that PCI hotplug was first introduced in commit 83d82e6f35a8:
> 
> +                    ShiftRight (0x4, \_GPE.PH00, Local1)
> +                    Return (Local1) /* IN status as the _STA */
> 
> Or maybe there's bizarre AML operand ordering going on there, like
> Intel's wrong-way-round assembler, and it only broke later when it was
> changed to being generated?
> 
> Either way, it's definitely wrong now, and instrumenting a Linux guest
> shows that it correctly sees _STA being 0x00 in function 0 of an empty
> slot, but then the loop in acpiphp_glue.c::get_slot_status() goes on to
> look at function 1 and sees that _STA evaluates to 0x04. Thus reporting
> an adapter is present in every slot in /sys/bus/pci/slots/*
> 
> Quite why Linux wants to look for function 1 being physically present
> when function 0 isn't... I don't want to think about right now.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Fixes: 83d82e6f35a8 ("hvmloader: pass-through: multi-function PCI hot-plug")
> ---
> Utterly untested in Xen. Tested the same change in a different
> environment which is using precisely the *same* AML for guest
> compatibility.
> 

This AML only relates to the hotplug controller for qemu-trad so it's 
unlikely anyone particularly cares any more. In fact I'm kind of 
surprised the generation code still exists.

   Paul

> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> index 1176da80ef..1d27809116 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -431,7 +431,7 @@ int main(int argc, char **argv)
>                   stmt("Store", "0x89, \\_GPE.DPT2");
>               }
>               if ( slot & 1 )
> -                stmt("ShiftRight", "0x4, \\_GPE.PH%02X, Local1", slot & ~1);
> +                stmt("ShiftRight", "\\_GPE.PH%02X, 0x04, Local1", slot & ~1);
>               else
>                   stmt("And", "\\_GPE.PH%02X, 0x0f, Local1", slot & ~1);
>               stmt("Return", "Local1"); /* IN status as the _STA */
>

On 20.03.2023 10:04, Paul Durrant wrote:
> On 17/03/2023 10:32, David Woodhouse wrote:
>> From: David Woodhouse <dwmw@amazon.co.uk>
>>
>> The emulated PIIX3 uses a nybble for the status of each PCI function,
>> so the status for e.g. slot 0 functions 0 and 1 respectively can be
>> read as (\_GPE.PH00 & 0x0F), and (\_GPE.PH00 >> 0x04).
>>
>> The AML that Xen gives to a guest gets the operand order for the odd-
>> numbered functions the wrong way round, returning (0x04 >> \_GPE.PH00)
>> instead.
>>
>> As far as I can tell, this was the wrong way round in Xen from the
>> moment that PCI hotplug was first introduced in commit 83d82e6f35a8:
>>
>> +                    ShiftRight (0x4, \_GPE.PH00, Local1)
>> +                    Return (Local1) /* IN status as the _STA */
>>
>> Or maybe there's bizarre AML operand ordering going on there, like
>> Intel's wrong-way-round assembler, and it only broke later when it was
>> changed to being generated?
>>
>> Either way, it's definitely wrong now, and instrumenting a Linux guest
>> shows that it correctly sees _STA being 0x00 in function 0 of an empty
>> slot, but then the loop in acpiphp_glue.c::get_slot_status() goes on to
>> look at function 1 and sees that _STA evaluates to 0x04. Thus reporting
>> an adapter is present in every slot in /sys/bus/pci/slots/*
>>
>> Quite why Linux wants to look for function 1 being physically present
>> when function 0 isn't... I don't want to think about right now.
>>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> Fixes: 83d82e6f35a8 ("hvmloader: pass-through: multi-function PCI hot-plug")
>> ---
>> Utterly untested in Xen. Tested the same change in a different
>> environment which is using precisely the *same* AML for guest
>> compatibility.
>>
> 
> This AML only relates to the hotplug controller for qemu-trad so it's 
> unlikely anyone particularly cares any more. In fact I'm kind of 
> surprised the generation code still exists.

Why would it not exist anymore? Use of qemu-trad is deprecated and
advised against, but it's still possible to use it. Otherwise quite a
bit of cleanup in libxl could also happen, for example.

Jan

On 20/03/2023 10:34, Jan Beulich wrote:
> On 20.03.2023 10:04, Paul Durrant wrote:
>> On 17/03/2023 10:32, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> The emulated PIIX3 uses a nybble for the status of each PCI function,
>>> so the status for e.g. slot 0 functions 0 and 1 respectively can be
>>> read as (\_GPE.PH00 & 0x0F), and (\_GPE.PH00 >> 0x04).
>>>
>>> The AML that Xen gives to a guest gets the operand order for the odd-
>>> numbered functions the wrong way round, returning (0x04 >> \_GPE.PH00)
>>> instead.
>>>
>>> As far as I can tell, this was the wrong way round in Xen from the
>>> moment that PCI hotplug was first introduced in commit 83d82e6f35a8:
>>>
>>> +                    ShiftRight (0x4, \_GPE.PH00, Local1)
>>> +                    Return (Local1) /* IN status as the _STA */
>>>
>>> Or maybe there's bizarre AML operand ordering going on there, like
>>> Intel's wrong-way-round assembler, and it only broke later when it was
>>> changed to being generated?
>>>
>>> Either way, it's definitely wrong now, and instrumenting a Linux guest
>>> shows that it correctly sees _STA being 0x00 in function 0 of an empty
>>> slot, but then the loop in acpiphp_glue.c::get_slot_status() goes on to
>>> look at function 1 and sees that _STA evaluates to 0x04. Thus reporting
>>> an adapter is present in every slot in /sys/bus/pci/slots/*
>>>
>>> Quite why Linux wants to look for function 1 being physically present
>>> when function 0 isn't... I don't want to think about right now.
>>>
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>> Fixes: 83d82e6f35a8 ("hvmloader: pass-through: multi-function PCI hot-plug")
>>> ---
>>> Utterly untested in Xen. Tested the same change in a different
>>> environment which is using precisely the *same* AML for guest
>>> compatibility.
>>>
>>
>> This AML only relates to the hotplug controller for qemu-trad so it's
>> unlikely anyone particularly cares any more. In fact I'm kind of
>> surprised the generation code still exists.
> 
> Why would it not exist anymore? Use of qemu-trad is deprecated and
> advised against, but it's still possible to use it. Otherwise quite a
> bit of cleanup in libxl could also happen, for example.
> 

Right. I'm just surprised that is not done already... seems like a while 
since trad was deprecated; I'd have thought it could be removed in the 
next release.

   Paul