[PATCH v4 0/3] Parse the HEST PCIe AER and set to relevant registers

[PATCH v4 0/3] Parse the HEST PCIe AER and set to relevant registers

[LeoLiu-oc]

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.

Changes in v2:
- Move the definition of structure "hest_parse_aer_info" to file apei.h.
- Link to v1: https://lore.kernel.org/all/20231115091612.580685-1-LeoLiu-oc@zhaoxin.com/

Changes in 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".
- Link to v2: https://lore.kernel.org/all/20231218030430.783495-1-LeoLiu-oc@zhaoxin.com/

Changes in v4:
- Fix some compilation warnings.
- Link to v3: https://lore.kernel.org/all/20240718062405.30571-1-LeoLiu-oc@zhaoxin.com/

LeoLiuoc (3):
  ACPI/APEI: Add hest_parse_pcie_aer()
  PCI: Add AER bits #defines for PCIe to PCI/PCI-X Bridge
  PCI/ACPI: Add pci_acpi_program_hest_aer_params()

 drivers/acpi/apei/hest.c      |  77 ++++++++++++++++++++++++-
 drivers/pci/pci-acpi.c        | 103 ++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h             |   9 +++
 drivers/pci/probe.c           |   1 +
 include/acpi/apei.h           |  17 ++++++
 include/uapi/linux/pci_regs.h |   3 +
 6 files changed, 208 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] ACPI/APEI: Add hest_parse_pcie_aer()

[LeoLiu-oc]

From: LeoLiuoc <LeoLiu-oc@zhaoxin.com>

The purpose of the function apei_hest_parse_aer() is used to parse and
extract register value from HEST PCIe AER structures. This applies to
all hardware platforms that has a PCI Express AER structure in HEST.

Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
---
 drivers/acpi/apei/hest.c | 77 ++++++++++++++++++++++++++++++++++++++--
 include/acpi/apei.h      | 17 +++++++++
 2 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 20d757687e3d..13075f5aea25 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -22,6 +22,7 @@
 #include <linux/kdebug.h>
 #include <linux/highmem.h>
 #include <linux/io.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <acpi/apei.h>
 #include <acpi/ghes.h>
@@ -132,9 +133,81 @@ static bool is_ghes_assist_struct(struct acpi_hest_header *hest_hdr)
 	return false;
 }
 
-typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
+#ifdef CONFIG_ACPI_APEI
+static bool hest_match_pci_devfn(struct acpi_hest_aer_common *p,
+				 struct pci_dev *dev)
+{
+	return ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(dev->bus) &&
+	       ACPI_HEST_BUS(p->bus) == dev->bus->number &&
+	       p->device == PCI_SLOT(dev->devfn) &&
+	       p->function == PCI_FUNC(dev->devfn);
+}
+
+static bool hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr,
+				    struct pci_dev *dev)
+{
+	u16 hest_type = hest_hdr->type;
+	u8 pcie_type = pci_pcie_type(dev);
+	struct acpi_hest_aer_common *common;
+
+	common = (struct acpi_hest_aer_common *)(hest_hdr + 1);
+
+	switch (hest_type) {
+	case ACPI_HEST_TYPE_AER_ROOT_PORT:
+		if (pcie_type != PCI_EXP_TYPE_ROOT_PORT)
+			return false;
+	break;
+	case ACPI_HEST_TYPE_AER_ENDPOINT:
+		if (pcie_type != PCI_EXP_TYPE_ENDPOINT)
+			return false;
+	break;
+	case ACPI_HEST_TYPE_AER_BRIDGE:
+		if (pcie_type != PCI_EXP_TYPE_PCI_BRIDGE &&
+		    pcie_type != PCI_EXP_TYPE_PCIE_BRIDGE)
+			return false;
+	break;
+	default:
+		return false;
+	break;
+	}
+
+	if (common->flags & ACPI_HEST_GLOBAL)
+		return true;
+
+	if (hest_match_pci_devfn(common, dev))
+		return true;
+
+	return false;
+}
+
+int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data)
+{
+	struct hest_parse_aer_info *info = data;
+
+	if (!hest_source_is_pcie_aer(hest_hdr, info->pci_dev))
+		return 0;
+
+	switch (hest_hdr->type) {
+	case ACPI_HEST_TYPE_AER_ROOT_PORT:
+		info->hest_aer_root_port = (struct acpi_hest_aer_root *)hest_hdr;
+		return 1;
+	break;
+	case ACPI_HEST_TYPE_AER_ENDPOINT:
+		info->hest_aer_endpoint = (struct acpi_hest_aer *)hest_hdr;
+		return 1;
+	break;
+	case ACPI_HEST_TYPE_AER_BRIDGE:
+		info->hest_aer_bridge = (struct acpi_hest_aer_bridge *)hest_hdr;
+		return 1;
+	break;
+	default:
+		return 0;
+	break;
+	}
+}
+#endif
 
