Re: [PATCH V3 06/10] PCI/TPH: Introduce API to retrieve TPH steering tags from ACPI

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Aug 01, 2024 at 11:58:46PM -0500, Wei Huang wrote:
> On 7/23/24 17:22, Bjorn Helgaas wrote:
> > > + * The st_info struct defines the steering tag returned by the firmware _DSM
> > > + * method defined in PCI Firmware Spec r3.3, sect 4.6.15 "_DSM to Query Cache
> > > + * Locality TPH Features"
> > 
> > I don't know what I'm missing, but my copy of the r3.3 spec, dated Jan
> > 20, 2021, doesn't have sec 4.6.15.
> 
> According to https://members.pcisig.com/wg/PCI-SIG/document/15470,
> the revision has "4.6.15. _DSM to Query Cache Locality TPH
> Features". PCI-SIG approved this ECN, but haven't merged it into PCI
> Firmware Specification 3.3 yet.

Thanks for the pointer.  Please update to refer to this as an approved
ECN to r3.3 and include the URL.  When it is eventually incorporated
into a PCI Firmware spec revision, it will not be r3.3.  It will
likely be r3.4 or r4.0, so r3.3 will never be the correct citation.

> > > + * pcie_tph_get_st_from_acpi() - Retrieve steering tag for a specific CPU
> > > + * using platform ACPI _DSM
> > 
> > 1) TPH and Steering Tags are not ACPI-specific, even though the only
> > current mechanism to learn the tags happens to be an ACPI _DSM, so I
> > think we should omit "acpi" from the name drivers use.
> > 
> > 2) The spec doesn't restrict Steering Tags to be for a CPU; it says
> > "processing resource such as a host processor or system cache
> > hierarchy ..."  But obviously this interface only comprehends an ACPI
> > CPU ID.  Maybe the function name should include "cpu".
> 
> How about pcie_tph_get_st_for_cpu() or pcie_tph_retreive_st_for_cpu()?

Sounds good.  The former is nice because it's shorter.
"pcie_tph_cpu_st()" would even be fine with me.  s/retreive/retrieve/
if you use that.

> > > diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
> > > index 854677651d81..b12a592f3d49 100644
> > > --- a/include/linux/pci-tph.h
> > > +++ b/include/linux/pci-tph.h
> > > @@ -9,15 +9,27 @@
> > >   #ifndef LINUX_PCI_TPH_H
> > >   #define LINUX_PCI_TPH_H
> > > +enum tph_mem_type {
> > > +	TPH_MEM_TYPE_VM,	/* volatile memory type */
> > > +	TPH_MEM_TYPE_PM		/* persistent memory type */
> > 
> > Where does this come from?  I don't see "vram" or "volatile" used in
> > the PCIe spec in this context.  Maybe this is from the PCI Firmware
> > spec?
> 
> Yes, this is defined in the ECN mentioned above. Do you have
> concerns about defining them here? If we want to remove it, then
> pcie_tph_get_st_from_acpi() function can only support one memory
> type (e.g.  vram). Any advice?

No concerns if they're defined in the ECN, but we need a citation so
we know where to look for these values.  Cite the ECN for now, and we
can update to the actual firmware spec citation when it becomes
available.

Bjorn