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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240802165842.GA154146@bhelgaas>
Date: Fri, 2 Aug 2024 11:58:42 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Wei Huang <wei.huang2@....com>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, netdev@...r.kernel.org,
	Jonathan.Cameron@...wei.com, corbet@....net, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	alex.williamson@...hat.com, gospo@...adcom.com,
	michael.chan@...adcom.com, ajit.khaparde@...adcom.com,
	somnath.kotur@...adcom.com, andrew.gospodarek@...adcom.com,
	manoj.panicker2@....com, Eric.VanTassell@....com,
	vadim.fedorenko@...ux.dev, horms@...nel.org, bagasdotme@...il.com,
	bhelgaas@...gle.com
Subject: Re: [PATCH V3 06/10] PCI/TPH: Introduce API to retrieve TPH steering
 tags from ACPI

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