-static int apei_hest_parse(apei_hest_func_t func, void *data)
+int apei_hest_parse(apei_hest_func_t func, void *data)
 {
 	struct acpi_hest_header *hest_hdr;
 	int i, rc, len;
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index dc60f7db5524..82d3cdf53e22 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -23,6 +23,15 @@ enum hest_status {
 	HEST_NOT_FOUND,
 };
 
+#ifdef CONFIG_ACPI_APEI
+struct hest_parse_aer_info {
+	struct pci_dev *pci_dev;
+	struct acpi_hest_aer *hest_aer_endpoint;
+	struct acpi_hest_aer_root *hest_aer_root_port;
+	struct acpi_hest_aer_bridge *hest_aer_bridge;
+};
+#endif
+
 extern int hest_disable;
 extern int erst_disable;
 #ifdef CONFIG_ACPI_APEI_GHES
@@ -33,10 +42,18 @@ void __init acpi_ghes_init(void);
 static inline void acpi_ghes_init(void) { }
 #endif
 
+typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
+int apei_hest_parse(apei_hest_func_t func, void *data);
+
 #ifdef CONFIG_ACPI_APEI
 void __init acpi_hest_init(void);
+int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data);
 #else
 static inline void acpi_hest_init(void) { }
+static inline int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data)
+{
+	return 0;
+}
 #endif
 
 int erst_write(const struct cper_record_header *record);
-- 
2.34.1

[PATCH v4 2/3] PCI: Add AER bits #defines for PCIe to PCI/PCI-X Bridge

[LeoLiu-oc]

From: LeoLiuoc <LeoLiu-oc@zhaoxin.com>

Define secondary uncorrectable error mask register, secondary
uncorrectable error severity register and secondary error capabilities and
control register bits in AER capability for PCIe to PCI/PCI-X Bridge.
Please refer to PCIe to PCI/PCI-X Bridge Specification r1.0, sec 5.2.3.2,
5.2.3.3 and 5.2.3.4.

Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
---
 include/uapi/linux/pci_regs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 1601c7ed5fab..e0581a084fea 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -808,6 +808,9 @@
 #define  PCI_ERR_ROOT_FATAL_RCV		0x00000040 /* Fatal Received */
 #define  PCI_ERR_ROOT_AER_IRQ		0xf8000000 /* Advanced Error Interrupt Message Number */
 #define PCI_ERR_ROOT_ERR_SRC	0x34	/* Error Source Identification */
+#define PCI_ERR_UNCOR_MASK2	0x30    /* PCIe to PCI/PCI-X Bridge */
+#define PCI_ERR_UNCOR_SEVER2	0x34    /* PCIe to PCI/PCI-X Bridge */
+#define PCI_ERR_CAP2		0x38    /* PCIe to PCI/PCI-X Bridge */
 
 /* Virtual Channel */
 #define PCI_VC_PORT_CAP1	0x04
-- 
2.34.1

