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

[PATCH 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.

leoliu-oc (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        | 100 ++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h             |   9 +++
 drivers/pci/probe.c           |   1 +
 include/acpi/actbl1.h         |   7 +++
 include/acpi/apei.h           |   8 +++
 include/uapi/linux/pci_regs.h |   3 +
 7 files changed, 203 insertions(+), 2 deletions(-)

-- 
2.34.1

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

[LeoLiu-oc]

From: leoliu-oc <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.

Signed-off-by: leoliu-oc <leoliu-oc@zhaoxin.com>
---
 drivers/acpi/apei/hest.c | 77 ++++++++++++++++++++++++++++++++++++++--
 include/acpi/actbl1.h    |  7 ++++
 include/acpi/apei.h      |  8 +++++
 3 files changed, 90 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 6aef1ee5e1bd..7fb797fdc1b1 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>
@@ -86,9 +87,81 @@ static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
 	return len;
 };
 
-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/actbl1.h b/include/acpi/actbl1.h
index a33375e055ad..90c27dc5325f 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -1629,6 +1629,13 @@ struct acpi_hest_generic_status {
 	u32 error_severity;
 };
 
+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;
+};
+
 /* Values for block_status flags above */
 
 #define ACPI_HEST_UNCORRECTABLE             (1)
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index dc60f7db5524..d12e6b6c4546 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -33,10 +33,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 2/3] PCI: Add AER bits #defines for PCIe to PCI/PCI-X Bridge

[LeoLiu-oc]

From: leoliu-oc <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, sec 5.2.3.2,
5.2.3.3 and 5.2.3.4.

Signed-off-by: leoliu-oc <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 a39193213ff2..987c513192e8 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -802,6 +802,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 3/3] PCI/ACPI: Add pci_acpi_program_hest_aer_params()

[LeoLiu-oc]

From: leoliu-oc <leoliu-oc@zhaoxin.com>

Call the func pci_acpi_program_hest_aer_params() for every PCIe device.
Extracting register value from HEST PCIe AER structures and programming
them into AER Capabilities are implemented in this function.
The implementation of the function 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.

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

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 004575091596..5d04068b490e 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,105 @@ 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;
+	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 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)
-- 
2.34.1

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

[Rafael J. Wysocki]

On Wed, Nov 15, 2023 at 10:16 AM LeoLiu-oc <LeoLiu-oc@zhaoxin.com> wrote:
>
> From: leoliu-oc <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.
>
> Signed-off-by: leoliu-oc <leoliu-oc@zhaoxin.com>
> ---
>  drivers/acpi/apei/hest.c | 77 ++++++++++++++++++++++++++++++++++++++--
>  include/acpi/actbl1.h    |  7 ++++
>  include/acpi/apei.h      |  8 +++++
>  3 files changed, 90 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 6aef1ee5e1bd..7fb797fdc1b1 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>
> @@ -86,9 +87,81 @@ static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
>         return len;
>  };
>
> -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/actbl1.h b/include/acpi/actbl1.h
> index a33375e055ad..90c27dc5325f 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h

This is an ACPICA header and it cannot be modified just like this.

The way to do that is to submit a pull request with the desired change
to the upstream ACPICA project on GitHub and add a Link tag pointing
to the upstream PR to the corresponding Linux patch.  Then, the Linux
patch can only be applied after the corresponding upstream PR has been
merged.

Thanks!

> @@ -1629,6 +1629,13 @@ struct acpi_hest_generic_status {
>         u32 error_severity;
>  };
>
> +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;
> +};
> +
>  /* Values for block_status flags above */
>
>  #define ACPI_HEST_UNCORRECTABLE             (1)
> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
> index dc60f7db5524..d12e6b6c4546 100644
> --- a/include/acpi/apei.h
> +++ b/include/acpi/apei.h
> @@ -33,10 +33,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
>

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

[Bjorn Helgaas]

