Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

On Wed, Jul 01, 2026 at 03:13:42PM +0100, Sudeep Holla wrote:
> On Wed, Jul 01, 2026 at 02:34:10PM +0100, Sudeep Holla wrote:
> > On Wed, Jul 01, 2026 at 12:44:30PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > On Tue, Jun 30, 2026 at 7:00 PM Sudeep Holla <sudeep.holla@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > ACPI defines a PCC OperationRegion as the shared memory fields that
> > > > > > follow the PCC signature. The OperationRegion length is likewise the
> > > > > > total size of the fields that succeed the signature in shared memory.
> > > > > >
> > > > > > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > > > > > populated by the platform and verified by OSPM.
> > > > > >
> > > > > > The PCC address space handler currently copies the OperationRegion
> > > > > > buffer to and from the start of the PCC shared memory region. That can
> > > > > > overwrite or expose the signature at byte offset 0, and it also misses
> > > > > > the last 4 bytes of the actual PCC OperationRegion data.
> > > > > >
> > > > > > Offset OperationRegion copies by the size of the signature and reject
> > > > > > regions that do not fit in the shared memory after that signature.
> > > > > >
> > > > > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > > > > > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxxxxx>
> > > > >
> > > > > Does this depend on anything else in the series or is it independent?
> > > > >
> > > >
> > > > It is independent. This just fixes the issue of including the signature in
> > > > the opregion data. It can go independently as all other patches as well.
> > > 
> > > So I've looked at Sashiko's feedback on this change:
> > > 
> > > https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc%40kernel.org
> > > 
> > > and I'm a bit worried.
> > > 
> > > Of course, the preexisting issues found by it will need to be
> > > addressed separately, but what about firmware released before the
> > > clarification in ACPI 6.6 and the possible misalignment issue
> > > mentioned by it?
> > 
> > I haven't analysed all the report from Sashiko yet. But unlike other patches
> > in the series, this one is not strictly to validate the signature.
> > 
> > Even before v6.6, the PCC Opregion didn't include the signature which is
> > at offset 0. The code always started copying to/from offset 0 missing the
> > last 32-bit field(word). Since v6.3 when PCC Opregion was added we have
> > this in the specification:
> > 
> >   | <section> Declaring message fields within a PCC OperationRegion
> >   |
> >   | For all PCC subspace types, the PCC Operation Region pertains to the
> >   | region of PCC subspace that succeeds the PCC signature.
> > 
> > So, this patch addresses that issue of wrong starting offset used by this
> > driver.
> > 
> > Other patches I agree, it is more likely we just need to warn and not
> > reject absence of wrong PCC signatures.
> > 
> > I will take a look at the other issues reported by Sashiko soon.
> > 
> 
> Forgot to add that let us wait at-least until I go through the report
> understand and conclude it is not fatal/critical before we can merge this.
> Indeed a quick read concerned as much that I would like to spend time
> analysing it carefully.
> 

Looking at the comments and digging ACPICA in more detail, I found the issue
exist there as well and that is the reason why we hadn't observed it in the
kernel. I will first post the fix for that and follow up here.

Thanks.

-- 
Regards,
Sudeep