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: <2fc282b0-97e4-448c-a77e-0ed63746d0a1@amd.com>
Date: Thu, 1 Aug 2024 23:58:46 -0500
From: Wei Huang <wei.huang2@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
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 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