[PATCH v4 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()

[LeoLiu-oc]

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. This function
applies to all hardware platforms that has a PCI Express AER structure
in HEST.

Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
---
 drivers/pci/pci-acpi.c | 103 +++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h      |   9 ++++
 drivers/pci/probe.c    |   1 +
 3 files changed, 113 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index af370628e583..6e29af8e6cc4 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -19,6 +19,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
 #include <linux/rwsem.h>
+#include <acpi/apei.h>
 #include "pci.h"
 
 /*
@@ -806,6 +807,108 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
 	return -ENODEV;
 }
 
+#ifdef CONFIG_ACPI_APEI
+/*
+ * program_hest_aer_common() - configure AER common registers for Root Ports,
+ * Endpoints and PCIe to PCI/PCI-X bridges
+ */
+static void program_hest_aer_common(struct acpi_hest_aer_common aer_common,
+				    struct pci_dev *dev, int pos)
+{
+	u32 uncor_mask;
+	u32 uncor_severity;
+	u32 cor_mask;
+	u32 adv_cap;
+
+	uncor_mask = aer_common.uncorrectable_mask;
+	uncor_severity = aer_common.uncorrectable_severity;
+	cor_mask = aer_common.correctable_mask;
+	adv_cap = aer_common.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_common(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_common(hest_aer_endpoint->aer, dev, pos);
+	break;
+	case PCI_EXP_TYPE_PCI_BRIDGE:
+		hest_aer_bridge = info.hest_aer_bridge;
+		program_hest_aer_common(hest_aer_bridge->aer, dev, pos);
+		program_hest_aer_bridge(hest_aer_bridge, dev, pos);
+	break;
+	default:
+		return;
+	break;
+	}
+}
+
+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 2e40fc63ba31..78bdc121c905 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -897,6 +897,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
 bool pci_acpi_preserve_config(struct pci_host_bridge *bridge);
 int pci_acpi_program_hp_params(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2e81ab0f5a25..33b8b46ca554 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2304,6 +2304,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)
-- 
2.34.1

Re: [PATCH v4 1/3] ACPI/APEI: Add hest_parse_pcie_aer()

[Yazen Ghannam]

On Thu, Dec 05, 2024 at 07:40:46PM +0800, LeoLiu-oc wrote:
> From: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
> 
> The purpose of the function apei_hest_parse_aer() is used to parse and
> extract register value from HEST PCIe AER structures. This applies to
> all hardware platforms that has a PCI Express AER structure in HEST.
> 
> Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
> ---
>  drivers/acpi/apei/hest.c | 77 ++++++++++++++++++++++++++++++++++++++--
>  include/acpi/apei.h      | 17 +++++++++
>  2 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 20d757687e3d..13075f5aea25 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -22,6 +22,7 @@
>  #include <linux/kdebug.h>
>  #include <linux/highmem.h>
>  #include <linux/io.h>
> +#include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <acpi/apei.h>
>  #include <acpi/ghes.h>
> @@ -132,9 +133,81 @@ static bool is_ghes_assist_struct(struct acpi_hest_header *hest_hdr)
>  	return false;
>  }
>  
> -typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
> +#ifdef CONFIG_ACPI_APEI

Why is this needed? The entire hest.c file is only built if
CONFIG_ACPI_APEI is enabled.

