Re: [PATCH] ACPI: PPTT: Fix table length check when parsing processor nodes

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 7 May 2025 12:55:00 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> On Wed, May 07, 2025 at 01:51:58PM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 7, 2025 at 1:47 PM Sudeep Holla <sudeep.holla@arm.com> wrote:  
> > >
> > > On Wed, May 07, 2025 at 01:44:26PM +0200, Rafael J. Wysocki wrote:  
> > > > On Wed, May 7, 2025 at 1:40 PM Sudeep Holla <sudeep.holla@arm.com> wrote:  
> > > > >
> > > > > On Wed, May 07, 2025 at 11:51:24AM +0800, Yicong Yang wrote:  
> > > > > > From: Yicong Yang <yangyicong@hisilicon.com>
> > > > > >
> > > > > > Below error is met on my board and QEMU VM on SMT or non-SMT machine:
> > > > > >   ACPI PPTT: PPTT table found, but unable to locate core 31 (31)
> > > > > >
> > > > > > This is because the processor node is found by iterating the PPTT
> > > > > > table under condition (for both acpi_find_processor_node() and
> > > > > > acpi_pptt_leaf_node()):
> > > > > >   while (entry + proc_sz < table_end)
> > > > > >     [parse the processor node]
> > > > > >
> > > > > > If the last processor node is happened to be the last node in the
> > > > > > PPTT table, above condition will always be false since
> > > > > > entry + proc_sz == table_end. Thus the last CPU is not parsed.
> > > > > > Fix the loop condition to resolve the issue.
> > > > > >
> > > > > > This issue is exposed by [1] but the root cause is explained above.
> > > > > > Before [1] entry + proc_sz is always smaller than table_end.
> > > > > >  
> > > > >
> > > > > Another thread [1]  with similar patch.  
> > > >
> > > > OK, so is this a correct fix?  
> > >
> > > While it may fix the issue on the surface, I just want to be sure there
> > > are no other issues with the PPTT table presented from the firmware.
> > > I will asked some questions on that thread before I can agree on the solution.  
> > 
> > Yeah, it looks like table_end points to the last byte of the table
> > instead of pointing to the first byte after the end of the table.  
> 
> Indeed and also we should have private resources like L1 cache described
> after the initial 20 bytes of the node. So I am bit worried if this will
> just hide other problems while it may solve this problem by looks of it.
> This example doesn't look like a proper PPTT matching real systems.
> 

Assuming I'm understanding the bug correctly...

SMT systems will hit this. There will typically be no private resources
for a thread as the L1I/D shared by multiple threads (which are processor
nodes IIRC).  Note we are trying to improve the cache description in QEMU
at the moment as it would definitely be better to present caches in PPTT,
but that isn't the main issue here.

Jonathan