On Wed, Nov 15, 2023 at 05:16:12PM +0800, LeoLiu-oc wrote:
> From: leoliu-oc <leoliu-oc@zhaoxin.com>
> 
> Call the func pci_acpi_program_hest_aer_params() for every PCIe device.
> Extracting register value from HEST PCIe AER structures and programming
> them into AER Capabilities are implemented in this function.
> The implementation of the function 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.

Rewrap into a single paragraph or add blank lines between.

If this is needed to make some machine work correctly, please include
that information here.

Based on Rafael's comment about getting the actbl1.h change merged via
ACPICA first, I assume this entire series is will wait for that ACPICA
update.

Bjorn

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

[LeoLiu-oc]

在 2023/12/7 7:08, Bjorn Helgaas 写道:
> On Wed, Nov 15, 2023 at 05:16:12PM +0800, LeoLiu-oc wrote:
>> From: leoliu-oc <leoliu-oc@zhaoxin.com>
>>
>> Call the func pci_acpi_program_hest_aer_params() for every PCIe device.
>> Extracting register value from HEST PCIe AER structures and programming
>> them into AER Capabilities are implemented in this function.
>> The implementation of the function 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.
> 
> Rewrap into a single paragraph or add blank lines between.
> 
> If this is needed to make some machine work correctly, please include
> that information here.
> 
> Based on Rafael's comment about getting the actbl1.h change merged via
> ACPICA first, I assume this entire series is will wait for that ACPICA
> update.
> 
> Bjorn

As mentioned in email " RE [PATCH 1/3] ACPI/APEI: Add 
hest_parse_pcie_aer()" , In the next version, I will move this data 
structure to another file.

Yours sincerely,
Leoliu-oc

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

[LeoLiu-oc]

在 2023/12/7 0:35, Rafael J. Wysocki 写道:
> On Wed, Nov 15, 2023 at 10:16 AM LeoLiu-oc <LeoLiu-oc@zhaoxin.com> wrote:
>>
>> From: leoliu-oc <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.
>>
>> Signed-off-by: leoliu-oc <leoliu-oc@zhaoxin.com>
>> ---
>>   drivers/acpi/apei/hest.c | 77 ++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/actbl1.h    |  7 ++++
>>   include/acpi/apei.h      |  8 +++++
>>   3 files changed, 90 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index 6aef1ee5e1bd..7fb797fdc1b1 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>
>> @@ -86,9 +87,81 @@ static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
>>          return len;
>>   };
>>
>> -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/actbl1.h b/include/acpi/actbl1.h
>> index a33375e055ad..90c27dc5325f 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
> 
> This is an ACPICA header and it cannot be modified just like this.
> 
> The way to do that is to submit a pull request with the desired change
> to the upstream ACPICA project on GitHub and add a Link tag pointing
> to the upstream PR to the corresponding Linux patch.  Then, the Linux
> patch can only be applied after the corresponding upstream PR has been
> merged.
> 
> Thanks!
> 
the data structure "hest_parse_aer_info" is added to file actbl1.h, it 
is not necessary to put this data structure in file actbl1.h. In the 
next version, I will move this data structure to another file.

Yours sincerely,
Leoliu-oc

>> @@ -1629,6 +1629,13 @@ struct acpi_hest_generic_status {
>>          u32 error_severity;
>>   };
>>
>> +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;
>> +};
>> +
>>   /* Values for block_status flags above */
>>
>>   #define ACPI_HEST_UNCORRECTABLE             (1)
>> diff --git a/include/acpi/apei.h b/include/acpi/apei.h
>> index dc60f7db5524..d12e6b6c4546 100644
>> --- a/include/acpi/apei.h
>> +++ b/include/acpi/apei.h
>> @@ -33,10 +33,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 v2 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. 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.

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      | 69 +++++++++++++++++++++++-
 drivers/pci/pci-acpi.c        | 98 +++++++++++++++++++++++++++++++++++
 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, 195 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 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.

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

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 6aef1ee5e1bd..7e7026c0001b 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>
@@ -86,9 +87,73 @@ static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
 	return len;
 };
 