> +static bool hest_match_pci_devfn(struct acpi_hest_aer_common *p,
> +				 struct pci_dev *dev)
> +{
> +	return ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(dev->bus) &&
> +	       ACPI_HEST_BUS(p->bus) == dev->bus->number &&
> +	       p->device == PCI_SLOT(dev->devfn) &&
> +	       p->function == PCI_FUNC(dev->devfn);

It may be nice to align all these lines on the "==".

> +}
> +
> +static bool hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr,
> +				    struct pci_dev *dev)
> +{
> +	u16 hest_type = hest_hdr->type;
> +	u8 pcie_type = pci_pcie_type(dev);
> +	struct acpi_hest_aer_common *common;
> +
> +	common = (struct acpi_hest_aer_common *)(hest_hdr + 1);
> +
> +	switch (hest_type) {
> +	case ACPI_HEST_TYPE_AER_ROOT_PORT:
> +		if (pcie_type != PCI_EXP_TYPE_ROOT_PORT)
> +			return false;
> +	break;

The breaks should be indented to the "if". Same for the rest of the
file.

> +	case ACPI_HEST_TYPE_AER_ENDPOINT:
> +		if (pcie_type != PCI_EXP_TYPE_ENDPOINT)
> +			return false;
> +	break;
> +	case ACPI_HEST_TYPE_AER_BRIDGE:
> +		if (pcie_type != PCI_EXP_TYPE_PCI_BRIDGE &&
> +		    pcie_type != PCI_EXP_TYPE_PCIE_BRIDGE)
> +			return false;
> +	break;
> +	default:
> +		return false;
> +	break;
> +	}
> +
> +	if (common->flags & ACPI_HEST_GLOBAL)
> +		return true;
> +
> +	if (hest_match_pci_devfn(common, dev))
> +		return true;
> +
> +	return false;
> +}
> +
> +int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data)
> +{
> +	struct hest_parse_aer_info *info = data;
> +
> +	if (!hest_source_is_pcie_aer(hest_hdr, info->pci_dev))
> +		return 0;
> +
> +	switch (hest_hdr->type) {
> +	case ACPI_HEST_TYPE_AER_ROOT_PORT:
> +		info->hest_aer_root_port = (struct acpi_hest_aer_root *)hest_hdr;
> +		return 1;
> +	break;
> +	case ACPI_HEST_TYPE_AER_ENDPOINT:
> +		info->hest_aer_endpoint = (struct acpi_hest_aer *)hest_hdr;
> +		return 1;
> +	break;
> +	case ACPI_HEST_TYPE_AER_BRIDGE:
> +		info->hest_aer_bridge = (struct acpi_hest_aer_bridge *)hest_hdr;
> +		return 1;
> +	break;
> +	default:
> +		return 0;
> +	break;
> +	}
> +}
> +#endif
>  
> -static int apei_hest_parse(apei_hest_func_t func, void *data)
> +int apei_hest_parse(apei_hest_func_t func, void *data)
>  {
>  	struct acpi_hest_header *hest_hdr;
>  	int i, rc, len;
> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
> index dc60f7db5524..82d3cdf53e22 100644
> --- a/include/acpi/apei.h
> +++ b/include/acpi/apei.h
> @@ -23,6 +23,15 @@ enum hest_status {
>  	HEST_NOT_FOUND,
>  };
>  
> +#ifdef CONFIG_ACPI_APEI
> +struct hest_parse_aer_info {
> +	struct pci_dev *pci_dev;
> +	struct acpi_hest_aer *hest_aer_endpoint;
> +	struct acpi_hest_aer_root *hest_aer_root_port;
> +	struct acpi_hest_aer_bridge *hest_aer_bridge;

These three pointers are mutually exclusive. Can you save just one
pointer and then cast it when checking the "port_type" in patch 3?

> +};
> +#endif

I think the #ifdef is not needed, because this is not declaring an
instance of the struct.

> +
>  extern int hest_disable;
>  extern int erst_disable;
>  #ifdef CONFIG_ACPI_APEI_GHES
> @@ -33,10 +42,18 @@ void __init acpi_ghes_init(void);
>  static inline void acpi_ghes_init(void) { }
>  #endif
>  
> +typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
> +int apei_hest_parse(apei_hest_func_t func, void *data);
> +

Minor nit: this could be done as a separate patch.

Patch 1: Move apei_hest_parse() to apei.h
Patch 2: Add new hest_parse_pcie_aer()

>  #ifdef CONFIG_ACPI_APEI
>  void __init acpi_hest_init(void);
> +int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data);
>  #else
>  static inline void acpi_hest_init(void) { }
> +static inline int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data)
> +{
> +	return 0;
> +}
>  #endif
>  
>  int erst_write(const struct cper_record_header *record);
> -- 

Thanks,
Yazen

Re: [PATCH v4 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()

[Yazen Ghannam]

On Thu, Dec 05, 2024 at 07:40:48PM +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. This function
> applies to all hardware platforms that has a PCI Express AER structure
> in HEST.
> 
> Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
> ---
>  drivers/pci/pci-acpi.c | 103 +++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h      |   9 ++++
>  drivers/pci/probe.c    |   1 +
>  3 files changed, 113 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index af370628e583..6e29af8e6cc4 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -19,6 +19,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_qos.h>
>  #include <linux/rwsem.h>
> +#include <acpi/apei.h>
>  #include "pci.h"
>  
>  /*
> @@ -806,6 +807,108 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
>  	return -ENODEV;
>  }
>  
> +#ifdef CONFIG_ACPI_APEI
> +/*
> + * program_hest_aer_common() - configure AER common registers for Root Ports,
> + * Endpoints and PCIe to PCI/PCI-X bridges
> + */
> +static void program_hest_aer_common(struct acpi_hest_aer_common aer_common,
> +				    struct pci_dev *dev, int pos)
> +{
> +	u32 uncor_mask;
> +	u32 uncor_severity;
> +	u32 cor_mask;
> +	u32 adv_cap;
> +
> +	uncor_mask = aer_common.uncorrectable_mask;
> +	uncor_severity = aer_common.uncorrectable_severity;
> +	cor_mask = aer_common.correctable_mask;
> +	adv_cap = aer_common.advanced_capabilities;
> +

These can be done at the same time as the declarations. Same for the
remaining functions.

> +	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_common(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_common(hest_aer_endpoint->aer, dev, pos);
> +	break;
> +	case PCI_EXP_TYPE_PCI_BRIDGE:
> +		hest_aer_bridge = info.hest_aer_bridge;
> +		program_hest_aer_common(hest_aer_bridge->aer, dev, pos);
> +		program_hest_aer_bridge(hest_aer_bridge, dev, pos);
> +	break;
> +	default:
> +		return;
> +	break;
> +	}
> +}
> +
> +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)

