| Message ID | 20231218030430.783495-4-LeoLiu-oc@zhaoxin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Parse the HEST PCIe AER and set to relevant registers | expand |
On Mon, Dec 18, 2023 at 11:04:30AM +0800, LeoLiu-oc wrote: > From: LeoLiuoc <LeoLiu-oc@zhaoxin.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@zhaoxin.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. That makes good sense, but maybe should have a one-line hint about this and maybe even a compiletime_assert(). > +} > + > +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); > + 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? > + 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; > + } > +}
在 2024/5/9 6:24, Bjorn Helgaas 写道: > > > [这封邮件来自外部发件人 谨防风险] > > On Mon, Dec 18, 2023 at 11:04:30AM +0800, LeoLiu-oc wrote: >> From: LeoLiuoc <LeoLiu-oc@zhaoxin.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@zhaoxin.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; >> + } >> +}
On Thu, 18 Jul 2024 11:46:46 -0400 Yazen Ghannam <yazen.ghannam@amd.com> wrote: > On Thu, Jul 18, 2024 at 02:24:02PM +0800, LeoLiu-oc wrote: > > From: LeoLiuoc <LeoLiu-oc@zhaoxin.com> > > > > According to the Section 18.3.2.4, 18.3.2.5 and 18.3.2.6 in ACPI SPEC > > r6.5, the register value form HEST PCI Express AER Structure should be > > written to relevant PCIe Device's AER Capabilities. So the purpose of the > > patch set is to extract register value from HEST PCI Express AER > > structures and program them into PCIe Device's AER registers. > > Refer to the ACPI SPEC r6.5 for the more detailed description. This patch > > is an effective supplement to _HPP/_HPX method when the Firmware does not > > support the _HPP/_HPX method and can be specially configured for the AER > > register of the specific device. > > > > v1->v2: > > - Move the definition of structure "hest_parse_aer_info" to file apei.h. > > > > v2->v3: > > - The applicable hardware for this patch is added to the commit > > information. > > - Change the function name "program_hest_aer_endpoint" to > > "program_hest_aer_common". > > - Add the comment to function "program_hest_aer_common". > > - Remove the "PCI_EXP_TYPE_PCIE_BRIDGE" branch handling in function > > "program_hest_aer_params". > > > > Please include a link to previous threads, if possible. Also, don't reply to previous thread. Just send a new thread but useful to include links to previous. Given it's nested in my client, v2 is https://lore.kernel.org/all/20231218030430.783495-1-LeoLiu-oc@zhaoxin.com/ > > Thanks, > Yazen >
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); +} + +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); + 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: + 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; + } +} + +int pci_acpi_program_hest_aer_params(struct pci_dev *dev) +{ + struct hest_parse_aer_info info = { + .pci_dev = dev + }; + + if (!pci_is_pcie(dev)) + return -ENODEV; + + if (apei_hest_parse(hest_parse_pcie_aer, &info) == 1) + program_hest_aer_params(info); + + return 0; +} +#endif + /** * pciehp_is_native - Check whether a hotplug port is handled by the OS * @bridge: Hotplug port to check diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 5ecbcf041179..1fc28f7a5972 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -714,6 +714,15 @@ static inline void pci_save_aer_state(struct pci_dev *dev) { } static inline void pci_restore_aer_state(struct pci_dev *dev) { } #endif +#ifdef CONFIG_ACPI_APEI +int pci_acpi_program_hest_aer_params(struct pci_dev *dev); +#else +static inline int pci_acpi_program_hest_aer_params(struct pci_dev *dev) +{ + return 0; +} +#endif + #ifdef CONFIG_ACPI int pci_acpi_program_hp_params(struct pci_dev *dev); extern const struct attribute_group pci_dev_acpi_attr_group; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ed6b7f48736a..45a45ab72846 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2274,6 +2274,7 @@ static void pci_configure_device(struct pci_dev *dev) pci_configure_serr(dev); pci_acpi_program_hp_params(dev); + pci_acpi_program_hest_aer_params(dev); } static void pci_release_capabilities(struct pci_dev *dev)