Re: [PATCH] Fix PCI hotplug AML

[Paul Durrant <xadimgnik@xxxxxxxxx>]

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@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > 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