Don't need the "== 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 2e40fc63ba31..78bdc121c905 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -897,6 +897,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);

The return value is never checked, so this can return void.

> +#else
> +static inline int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #ifdef CONFIG_ACPI
>  bool pci_acpi_preserve_config(struct pci_host_bridge *bridge);
>  int pci_acpi_program_hp_params(struct pci_dev *dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2e81ab0f5a25..33b8b46ca554 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2304,6 +2304,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);

This should not be called here unconditionally.

OS should only write AER registers if granted permission through
_OSC.

It would be more appropriate to call this from pci_aer_init().

Thanks,
Yazen

Re: [PATCH v4 1/3] ACPI/APEI: Add hest_parse_pcie_aer()

[LeoLiu-oc]

在 2024/12/12 3:20, Yazen Ghannam 写道:
> 
> 
> [这封邮件来自外部发件人 谨防风险]
> 
> On Thu, Dec 05, 2024 at 07:40:46PM +0800, LeoLiu-oc wrote:
>> From: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
>>
>> The purpose of the function apei_hest_parse_aer() is used to parse and
>> extract register value from HEST PCIe AER structures. This applies to
>> all hardware platforms that has a PCI Express AER structure in HEST.
>>
>> Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
>> ---
>>   drivers/acpi/apei/hest.c | 77 ++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/apei.h      | 17 +++++++++
>>   2 files changed, 92 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index 20d757687e3d..13075f5aea25 100644
>> --- a/drivers/acpi/apei/hest.c
>> +++ b/drivers/acpi/apei/hest.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/kdebug.h>
>>   #include <linux/highmem.h>
>>   #include <linux/io.h>
>> +#include <linux/pci.h>
>>   #include <linux/platform_device.h>
>>   #include <acpi/apei.h>
>>   #include <acpi/ghes.h>
>> @@ -132,9 +133,81 @@ static bool is_ghes_assist_struct(struct acpi_hest_header *hest_hdr)
>>        return false;
>>   }
>>
>> -typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
>> +#ifdef CONFIG_ACPI_APEI
> 
> Why is this needed? The entire hest.c file is only built if
> CONFIG_ACPI_APEI is enabled.
> 
I agree with your point.