-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;
+
+	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;
+	}
+
+	common = (struct acpi_hest_aer_common *)(hest_hdr + 1);
+	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;
+	case ACPI_HEST_TYPE_AER_ENDPOINT:
+		info->hest_aer_endpoint = (struct acpi_hest_aer *)hest_hdr;
+		return 1;
+	case ACPI_HEST_TYPE_AER_BRIDGE:
+		info->hest_aer_bridge = (struct acpi_hest_aer_bridge *)hest_hdr;
+		return 1;
+	default:
+		return 0;
+	}
+}
+#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 v2 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, 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 a39193213ff2..987c513192e8 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -802,6 +802,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 v2 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.

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);
+}
+
+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)
-- 
2.34.1

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

[Bjorn Helgaas]

On Mon, Dec 18, 2023 at 11:04:27AM +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.

Just noticed that this removes the ACPICA header dependency problem
that Rafael pointed out.  This also applies (with minor offsets) to
v6.9-rc1, so it's not very stale.  We're almost to the v6.9 final
release, so when v6.10-rc1 is tagged, can you rebase to that and
repost this?

I assume you have a platform that uses this.  It would be good to
mention that in the commit log of patches 1 and 3 so we have some idea
of where it's useful and where changes need to be tested.

> 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      | 69 +++++++++++++++++++++++-
>  drivers/pci/pci-acpi.c        | 98 +++++++++++++++++++++++++++++++++++
>  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, 195 insertions(+), 2 deletions(-)
> 
> -- 
> 2.34.1
> 

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

[Bjorn Helgaas]

On Mon, Dec 18, 2023 at 11:04:29AM +0800, LeoLiu-oc wrote:
> 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, sec 5.2.3.2,
> 5.2.3.3 and 5.2.3.4.

Please include the spec revision.  The only one I'm aware of is r1.0.

> 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 a39193213ff2..987c513192e8 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -802,6 +802,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
> 

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

[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.

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;
> +	}
> +}

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

[LeoLiu-oc]

在 2024/5/9 6:04, Bjorn Helgaas 写道:
> 
> 
> [这封邮件来自外部发件人 谨防风险]
> 
> On Mon, Dec 18, 2023 at 11:04:27AM +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.
> 
> Just noticed that this removes the ACPICA header dependency problem
> that Rafael pointed out.  This also applies (with minor offsets) to
> v6.9-rc1, so it's not very stale.  We're almost to the v6.9 final
> release, so when v6.10-rc1 is tagged, can you rebase to that and
> repost this?
> 

Thank you very much for your review. I will make a new patch version 
based on the kernel v6.10-rc1 according to your suggestion.

> I assume you have a platform that uses this.  It would be good to
> mention that in the commit log of patches 1 and 3 so we have some idea
> of where it's useful and where changes need to be tested.
> 
Okay, I will add hardware suitability information for this patch to the 
commit log.

Yours sincerely
Leoliu-oc

>> 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      | 69 +++++++++++++++++++++++-
>>   drivers/pci/pci-acpi.c        | 98 +++++++++++++++++++++++++++++++++++
>>   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, 195 insertions(+), 2 deletions(-)
>>
>> --
>> 2.34.1
>>

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

[LeoLiu-oc]

在 2024/5/9 6:10, Bjorn Helgaas 写道:
> 
> 
> [这封邮件来自外部发件人 谨防风险]
> 
> On Mon, Dec 18, 2023 at 11:04:29AM +0800, LeoLiu-oc wrote:
>> 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, sec 5.2.3.2,
>> 5.2.3.3 and 5.2.3.4.
> 
> Please include the spec revision.  The only one I'm aware of is r1.0.
> 
Yes，the PCIe to PCI/PCI-X Bridge Specification version is r1.0.
I will supplement the protocol version in the next edition.

Yours sincerely
Leoliu-oc

>> 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 a39193213ff2..987c513192e8 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -802,6 +802,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
>>

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

[LeoLiu-oc]

在 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;
>> +     }
>> +}