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