>> +static bool hest_match_pci_devfn(struct acpi_hest_aer_common *p,
>> +                              struct pci_dev *dev)
>> +{
>> +     return ACPI_HEST_SEGMENT(p->bus) == pci_domain_nr(dev->bus) &&
>> +            ACPI_HEST_BUS(p->bus) == dev->bus->number &&
>> +            p->device == PCI_SLOT(dev->devfn) &&
>> +            p->function == PCI_FUNC(dev->devfn);
> 
> It may be nice to align all these lines on the "==".
> 
Okay, I will make the changes in the next version

>> +}
>> +
>> +static bool hest_source_is_pcie_aer(struct acpi_hest_header *hest_hdr,
>> +                                 struct pci_dev *dev)
>> +{
>> +     u16 hest_type = hest_hdr->type;
>> +     u8 pcie_type = pci_pcie_type(dev);
>> +     struct acpi_hest_aer_common *common;
>> +
>> +     common = (struct acpi_hest_aer_common *)(hest_hdr + 1);
>> +
>> +     switch (hest_type) {
>> +     case ACPI_HEST_TYPE_AER_ROOT_PORT:
>> +             if (pcie_type != PCI_EXP_TYPE_ROOT_PORT)
>> +                     return false;
>> +     break;
> 
> The breaks should be indented to the "if". Same for the rest of the
> file.
> 
I agree with your point of view and will make modifications in the next 
version.

>> +     case ACPI_HEST_TYPE_AER_ENDPOINT:
>> +             if (pcie_type != PCI_EXP_TYPE_ENDPOINT)
>> +                     return false;
>> +     break;
>> +     case ACPI_HEST_TYPE_AER_BRIDGE:
>> +             if (pcie_type != PCI_EXP_TYPE_PCI_BRIDGE &&
>> +                 pcie_type != PCI_EXP_TYPE_PCIE_BRIDGE)
>> +                     return false;
>> +     break;
>> +     default:
>> +             return false;
>> +     break;
>> +     }
>> +
>> +     if (common->flags & ACPI_HEST_GLOBAL)
>> +             return true;
>> +
>> +     if (hest_match_pci_devfn(common, dev))
>> +             return true;
>> +
>> +     return false;
>> +}
>> +
>> +int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data)
>> +{
>> +     struct hest_parse_aer_info *info = data;
>> +
>> +     if (!hest_source_is_pcie_aer(hest_hdr, info->pci_dev))
>> +             return 0;
>> +
>> +     switch (hest_hdr->type) {
>> +     case ACPI_HEST_TYPE_AER_ROOT_PORT:
>> +             info->hest_aer_root_port = (struct acpi_hest_aer_root *)hest_hdr;
>> +             return 1;
>> +     break;
>> +     case ACPI_HEST_TYPE_AER_ENDPOINT:
>> +             info->hest_aer_endpoint = (struct acpi_hest_aer *)hest_hdr;
>> +             return 1;
>> +     break;
>> +     case ACPI_HEST_TYPE_AER_BRIDGE:
>> +             info->hest_aer_bridge = (struct acpi_hest_aer_bridge *)hest_hdr;
>> +             return 1;
>> +     break;
>> +     default:
>> +             return 0;
>> +     break;
>> +     }
>> +}
>> +#endif
>>
>> -static int apei_hest_parse(apei_hest_func_t func, void *data)
>> +int apei_hest_parse(apei_hest_func_t func, void *data)
>>   {
>>        struct acpi_hest_header *hest_hdr;
>>        int i, rc, len;
>> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
>> index dc60f7db5524..82d3cdf53e22 100644
>> --- a/include/acpi/apei.h
>> +++ b/include/acpi/apei.h
>> @@ -23,6 +23,15 @@ enum hest_status {
>>        HEST_NOT_FOUND,
>>   };
>>
>> +#ifdef CONFIG_ACPI_APEI
>> +struct hest_parse_aer_info {
>> +     struct pci_dev *pci_dev;
>> +     struct acpi_hest_aer *hest_aer_endpoint;
>> +     struct acpi_hest_aer_root *hest_aer_root_port;
>> +     struct acpi_hest_aer_bridge *hest_aer_bridge;
> 
> These three pointers are mutually exclusive. Can you save just one
> pointer and then cast it when checking the "port_type" in patch 3?
> 
>> +};
>> +#endif
> 
> I think the #ifdef is not needed, because this is not declaring an
> instance of the struct.
> 
I agree with your point of view and will make modifications in the next 
version.

>> +
>>   extern int hest_disable;
>>   extern int erst_disable;
>>   #ifdef CONFIG_ACPI_APEI_GHES
>> @@ -33,10 +42,18 @@ void __init acpi_ghes_init(void);
>>   static inline void acpi_ghes_init(void) { }
>>   #endif
>>
>> +typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data);
>> +int apei_hest_parse(apei_hest_func_t func, void *data);
>> +
> 
> Minor nit: this could be done as a separate patch.
> 
> Patch 1: Move apei_hest_parse() to apei.h
> Patch 2: Add new hest_parse_pcie_aer()
> 
Ok, your suggestion is very reasonable, I will modify it in the next 
version.

Leoliu-oc
Best Regards

>>   #ifdef CONFIG_ACPI_APEI
>>   void __init acpi_hest_init(void);
>> +int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data);
>>   #else
>>   static inline void acpi_hest_init(void) { }
>> +static inline int hest_parse_pcie_aer(struct acpi_hest_header *hest_hdr, void *data)
>> +{
>> +     return 0;
>> +}
>>   #endif
>>
>>   int erst_write(const struct cper_record_header *record);
>> --
> 
> Thanks,
> Yazen

