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

[Wei Huang]

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.

<snip>

> > +
> > +/**
> > + * 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()?

> > 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?

> > +static inline int pcie_tph_get_st_from_acpi(struct pci_dev *dev, unsigned int cpu_acpi_uid,
> > +					    enum tph_mem_type tag_type, u8 req_enable,
> > +					    u16 *tag)
> > +{ return false; }
>
> "false" is not "int".
>
> Apparently you want to return "success" in this case, when
> CONFIG_PCIE_TPH is not enabled?  I suggested leaving this non-exported
> for now, which would mean removing this altogether.  But if/when we do
> export it, I think maybe it should return error so a caller doesn't
> assume it succeeded, since *tag will be garbage.
>
> Bjorn