Re: [PATCH] ACPI/PPTT: fix off-by-one error

Heyne, Maximilian
2025-05-07T14:29:25Z

On Wed, May 07, 2025 at 01:56:53PM +0100, Sudeep Holla wrote:
> On Wed, May 07, 2025 at 12:42:14PM +0000, Heyne, Maximilian wrote:
> > On Wed, May 07, 2025 at 01:30:53PM +0100, Sudeep Holla wrote:
> > > On Wed, May 07, 2025 at 11:56:48AM +0000, Heyne, Maximilian wrote:
> > > > On Wed, May 07, 2025 at 12:52:18PM +0100, Sudeep Holla wrote:
> > > > >
> > > > > Just to understand, this node is absolutely processor node with no
> > > > > private resources ? I find it hard to trust this as most of the CPUs
> > > > > do have L1 I&D caches. If they were present the table can't abruptly end
> > > > > like this.
> > > >
> > > > Yes looks like it. In our case the ACPI subtable has length 0x14 which is
> > > > exactly sizeof(acpi_pptt_processor).
> > > >
> > >
> > > OK, this seem like it is emulated platform with no private resources as
> > > it is specified in the other similar patch clearly(QEMU/VM). So this
> > > doesn't match real platforms. Your PPTT is wrong if it is real hardware
> > > platform as you must have private resources.
> > >
> > > Anyways if we allow emulation to present CPUs without private resources
> > > we may have to consider allowing this as the computed pointer will match
> > > the table end.
> >
> > Is there a need by the ACPI specification that the Cache information
> > must come after the processor information? Because on our platform there
> > is Cache and it's described but at a different location seemingly. It
> > looks like caches are described first and then the CPUs.
> >
>
> That is fine but you must have reference to those caches in the processor
> node and the length of the node won't be 0x14 in that case and you shouldn't
> hit this issue. So if this is real platform, then yes I am must say you
> PPTT is wrong especially if there are caches in the table as you say just
> that processor nodes are not pointing to them correctly then ?

The ACPI tables in our case describe a core first which references the
cache as private resource and then a thread whose parent is the core but
this doesn't have a private resource. This is how it looks like:

[C8Eh 3214 1] Subtable Type : 00 [Processor Hierarchy Node]
[C8Fh 3215 1] Length : 1C
[C90h 3216 2] Reserved : 0000
[C92h 3218 4] Flags (decoded below) : 00000002
Physical package : 0
ACPI Processor ID valid : 1
Processor is a thread : 0
Node is a leaf : 0
Identical Implementation : 0
[C96h 3222 4] Parent : 000000A2
[C9Ah 3226 4] ACPI Processor ID : 0000003F
[C9Eh 3230 4] Private Resource Number : 00000002
[CA2h 3234 4] Private Resource : 00000072
[CA6h 3238 4] Private Resource : 0000008A

[CAAh 3242 1] Subtable Type : 00 [Processor Hierarchy Node]
[CABh 3243 1] Length : 14
[CACh 3244 2] Reserved : 0000
[CAEh 3246 4] Flags (decoded below) : 0000000E
Physical package : 0
ACPI Processor ID valid : 1
Processor is a thread : 1
Node is a leaf : 1
Identical Implementation : 0
[CB2h 3250 4] Parent : 00000C8E
[CB6h 3254 4] ACPI Processor ID : 0000003F
[CBAh 3258 4] Private Resource Number : 00000000

>
> > I can try to drill even deeper here if you insist. As said I'm no
> > subject matter expert here. But is there something obviously wrong with
> > my patch or would it be ok to just take it?
> >
>
> Yes you much check your PPTT if it is real hardware platform. I am OK
> with the change in terms of QEMU or VM. You may need to reword commit
> message a bit. I will respond separately.
>
> --
> Regards,
> Sudeep

Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597