Re: [PATCH v4 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()

[LeoLiu-oc]

在 2024/12/12 3:47, Yazen Ghannam 写道:
> 
> 
> [这封邮件来自外部发件人 谨防风险]
> 
> On Thu, Dec 05, 2024 at 07:40:48PM +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. This function
>> applies to all hardware platforms that has a PCI Express AER structure
>> in HEST.
>>
>> Signed-off-by: LeoLiuoc <LeoLiu-oc@zhaoxin.com>
>> ---
>>   drivers/pci/pci-acpi.c | 103 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci.h      |   9 ++++
>>   drivers/pci/probe.c    |   1 +
>>   3 files changed, 113 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index af370628e583..6e29af8e6cc4 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/pm_runtime.h>
>>   #include <linux/pm_qos.h>
>>   #include <linux/rwsem.h>
>> +#include <acpi/apei.h>
>>   #include "pci.h"
>>
>>   /*
>> @@ -806,6 +807,108 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
>>        return -ENODEV;
>>   }
>>
>> +#ifdef CONFIG_ACPI_APEI
>> +/*
>> + * program_hest_aer_common() - configure AER common registers for Root Ports,
>> + * Endpoints and PCIe to PCI/PCI-X bridges
>> + */
>> +static void program_hest_aer_common(struct acpi_hest_aer_common aer_common,
>> +                                 struct pci_dev *dev, int pos)
>> +{
>> +     u32 uncor_mask;
>> +     u32 uncor_severity;
>> +     u32 cor_mask;
>> +     u32 adv_cap;
>> +
>> +     uncor_mask = aer_common.uncorrectable_mask;
>> +     uncor_severity = aer_common.uncorrectable_severity;
>> +     cor_mask = aer_common.correctable_mask;
>> +     adv_cap = aer_common.advanced_capabilities;
>> +
> 
> These can be done at the same time as the declarations. Same for the
> remaining functions.
> 
These functions are called only in this pci-acpi.c file and should not 
require additional declarations.

>> +     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_common(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_common(hest_aer_endpoint->aer, dev, pos);
>> +     break;
>> +     case PCI_EXP_TYPE_PCI_BRIDGE:
>> +             hest_aer_bridge = info.hest_aer_bridge;
>> +             program_hest_aer_common(hest_aer_bridge->aer, dev, pos);
>> +             program_hest_aer_bridge(hest_aer_bridge, dev, pos);
>> +     break;
>> +     default:
>> +             return;
>> +     break;
>> +     }
>> +}
>> +
>> +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)
> 
> Don't need the "== 1".
> 
The apei_hest_parse function may return an error value, data are written 
to the aer registers only if the device in hest_parse_pcie_aer() matches 
the correct hest aer structure information. It is necessary to check 
whether the return value of the apei_hest_parse function is equal to 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 2e40fc63ba31..78bdc121c905 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -897,6 +897,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);
> 
> The return value is never checked, so this can return void.
> 
Ok, I agree with you, I will modify it in the next version.

>> +#else
>> +static inline int pci_acpi_program_hest_aer_params(struct pci_dev *dev)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>   #ifdef CONFIG_ACPI
>>   bool pci_acpi_preserve_config(struct pci_host_bridge *bridge);
>>   int pci_acpi_program_hp_params(struct pci_dev *dev);
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 2e81ab0f5a25..33b8b46ca554 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2304,6 +2304,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);
> 
> This should not be called here unconditionally.
> 
> OS should only write AER registers if granted permission through
> _OSC.
> 
> It would be more appropriate to call this from pci_aer_init().
> 
> Thanks,
> Yazen
The question about whether OS has control of AER to write the 
information in the HEST AER structure to the AER register of the 
corresponding device has been discussed, see the link for more details: 
https://lore.kernel.org/all/a3a603ef-b813-4798-bb54-62076d53bd3a@zhaoxin.com/

It is inappropriate to add the pci_acpi_program_hest_aer_params function 
to the pci_aer_init function，The reason for this is that no involvement 
of the AER driver.

Leoliu-oc
Best Regards