| 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
| ||
|
Message-ID: <b99685d9-9f3a-4c21-8d33-2eaa5de8be54@zhaoxin.com>
Date: Thu, 9 May 2024 17:06:52 +0800
From: LeoLiu-oc <leoliu-oc@...oxin.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <rafael@...nel.org>, <lenb@...nel.org>, <james.morse@....com>,
<tony.luck@...el.com>, <bp@...en8.de>, <bhelgaas@...gle.com>,
<robert.moore@...el.com>, <linux-acpi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
<acpica-devel@...ts.linux.dev>, <CobeChen@...oxin.com>,
<TonyWWang@...oxin.com>, <ErosZhang@...oxin.com>, <LeoLiu@...oxin.com>
Subject: Re: [PATCH v2 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()
在 2024/5/9 6:24, Bjorn Helgaas 写道:
>
>
> [这封邮件来自外部发件人 谨防风险]
>
> On Mon, Dec 18, 2023 at 11:04:30AM +0800, LeoLiu-oc wrote:
>> From: LeoLiuoc <LeoLiu-oc@...oxin.com>
>>
>> Call the func pci_acpi_program_hest_aer_params() for every PCIe device,
>> the purpose of this function is to extract register value from HEST PCIe
>> AER structures and program them into AER Capabilities.
>>
>> Signed-off-by: LeoLiuoc <LeoLiu-oc@...oxin.com>
>> ---
>> drivers/pci/pci-acpi.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>> drivers/pci/pci.h | 9 ++++
>> drivers/pci/probe.c | 1 +
>> 3 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 004575091596..3a183d5e20cb 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -18,6 +18,7 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/pm_qos.h>
>> #include <linux/rwsem.h>
>> +#include <acpi/apei.h>
>> #include "pci.h"
>>
>> /*
>> @@ -783,6 +784,103 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
>> return -ENODEV;
>> }
>>
>> +#ifdef CONFIG_ACPI_APEI
>> +static void program_hest_aer_endpoint(struct acpi_hest_aer_common aer_endpoint,
>> + struct pci_dev *dev, int pos)
>> +{
>> + u32 uncor_mask;
>> + u32 uncor_severity;
>> + u32 cor_mask;
>> + u32 adv_cap;
>> +
>> + uncor_mask = aer_endpoint.uncorrectable_mask;
>> + uncor_severity = aer_endpoint.uncorrectable_severity;
>> + cor_mask = aer_endpoint.correctable_mask;
>> + adv_cap = aer_endpoint.advanced_capabilities;
>> +
>> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK, uncor_mask);
>> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, uncor_severity);
>> + pci_write_config_dword(dev, pos + PCI_ERR_COR_MASK, cor_mask);
>> + pci_write_config_dword(dev, pos + PCI_ERR_CAP, adv_cap);
>
> This is named for "endpoint", but it is used for Root Ports,
> Endpoints, and PCIe to PCI/PCI-X bridges. It relies on these four
> fields being in the same place for all those HEST structures.
>
Change the function name " program_hest_aer_endpoint " to
"program_hest_aer_common" and the parameters of the function
"aer_endpoint" to "aer_common". Do you think this is appropriate?
> That makes good sense, but maybe should have a one-line hint about
> this and maybe even a compiletime_assert().
>
I intend to add the following comment to the function in next
version："/* Configure AER common registers for Root Ports, Endpoints,
and PCIe to PCI/PCI-X bridges */", Is this description appropriate?
>> +}
>> +
>> +static void program_hest_aer_root(struct acpi_hest_aer_root *aer_root, struct pci_dev *dev, int pos)
>> +{
>> + u32 root_err_cmd;
>> +
>> + root_err_cmd = aer_root->root_error_command;
>> +
>> + pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, root_err_cmd);
>> +}
>> +
>> +static void program_hest_aer_bridge(struct acpi_hest_aer_bridge *hest_aer_bridge,
>> + struct pci_dev *dev, int pos)
>> +{
>> + u32 uncor_mask2;
>> + u32 uncor_severity2;
>> + u32 adv_cap2;
>> +
>> + uncor_mask2 = hest_aer_bridge->uncorrectable_mask2;
>> + uncor_severity2 = hest_aer_bridge->uncorrectable_severity2;
>> + adv_cap2 = hest_aer_bridge->advanced_capabilities2;
>> +
>> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_MASK2, uncor_mask2);
>> + pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER2, uncor_severity2);
>> + pci_write_config_dword(dev, pos + PCI_ERR_CAP2, adv_cap2);
>> +}
>> +
>> +static void program_hest_aer_params(struct hest_parse_aer_info info)
>> +{
>> + struct pci_dev *dev;
>> + int port_type;
>> + int pos;
>> + struct acpi_hest_aer_root *hest_aer_root;
>> + struct acpi_hest_aer *hest_aer_endpoint;
>> + struct acpi_hest_aer_bridge *hest_aer_bridge;
>> +
>> + dev = info.pci_dev;
>> + port_type = pci_pcie_type(dev);
>
> I'd put these two initializations up at the declarations, e.g.,
>
> struct pci_dev *dev = info.pci_dev;
> int port_type = pci_pcie_type(dev);
>
Okay, this will be modified in the next version.
>> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>> + if (!pos)
>> + return;
>> +
>> + switch (port_type) {
>> + case PCI_EXP_TYPE_ROOT_PORT:
>> + hest_aer_root = info.hest_aer_root_port;
>> + program_hest_aer_endpoint(hest_aer_root->aer, dev, pos);
>> + program_hest_aer_root(hest_aer_root, dev, pos);
>> + break;
>> + case PCI_EXP_TYPE_ENDPOINT:
>> + hest_aer_endpoint = info.hest_aer_endpoint;
>> + program_hest_aer_endpoint(hest_aer_endpoint->aer, dev, pos);
>> + break;
>> + case PCI_EXP_TYPE_PCI_BRIDGE:
>> + case PCI_EXP_TYPE_PCIE_BRIDGE:
>
> PCI_EXP_TYPE_PCIE_BRIDGE is a PCI/PCI-X to PCIe Bridge, also known as
> a "reverse bridge". This has a conventional PCI or PCI-X primary
> interface and a PCIe secondary interface. It's not clear to me that
> these bridges can support AER.
>
> For one thing, the AER Capability must be in extended config space,
> which would only be available for PCI-X Mode 2 reverse bridges.
>
> The acpi_hest_aer_bridge certainly makes sense for
> PCI_EXP_TYPE_PCI_BRIDGE (a PCIe to PCI/PCI-X bridge), but the ACPI
> spec (r6.5, sec 18.3.2.6) is not very clear about whether it also
> applies to PCI_EXP_TYPE_PCIE_BRIDGE (a reverse bridge).
>
> Do you actually need this PCI_EXP_TYPE_PCIE_BRIDGE case?
>
Yes, you are right. I will fix this in the next version.
Yours sincerely
Leoliu-oc
>> + hest_aer_bridge = info.hest_aer_bridge;
>> + program_hest_aer_endpoint(hest_aer_bridge->aer, dev, pos);
>> + program_hest_aer_bridge(hest_aer_bridge, dev, pos);
>> + break;
>> + default:
>> + return;
>> + }
>> +}
Powered by blists - more mailing lists