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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep