Re: PCI Hotplug ACPI device names only 3 characters long

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 5 Sep 2023 16:43:54 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 05, 2023 at 07:45:12PM +0200, Marcello Sylverster Bauer wrote:
> > Hi Michael,
> > 
> > On 9/5/23 18:44, Michael S. Tsirkin wrote:  
> > > On Tue, Sep 05, 2023 at 05:05:33PM +0200, Marcello Sylverster Bauer wrote:  
> > > > Greetings,
> > > > 
> > > > I'm currently working on a project to support Intel IPU6 in QEMU via VFIO so
> > > > that the guest system can access the camera. This requires extending the
> > > > ACPI device definition so that the guest knows how to access the camera.
> > > > 
> > > > However, I cannot extend the PCI devices because their names are not 4
> > > > characters long and therefore do not follow the ACPI specification.
> > > > 
> > > > When I use '-acpitable' to include my own SSDT for the IPU6 PCI device, it
> > > > does not allow me to declare the device as an External Object because it
> > > > automatically adds padding underscores.
> > > > 
> > > > e.g.
> > > > Before:
> > > > ```
> > > > External(_SB.PCI0.S18.SA0, DeviceObj)
> > > > ```
> > > > After:
> > > > ```
> > > > External(_SB.PCI0.S18_.SA0_, DeviceObj)
> > > > ```
> > > > 
> > > > Adding the underscore padding is hard coded in iASL and also in QEMU when
> > > > parsing an ASL file. (see: build_append_nameseg())
> > > > 
> > > > So here are my questions:
> > > > 1. Is there a solution to extend the ACPI PCI device using '-acpitable'
> > > > without having to patch iASL or QEMU?
> > > > 2. Are there any plans to change the names to comply with the ACPI spec?
> > > > (e.g. use "S%.03X" format string instead)
> > > > 
> > > > Thanks
> > > > Marcello  
> > > 
> > > 
> > > 1.  All names in ACPI are always exactly 4 characters long. _ is a legal character
> > >      but names beginning with _ are reserved.  
> > 
> > Exactly, which is why I want to address this issue here. Currently, Qemu
> > generates ACPI device names with only 3 characters. (See
> > build_append_pci_bus_devices() in hw/i386/acpi-build.c).
> > For example, the device I want to append entries to has the path
> > "_SB.PCI0.S18.SA0", but I can't because of the two auto-generated devices
> > with only 3 characters in their names.  

if your ssdt really has shorter names. You can use iasl to de-compile it
wiht bytte code included and check manually if if namesegment(s) are short
or padded with '_'.
(ex: iasl -d -l DSDT)

> 
> They are 4 characters otherwise OSPMs wouldn't work.
> In your example the path is _SB.PCI0.S18_.SA0_ - you disassembler probably
> just helpfully hides it for readability.
> 
> > > There's no rule in ACPI
> > >      spec that says they need to follow S%.03X or any other specific format.
> > >      I'm pretty sure we do follow the ACPI specification in this but feel free to
> > >      prove me wrong.  
> > 
> > You have misunderstood me. Currently, Qemu uses the following format to
> > create PCI ACPI devices:
> > 
> > ```
> > aml_name("S%.02X", devfn)
> > ```
> > 
> > My question is whether we should change it to something that results in a 4
> > character name like "S%.03X" or "S%.02X_".  
> 
> I think you misunderstand the code. Look at build_append_nameseg and you will
> see that the name is always ACPI_NAMESEG_LEN characters which equals 4.

ACPI 6.5 spec says this:

20.2.2 Name Objects Encoding
...
NameSeg :=
...
// Notice that NameSegs shorter than 4 characters are filled with trailing underscores (‘_’s).

that's what QEMU does if user has supplied shorter than 4 characters name

> 
> > I have tested it and it works fine as long as any hardcoded path references
> > are adjusted. But I'm not 100% sure if this could cause any regressions.
> >   
> > > 2.  You can probably add something to existing ACPI devices using Scope().  
> > 
> > I'm pretty sure the external object is required when loading a separate
> > SSDT, but I'll try by just using scopes.
> >   
> > >      I would not advise relying on this - current names are not a stable
> > >      interface that we guarantee across QEMU versions.
> > >      If adding this functionality is desirable, I think we'll need some new interface
> > >      to set a stable ACPI name. Maybe using aliases.  
> > 
> > Currently I'm just working on a PoW to get IPU6 working in QEMU, so
> > instability is fine.
> > 
> > Thanks,
> > Marcello
> >   
> > > 
> > >   
>