| Message ID | 20250107143852.3692571-6-terry.bowman@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable CXL PCIe port protocol error handling and logging | expand |
On 1/7/2025 10:38 PM, Terry Bowman wrote: > The AER service driver supports handling Downstream Port Protocol Errors in > Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same > functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) > mode.[1] > > CXL and PCIe Protocol Error handling have different requirements that > necessitate a separate handling path. The AER service driver may try to > recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not > suitable for CXL PCIe Port devices because of potential for system memory > corruption. Instead, CXL Protocol Error handling must use a kernel panic > in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol > Error handling does not panic the kernel in response to a UCE. > > Introduce a separate path for CXL Protocol Error handling in the AER > service driver. This will allow CXL Protocol Errors to use CXL specific > handling instead of PCIe handling. Add the CXL specific changes without > affecting or adding functionality in the PCIe handling. > > Make this update alongside the existing Downstream Port RCH error handling > logic, extending support to CXL PCIe Ports in VH mode. > > is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel > config. Update is_internal_error()'s function declaration such that it is > always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled > or disabled. > > The uncorrectable error (UCE) handling will be added in a future patch. > > [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and > Upstream Switch Ports > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- > 1 file changed, 40 insertions(+), 21 deletions(-) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index f8b3350fcbb4..62be599e3bee 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, > return true; > } > > -#ifdef CONFIG_PCIEAER_CXL > +static bool is_internal_error(struct aer_err_info *info) > +{ > + if (info->severity == AER_CORRECTABLE) > + return info->status & PCI_ERR_COR_INTERNAL; > > + return info->status & PCI_ERR_UNC_INTN; > +} > + > +#ifdef CONFIG_PCIEAER_CXL > /** > * pci_aer_unmask_internal_errors - unmask internal errors > * @dev: pointer to the pcie_dev data structure > @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) > return (pcie_ports_native || host->native_aer); > } > > -static bool is_internal_error(struct aer_err_info *info) > -{ > - if (info->severity == AER_CORRECTABLE) > - return info->status & PCI_ERR_COR_INTERNAL; > - > - return info->status & PCI_ERR_UNC_INTN; > -} > - > static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) > { > struct aer_err_info *info = (struct aer_err_info *)data; > @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) > > static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) > { > - /* > - * Internal errors of an RCEC indicate an AER error in an > - * RCH's downstream port. Check and handle them in the CXL.mem > - * device driver. > - */ > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && > - is_internal_error(info)) > - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) > + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); > + > + if (info->severity == AER_CORRECTABLE) { > + struct pci_driver *pdrv = dev->driver; > + int aer = dev->aer_cap; > + > + if (aer) > + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, > + info->status); > + > + if (pdrv && pdrv->cxl_err_handler && > + pdrv->cxl_err_handler->cor_error_detected) > + pdrv->cxl_err_handler->cor_error_detected(dev); > + > + pcie_clear_device_status(dev); > + } > } > > static int handles_cxl_error_iter(struct pci_dev *dev, void *data) > @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) > { > bool handles_cxl = false; > > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && > - pcie_aer_is_native(dev)) > + if (!pcie_aer_is_native(dev)) > + return false; > + > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) > pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); > + else > + handles_cxl = pcie_is_cxl_port(dev); My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? Ming > > return handles_cxl; > } > @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) > static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } > static inline void cxl_handle_error(struct pci_dev *dev, > struct aer_err_info *info) { } > +static bool handles_cxl_errors(struct pci_dev *dev) > +{ > + return false; > +} > #endif > > /** > @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) > > static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) > { > - cxl_handle_error(dev, info); > - pci_aer_handle_error(dev, info); > + if (is_internal_error(info) && handles_cxl_errors(dev)) > + cxl_handle_error(dev, info); > + else > + pci_aer_handle_error(dev, info); > + > pci_dev_put(dev); > } >
On Tue, 14 Jan 2025 14:54:23 +0800 Li Ming <ming.li@zohomail.com> wrote: > On 1/7/2025 10:38 PM, Terry Bowman wrote: > > The AER service driver supports handling Downstream Port Protocol Errors in > > Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same > > functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) > > mode.[1] > > > > CXL and PCIe Protocol Error handling have different requirements that > > necessitate a separate handling path. The AER service driver may try to > > recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not > > suitable for CXL PCIe Port devices because of potential for system memory > > corruption. Instead, CXL Protocol Error handling must use a kernel panic > > in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol > > Error handling does not panic the kernel in response to a UCE. > > > > Introduce a separate path for CXL Protocol Error handling in the AER > > service driver. This will allow CXL Protocol Errors to use CXL specific > > handling instead of PCIe handling. Add the CXL specific changes without > > affecting or adding functionality in the PCIe handling. > > > > Make this update alongside the existing Downstream Port RCH error handling > > logic, extending support to CXL PCIe Ports in VH mode. > > > > is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel > > config. Update is_internal_error()'s function declaration such that it is > > always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled > > or disabled. > > > > The uncorrectable error (UCE) handling will be added in a future patch. > > > > [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and > > Upstream Switch Ports > > > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > > --- > > drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- > > 1 file changed, 40 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index f8b3350fcbb4..62be599e3bee 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, > > return true; > > } > > > > -#ifdef CONFIG_PCIEAER_CXL > > +static bool is_internal_error(struct aer_err_info *info) > > +{ > > + if (info->severity == AER_CORRECTABLE) > > + return info->status & PCI_ERR_COR_INTERNAL; > > > > + return info->status & PCI_ERR_UNC_INTN; > > +} > > + > > +#ifdef CONFIG_PCIEAER_CXL > > /** > > * pci_aer_unmask_internal_errors - unmask internal errors > > * @dev: pointer to the pcie_dev data structure > > @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) > > return (pcie_ports_native || host->native_aer); > > } > > > > -static bool is_internal_error(struct aer_err_info *info) > > -{ > > - if (info->severity == AER_CORRECTABLE) > > - return info->status & PCI_ERR_COR_INTERNAL; > > - > > - return info->status & PCI_ERR_UNC_INTN; > > -} > > - > > static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) > > { > > struct aer_err_info *info = (struct aer_err_info *)data; > > @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) > > > > static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) > > { > > - /* > > - * Internal errors of an RCEC indicate an AER error in an > > - * RCH's downstream port. Check and handle them in the CXL.mem > > - * device driver. > > - */ > > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && > > - is_internal_error(info)) > > - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); > > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) > > + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); > > + > > + if (info->severity == AER_CORRECTABLE) { > > + struct pci_driver *pdrv = dev->driver; > > + int aer = dev->aer_cap; > > + > > + if (aer) > > + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, > > + info->status); > > + > > + if (pdrv && pdrv->cxl_err_handler && > > + pdrv->cxl_err_handler->cor_error_detected) > > + pdrv->cxl_err_handler->cor_error_detected(dev); > > + > > + pcie_clear_device_status(dev); > > + } > > } > > > > static int handles_cxl_error_iter(struct pci_dev *dev, void *data) > > @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) > > { > > bool handles_cxl = false; > > > > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && > > - pcie_aer_is_native(dev)) > > + if (!pcie_aer_is_native(dev)) > > + return false; > > + > > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) > > pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); > > + else > > + handles_cxl = pcie_is_cxl_port(dev); > > My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). > > pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? > > Good spot. Agreed a check on the mode makes sense. Jonathan > Ming > > > > > return handles_cxl; > > } > > @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) > > static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } > > static inline void cxl_handle_error(struct pci_dev *dev, > > struct aer_err_info *info) { } > > +static bool handles_cxl_errors(struct pci_dev *dev) > > +{ > > + return false; > > +} > > #endif > > > > /** > > @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) > > > > static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) > > { > > - cxl_handle_error(dev, info); > > - pci_aer_handle_error(dev, info); > > + if (is_internal_error(info) && handles_cxl_errors(dev)) > > + cxl_handle_error(dev, info); > > + else > > + pci_aer_handle_error(dev, info); > > + > > pci_dev_put(dev); > > } > > > > >
Terry Bowman wrote: > The AER service driver supports handling Downstream Port Protocol Errors in > Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same > functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) > mode.[1] > > CXL and PCIe Protocol Error handling have different requirements that > necessitate a separate handling path. The AER service driver may try to > recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not > suitable for CXL PCIe Port devices because of potential for system memory > corruption. Instead, CXL Protocol Error handling must use a kernel panic > in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol > Error handling does not panic the kernel in response to a UCE. > > Introduce a separate path for CXL Protocol Error handling in the AER > service driver. This will allow CXL Protocol Errors to use CXL specific > handling instead of PCIe handling. Add the CXL specific changes without > affecting or adding functionality in the PCIe handling. > > Make this update alongside the existing Downstream Port RCH error handling > logic, extending support to CXL PCIe Ports in VH mode. > > is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel > config. Update is_internal_error()'s function declaration such that it is > always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled > or disabled. > > The uncorrectable error (UCE) handling will be added in a future patch. > > [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and > Upstream Switch Ports > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> [snip]
On 1/14/2025 12:54 AM, Li Ming wrote: > On 1/7/2025 10:38 PM, Terry Bowman wrote: >> The AER service driver supports handling Downstream Port Protocol Errors in >> Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same >> functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) >> mode.[1] >> >> CXL and PCIe Protocol Error handling have different requirements that >> necessitate a separate handling path. The AER service driver may try to >> recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not >> suitable for CXL PCIe Port devices because of potential for system memory >> corruption. Instead, CXL Protocol Error handling must use a kernel panic >> in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol >> Error handling does not panic the kernel in response to a UCE. >> >> Introduce a separate path for CXL Protocol Error handling in the AER >> service driver. This will allow CXL Protocol Errors to use CXL specific >> handling instead of PCIe handling. Add the CXL specific changes without >> affecting or adding functionality in the PCIe handling. >> >> Make this update alongside the existing Downstream Port RCH error handling >> logic, extending support to CXL PCIe Ports in VH mode. >> >> is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel >> config. Update is_internal_error()'s function declaration such that it is >> always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled >> or disabled. >> >> The uncorrectable error (UCE) handling will be added in a future patch. >> >> [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and >> Upstream Switch Ports >> >> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- >> 1 file changed, 40 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >> index f8b3350fcbb4..62be599e3bee 100644 >> --- a/drivers/pci/pcie/aer.c >> +++ b/drivers/pci/pcie/aer.c >> @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, >> return true; >> } >> >> -#ifdef CONFIG_PCIEAER_CXL >> +static bool is_internal_error(struct aer_err_info *info) >> +{ >> + if (info->severity == AER_CORRECTABLE) >> + return info->status & PCI_ERR_COR_INTERNAL; >> >> + return info->status & PCI_ERR_UNC_INTN; >> +} >> + >> +#ifdef CONFIG_PCIEAER_CXL >> /** >> * pci_aer_unmask_internal_errors - unmask internal errors >> * @dev: pointer to the pcie_dev data structure >> @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) >> return (pcie_ports_native || host->native_aer); >> } >> >> -static bool is_internal_error(struct aer_err_info *info) >> -{ >> - if (info->severity == AER_CORRECTABLE) >> - return info->status & PCI_ERR_COR_INTERNAL; >> - >> - return info->status & PCI_ERR_UNC_INTN; >> -} >> - >> static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >> { >> struct aer_err_info *info = (struct aer_err_info *)data; >> @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >> >> static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) >> { >> - /* >> - * Internal errors of an RCEC indicate an AER error in an >> - * RCH's downstream port. Check and handle them in the CXL.mem >> - * device driver. >> - */ >> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >> - is_internal_error(info)) >> - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >> + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >> + >> + if (info->severity == AER_CORRECTABLE) { >> + struct pci_driver *pdrv = dev->driver; >> + int aer = dev->aer_cap; >> + >> + if (aer) >> + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, >> + info->status); >> + >> + if (pdrv && pdrv->cxl_err_handler && >> + pdrv->cxl_err_handler->cor_error_detected) >> + pdrv->cxl_err_handler->cor_error_detected(dev); >> >> + pcie_clear_device_status(dev); >> + } >> } >> >> static int handles_cxl_error_iter(struct pci_dev *dev, void *data) >> @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) >> { >> bool handles_cxl = false; >> >> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >> - pcie_aer_is_native(dev)) >> + if (!pcie_aer_is_native(dev)) >> + return false; >> + >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >> pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); >> + else >> + handles_cxl = pcie_is_cxl_port(dev); > My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). > > pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? > > > Ming Hi Ming and Jonathan, RCH AER & RCH RAS are currently logged by the CXL driver's RCH handlers. If the recommended change is made then RCH RAS will not be logged and the user would miss CXL details about the alternate protocol training failure. Also, AER is not CXL required and as a result in some cases you would only have the RCEC forwarded UIE/CIE message logged by the AER driver without any other logging. Is there value in *not* logging CXL RAS for errors on an untrained RCH link? Isn't it more informative to log PCIe AER and CXL RAS in this case? Regards, Terry >> >> return handles_cxl; >> } >> @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) >> static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } >> static inline void cxl_handle_error(struct pci_dev *dev, >> struct aer_err_info *info) { } >> +static bool handles_cxl_errors(struct pci_dev *dev) >> +{ >> + return false; >> +} >> #endif >> >> /** >> @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) >> >> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) >> { >> - cxl_handle_error(dev, info); >> - pci_aer_handle_error(dev, info); >> + if (is_internal_error(info) && handles_cxl_errors(dev)) >> + cxl_handle_error(dev, info); >> + else >> + pci_aer_handle_error(dev, info); >> + >> pci_dev_put(dev); >> } >> >
On 1/14/2025 5:20 AM, Jonathan Cameron wrote: > On Tue, 14 Jan 2025 14:54:23 +0800 > Li Ming <ming.li@zohomail.com> wrote: > >> On 1/7/2025 10:38 PM, Terry Bowman wrote: >>> The AER service driver supports handling Downstream Port Protocol Errors in >>> Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same >>> functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) >>> mode.[1] >>> >>> CXL and PCIe Protocol Error handling have different requirements that >>> necessitate a separate handling path. The AER service driver may try to >>> recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not >>> suitable for CXL PCIe Port devices because of potential for system memory >>> corruption. Instead, CXL Protocol Error handling must use a kernel panic >>> in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol >>> Error handling does not panic the kernel in response to a UCE. >>> >>> Introduce a separate path for CXL Protocol Error handling in the AER >>> service driver. This will allow CXL Protocol Errors to use CXL specific >>> handling instead of PCIe handling. Add the CXL specific changes without >>> affecting or adding functionality in the PCIe handling. >>> >>> Make this update alongside the existing Downstream Port RCH error handling >>> logic, extending support to CXL PCIe Ports in VH mode. >>> >>> is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel >>> config. Update is_internal_error()'s function declaration such that it is >>> always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled >>> or disabled. >>> >>> The uncorrectable error (UCE) handling will be added in a future patch. >>> >>> [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and >>> Upstream Switch Ports >>> >>> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >>> --- >>> drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- >>> 1 file changed, 40 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>> index f8b3350fcbb4..62be599e3bee 100644 >>> --- a/drivers/pci/pcie/aer.c >>> +++ b/drivers/pci/pcie/aer.c >>> @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, >>> return true; >>> } >>> >>> -#ifdef CONFIG_PCIEAER_CXL >>> +static bool is_internal_error(struct aer_err_info *info) >>> +{ >>> + if (info->severity == AER_CORRECTABLE) >>> + return info->status & PCI_ERR_COR_INTERNAL; >>> >>> + return info->status & PCI_ERR_UNC_INTN; >>> +} >>> + >>> +#ifdef CONFIG_PCIEAER_CXL >>> /** >>> * pci_aer_unmask_internal_errors - unmask internal errors >>> * @dev: pointer to the pcie_dev data structure >>> @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) >>> return (pcie_ports_native || host->native_aer); >>> } >>> >>> -static bool is_internal_error(struct aer_err_info *info) >>> -{ >>> - if (info->severity == AER_CORRECTABLE) >>> - return info->status & PCI_ERR_COR_INTERNAL; >>> - >>> - return info->status & PCI_ERR_UNC_INTN; >>> -} >>> - >>> static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>> { >>> struct aer_err_info *info = (struct aer_err_info *)data; >>> @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>> >>> static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>> { >>> - /* >>> - * Internal errors of an RCEC indicate an AER error in an >>> - * RCH's downstream port. Check and handle them in the CXL.mem >>> - * device driver. >>> - */ >>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>> - is_internal_error(info)) >>> - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>> + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>> + >>> + if (info->severity == AER_CORRECTABLE) { >>> + struct pci_driver *pdrv = dev->driver; >>> + int aer = dev->aer_cap; >>> + >>> + if (aer) >>> + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, >>> + info->status); >>> + >>> + if (pdrv && pdrv->cxl_err_handler && >>> + pdrv->cxl_err_handler->cor_error_detected) >>> + pdrv->cxl_err_handler->cor_error_detected(dev); >>> + >>> + pcie_clear_device_status(dev); >>> + } >>> } >>> >>> static int handles_cxl_error_iter(struct pci_dev *dev, void *data) >>> @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) >>> { >>> bool handles_cxl = false; >>> >>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>> - pcie_aer_is_native(dev)) >>> + if (!pcie_aer_is_native(dev)) >>> + return false; >>> + >>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>> pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); >>> + else >>> + handles_cxl = pcie_is_cxl_port(dev); >> My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). >> >> pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? >> >> > Good spot. > > Agreed a check on the mode makes sense. > > Jonathan Hi Jonathan, I responded to you and Ming here: https://lore.kernel.org/linux-cxl/20250107143852.3692571-1-terry.bowman@amd.com/T/#m74f758d744ae446db5d07c541dc84f0a1d57996e Regards, Terry >> Ming >> >>> >>> return handles_cxl; >>> } >>> @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) >>> static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } >>> static inline void cxl_handle_error(struct pci_dev *dev, >>> struct aer_err_info *info) { } >>> +static bool handles_cxl_errors(struct pci_dev *dev) >>> +{ >>> + return false; >>> +} >>> #endif >>> >>> /** >>> @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>> >>> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) >>> { >>> - cxl_handle_error(dev, info); >>> - pci_aer_handle_error(dev, info); >>> + if (is_internal_error(info) && handles_cxl_errors(dev)) >>> + cxl_handle_error(dev, info); >>> + else >>> + pci_aer_handle_error(dev, info); >>> + >>> pci_dev_put(dev); >>> } >>> >> >>
On 1/15/2025 3:29 AM, Bowman, Terry wrote: > > On 1/14/2025 12:54 AM, Li Ming wrote: >> On 1/7/2025 10:38 PM, Terry Bowman wrote: >>> The AER service driver supports handling Downstream Port Protocol Errors in >>> Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same >>> functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) >>> mode.[1] >>> >>> CXL and PCIe Protocol Error handling have different requirements that >>> necessitate a separate handling path. The AER service driver may try to >>> recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not >>> suitable for CXL PCIe Port devices because of potential for system memory >>> corruption. Instead, CXL Protocol Error handling must use a kernel panic >>> in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol >>> Error handling does not panic the kernel in response to a UCE. >>> >>> Introduce a separate path for CXL Protocol Error handling in the AER >>> service driver. This will allow CXL Protocol Errors to use CXL specific >>> handling instead of PCIe handling. Add the CXL specific changes without >>> affecting or adding functionality in the PCIe handling. >>> >>> Make this update alongside the existing Downstream Port RCH error handling >>> logic, extending support to CXL PCIe Ports in VH mode. >>> >>> is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel >>> config. Update is_internal_error()'s function declaration such that it is >>> always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled >>> or disabled. >>> >>> The uncorrectable error (UCE) handling will be added in a future patch. >>> >>> [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and >>> Upstream Switch Ports >>> >>> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >>> --- >>> drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- >>> 1 file changed, 40 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>> index f8b3350fcbb4..62be599e3bee 100644 >>> --- a/drivers/pci/pcie/aer.c >>> +++ b/drivers/pci/pcie/aer.c >>> @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, >>> return true; >>> } >>> >>> -#ifdef CONFIG_PCIEAER_CXL >>> +static bool is_internal_error(struct aer_err_info *info) >>> +{ >>> + if (info->severity == AER_CORRECTABLE) >>> + return info->status & PCI_ERR_COR_INTERNAL; >>> >>> + return info->status & PCI_ERR_UNC_INTN; >>> +} >>> + >>> +#ifdef CONFIG_PCIEAER_CXL >>> /** >>> * pci_aer_unmask_internal_errors - unmask internal errors >>> * @dev: pointer to the pcie_dev data structure >>> @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) >>> return (pcie_ports_native || host->native_aer); >>> } >>> >>> -static bool is_internal_error(struct aer_err_info *info) >>> -{ >>> - if (info->severity == AER_CORRECTABLE) >>> - return info->status & PCI_ERR_COR_INTERNAL; >>> - >>> - return info->status & PCI_ERR_UNC_INTN; >>> -} >>> - >>> static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>> { >>> struct aer_err_info *info = (struct aer_err_info *)data; >>> @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>> >>> static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>> { >>> - /* >>> - * Internal errors of an RCEC indicate an AER error in an >>> - * RCH's downstream port. Check and handle them in the CXL.mem >>> - * device driver. >>> - */ >>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>> - is_internal_error(info)) >>> - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>> + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>> + >>> + if (info->severity == AER_CORRECTABLE) { >>> + struct pci_driver *pdrv = dev->driver; >>> + int aer = dev->aer_cap; >>> + >>> + if (aer) >>> + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, >>> + info->status); >>> + >>> + if (pdrv && pdrv->cxl_err_handler && >>> + pdrv->cxl_err_handler->cor_error_detected) >>> + pdrv->cxl_err_handler->cor_error_detected(dev); >>> >>> + pcie_clear_device_status(dev); >>> + } >>> } >>> >>> static int handles_cxl_error_iter(struct pci_dev *dev, void *data) >>> @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) >>> { >>> bool handles_cxl = false; >>> >>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>> - pcie_aer_is_native(dev)) >>> + if (!pcie_aer_is_native(dev)) >>> + return false; >>> + >>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>> pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); >>> + else >>> + handles_cxl = pcie_is_cxl_port(dev); >> My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). >> >> pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? >> >> >> Ming > Hi Ming and Jonathan, > > RCH AER & RCH RAS are currently logged by the CXL driver's RCH handlers. > > If the recommended change is made then RCH RAS will not be logged and the > user would miss CXL details about the alternate protocol training failure. > Also, AER is not CXL required and as a result in some cases you would only > have the RCEC forwarded UIE/CIE message logged by the AER driver without > any other logging. > > Is there value in *not* logging CXL RAS for errors on an untrained RCH > link? Isn't it more informative to log PCIe AER and CXL RAS in this case? > > Regards, > Terry Hi Terry, I don't understand why the recommended change will influence RCH RAS handling, would you mind giving more details? My understanding is that above 'pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl)' is used for RCH case. And the 'else' block is used for VH case, so just check if the cxl port is working on CXL mode in pcie_is_cxl_port() or adding an extra function to check it in the 'else' block. I think it will not change RCH AER & RAS handling, is it right? or do I miss other details? Ming >>> >>> return handles_cxl; >>> } >>> @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) >>> static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } >>> static inline void cxl_handle_error(struct pci_dev *dev, >>> struct aer_err_info *info) { } >>> +static bool handles_cxl_errors(struct pci_dev *dev) >>> +{ >>> + return false; >>> +} >>> #endif >>> >>> /** >>> @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>> >>> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) >>> { >>> - cxl_handle_error(dev, info); >>> - pci_aer_handle_error(dev, info); >>> + if (is_internal_error(info) && handles_cxl_errors(dev)) >>> + cxl_handle_error(dev, info); >>> + else >>> + pci_aer_handle_error(dev, info); >>> + >>> pci_dev_put(dev); >>> } >>>
On 1/14/2025 7:18 PM, Li Ming wrote: > On 1/15/2025 3:29 AM, Bowman, Terry wrote: >> On 1/14/2025 12:54 AM, Li Ming wrote: >>> On 1/7/2025 10:38 PM, Terry Bowman wrote: >>>> The AER service driver supports handling Downstream Port Protocol Errors in >>>> Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same >>>> functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) >>>> mode.[1] >>>> >>>> CXL and PCIe Protocol Error handling have different requirements that >>>> necessitate a separate handling path. The AER service driver may try to >>>> recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not >>>> suitable for CXL PCIe Port devices because of potential for system memory >>>> corruption. Instead, CXL Protocol Error handling must use a kernel panic >>>> in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol >>>> Error handling does not panic the kernel in response to a UCE. >>>> >>>> Introduce a separate path for CXL Protocol Error handling in the AER >>>> service driver. This will allow CXL Protocol Errors to use CXL specific >>>> handling instead of PCIe handling. Add the CXL specific changes without >>>> affecting or adding functionality in the PCIe handling. >>>> >>>> Make this update alongside the existing Downstream Port RCH error handling >>>> logic, extending support to CXL PCIe Ports in VH mode. >>>> >>>> is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel >>>> config. Update is_internal_error()'s function declaration such that it is >>>> always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled >>>> or disabled. >>>> >>>> The uncorrectable error (UCE) handling will be added in a future patch. >>>> >>>> [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and >>>> Upstream Switch Ports >>>> >>>> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >>>> --- >>>> drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- >>>> 1 file changed, 40 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>>> index f8b3350fcbb4..62be599e3bee 100644 >>>> --- a/drivers/pci/pcie/aer.c >>>> +++ b/drivers/pci/pcie/aer.c >>>> @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, >>>> return true; >>>> } >>>> >>>> -#ifdef CONFIG_PCIEAER_CXL >>>> +static bool is_internal_error(struct aer_err_info *info) >>>> +{ >>>> + if (info->severity == AER_CORRECTABLE) >>>> + return info->status & PCI_ERR_COR_INTERNAL; >>>> >>>> + return info->status & PCI_ERR_UNC_INTN; >>>> +} >>>> + >>>> +#ifdef CONFIG_PCIEAER_CXL >>>> /** >>>> * pci_aer_unmask_internal_errors - unmask internal errors >>>> * @dev: pointer to the pcie_dev data structure >>>> @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) >>>> return (pcie_ports_native || host->native_aer); >>>> } >>>> >>>> -static bool is_internal_error(struct aer_err_info *info) >>>> -{ >>>> - if (info->severity == AER_CORRECTABLE) >>>> - return info->status & PCI_ERR_COR_INTERNAL; >>>> - >>>> - return info->status & PCI_ERR_UNC_INTN; >>>> -} >>>> - >>>> static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>>> { >>>> struct aer_err_info *info = (struct aer_err_info *)data; >>>> @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>>> >>>> static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>>> { >>>> - /* >>>> - * Internal errors of an RCEC indicate an AER error in an >>>> - * RCH's downstream port. Check and handle them in the CXL.mem >>>> - * device driver. >>>> - */ >>>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>>> - is_internal_error(info)) >>>> - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>>> + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>>> + >>>> + if (info->severity == AER_CORRECTABLE) { >>>> + struct pci_driver *pdrv = dev->driver; >>>> + int aer = dev->aer_cap; >>>> + >>>> + if (aer) >>>> + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, >>>> + info->status); >>>> + >>>> + if (pdrv && pdrv->cxl_err_handler && >>>> + pdrv->cxl_err_handler->cor_error_detected) >>>> + pdrv->cxl_err_handler->cor_error_detected(dev); >>>> >>>> + pcie_clear_device_status(dev); >>>> + } >>>> } >>>> >>>> static int handles_cxl_error_iter(struct pci_dev *dev, void *data) >>>> @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) >>>> { >>>> bool handles_cxl = false; >>>> >>>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>>> - pcie_aer_is_native(dev)) >>>> + if (!pcie_aer_is_native(dev)) >>>> + return false; >>>> + >>>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>>> pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); >>>> + else >>>> + handles_cxl = pcie_is_cxl_port(dev); >>> My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). >>> >>> pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? >>> >>> >>> Ming >> Hi Ming and Jonathan, >> >> RCH AER & RCH RAS are currently logged by the CXL driver's RCH handlers. >> >> If the recommended change is made then RCH RAS will not be logged and the >> user would miss CXL details about the alternate protocol training failure. >> Also, AER is not CXL required and as a result in some cases you would only >> have the RCEC forwarded UIE/CIE message logged by the AER driver without >> any other logging. >> >> Is there value in *not* logging CXL RAS for errors on an untrained RCH >> link? Isn't it more informative to log PCIe AER and CXL RAS in this case? >> >> Regards, >> Terry > Hi Terry, > > > I don't understand why the recommended change will influence RCH RAS handling, would you mind giving more details? > > My understanding is that above 'pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl)' is used for RCH case. > > And the 'else' block is used for VH case, so just check if the cxl port is working on CXL mode in pcie_is_cxl_port() or adding an extra function to check it in the 'else' block. I think it will not change RCH AER & RAS handling, is it right? or do I miss other details? > > > Ming Hi Ming, You're recommending this example case is handled by pci_aer_handle_error() rather than cxl_handle_error(). Correct me if I misunderstood. And, I believe this should continue to be handled by cxl_handle_error(). There are 2 issues with the recommended approach that deserve to be mentioned. First, the RCH Downstream Port (DP) is implemented as an RCRB and does not have a SBDF.[1] The RCH AER error is reported with the RCEC SBDF in the AER SRC_ID register.[2] The RCEC is used to find the RCH's handlers using a CXL unique procedure (see cxl_handle_error()). The logic in pci_aer_handle_error() operates on a 'struct pci_dev' type and pci_aer_handle_error() is not plumbed to support searching for the RCH handlers. Using pci_aer_handle_error would require significant changes to support a CXL RCH in addition to a PCIe device. These changes are already in cxl_handle_error(). Another issue to note is the CXL RAS information will (should) not be logged with this recommended change. pci_aer_handle_error is PCIe specific and is not aware of CXL RAS. As a result,pci_aer_handle_error() is not suited to log the CXL RAS. The example scenario was the RCH DP failed training. The user needs to know why training failed and these details are stored in the CXL RAS registers. Again, CXL RAS needs to be logged as well but CXL specific awareness shouldn't be added to pci_aer_handle_error(). Terry [1] CXL r3.1 - 8.2 Memory Mapped Registers [2] CXL r3.1 - 12.2.1.1 RCH Downstream Port-detected Errors >>>> >>>> return handles_cxl; >>>> } >>>> @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) >>>> static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } >>>> static inline void cxl_handle_error(struct pci_dev *dev, >>>> struct aer_err_info *info) { } >>>> +static bool handles_cxl_errors(struct pci_dev *dev) >>>> +{ >>>> + return false; >>>> +} >>>> #endif >>>> >>>> /** >>>> @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>>> >>>> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) >>>> { >>>> - cxl_handle_error(dev, info); >>>> - pci_aer_handle_error(dev, info); >>>> + if (is_internal_error(info) && handles_cxl_errors(dev)) >>>> + cxl_handle_error(dev, info); >>>> + else >>>> + pci_aer_handle_error(dev, info); >>>> + >>>> pci_dev_put(dev); >>>> } >>>> >
On 1/15/2025 10:39 PM, Bowman, Terry wrote: > > > On 1/14/2025 7:18 PM, Li Ming wrote: >> On 1/15/2025 3:29 AM, Bowman, Terry wrote: >>> On 1/14/2025 12:54 AM, Li Ming wrote: >>>> On 1/7/2025 10:38 PM, Terry Bowman wrote: >>>>> The AER service driver supports handling Downstream Port Protocol Errors in >>>>> Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same >>>>> functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH) >>>>> mode.[1] >>>>> >>>>> CXL and PCIe Protocol Error handling have different requirements that >>>>> necessitate a separate handling path. The AER service driver may try to >>>>> recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not >>>>> suitable for CXL PCIe Port devices because of potential for system memory >>>>> corruption. Instead, CXL Protocol Error handling must use a kernel panic >>>>> in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol >>>>> Error handling does not panic the kernel in response to a UCE. >>>>> >>>>> Introduce a separate path for CXL Protocol Error handling in the AER >>>>> service driver. This will allow CXL Protocol Errors to use CXL specific >>>>> handling instead of PCIe handling. Add the CXL specific changes without >>>>> affecting or adding functionality in the PCIe handling. >>>>> >>>>> Make this update alongside the existing Downstream Port RCH error handling >>>>> logic, extending support to CXL PCIe Ports in VH mode. >>>>> >>>>> is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel >>>>> config. Update is_internal_error()'s function declaration such that it is >>>>> always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled >>>>> or disabled. >>>>> >>>>> The uncorrectable error (UCE) handling will be added in a future patch. >>>>> >>>>> [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and >>>>> Upstream Switch Ports >>>>> >>>>> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >>>>> --- >>>>> drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++--------------- >>>>> 1 file changed, 40 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>>>> index f8b3350fcbb4..62be599e3bee 100644 >>>>> --- a/drivers/pci/pcie/aer.c >>>>> +++ b/drivers/pci/pcie/aer.c >>>>> @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, >>>>> return true; >>>>> } >>>>> >>>>> -#ifdef CONFIG_PCIEAER_CXL >>>>> +static bool is_internal_error(struct aer_err_info *info) >>>>> +{ >>>>> + if (info->severity == AER_CORRECTABLE) >>>>> + return info->status & PCI_ERR_COR_INTERNAL; >>>>> >>>>> + return info->status & PCI_ERR_UNC_INTN; >>>>> +} >>>>> + >>>>> +#ifdef CONFIG_PCIEAER_CXL >>>>> /** >>>>> * pci_aer_unmask_internal_errors - unmask internal errors >>>>> * @dev: pointer to the pcie_dev data structure >>>>> @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) >>>>> return (pcie_ports_native || host->native_aer); >>>>> } >>>>> >>>>> -static bool is_internal_error(struct aer_err_info *info) >>>>> -{ >>>>> - if (info->severity == AER_CORRECTABLE) >>>>> - return info->status & PCI_ERR_COR_INTERNAL; >>>>> - >>>>> - return info->status & PCI_ERR_UNC_INTN; >>>>> -} >>>>> - >>>>> static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>>>> { >>>>> struct aer_err_info *info = (struct aer_err_info *)data; >>>>> @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) >>>>> >>>>> static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>>>> { >>>>> - /* >>>>> - * Internal errors of an RCEC indicate an AER error in an >>>>> - * RCH's downstream port. Check and handle them in the CXL.mem >>>>> - * device driver. >>>>> - */ >>>>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>>>> - is_internal_error(info)) >>>>> - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>>>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>>>> + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); >>>>> + >>>>> + if (info->severity == AER_CORRECTABLE) { >>>>> + struct pci_driver *pdrv = dev->driver; >>>>> + int aer = dev->aer_cap; >>>>> + >>>>> + if (aer) >>>>> + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, >>>>> + info->status); >>>>> + >>>>> + if (pdrv && pdrv->cxl_err_handler && >>>>> + pdrv->cxl_err_handler->cor_error_detected) >>>>> + pdrv->cxl_err_handler->cor_error_detected(dev); >>>>> >>>>> + pcie_clear_device_status(dev); >>>>> + } >>>>> } >>>>> >>>>> static int handles_cxl_error_iter(struct pci_dev *dev, void *data) >>>>> @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) >>>>> { >>>>> bool handles_cxl = false; >>>>> >>>>> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && >>>>> - pcie_aer_is_native(dev)) >>>>> + if (!pcie_aer_is_native(dev)) >>>>> + return false; >>>>> + >>>>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) >>>>> pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); >>>>> + else >>>>> + handles_cxl = pcie_is_cxl_port(dev); >>>> My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error(). >>>> >>>> pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense? >>>> >>>> >>>> Ming >>> Hi Ming and Jonathan, >>> >>> RCH AER & RCH RAS are currently logged by the CXL driver's RCH handlers. >>> >>> If the recommended change is made then RCH RAS will not be logged and the >>> user would miss CXL details about the alternate protocol training failure. >>> Also, AER is not CXL required and as a result in some cases you would only >>> have the RCEC forwarded UIE/CIE message logged by the AER driver without >>> any other logging. >>> >>> Is there value in *not* logging CXL RAS for errors on an untrained RCH >>> link? Isn't it more informative to log PCIe AER and CXL RAS in this case? >>> >>> Regards, >>> Terry >> Hi Terry, >> >> >> I don't understand why the recommended change will influence RCH RAS handling, would you mind giving more details? >> >> My understanding is that above 'pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl)' is used for RCH case. >> >> And the 'else' block is used for VH case, so just check if the cxl port is working on CXL mode in pcie_is_cxl_port() or adding an extra function to check it in the 'else' block. I think it will not change RCH AER & RAS handling, is it right? or do I miss other details? >> >> >> Ming > Hi Ming, > > You're recommending this example case is handled by pci_aer_handle_error() rather than cxl_handle_error(). Correct me if I misunderstood. And, I believe this should continue to be handled by cxl_handle_error(). There are 2 issues with the recommended approach that deserve to be mentioned. I guess that what you thought is the recommended change using pci_aer_handle_error() to handle CXL RAS issues? If yes, it is not what I meant. handles_cxl_errors() is used to distinguish if the errors is a CXL error or a PCIe error. if the returned value of handles_cxl_errors() is 'true', that means the error is a CXL error. Then invoking either cxl_handle_error() or pcie_aer_handle_error() depending on the returned value. I think no problem in this part. handles_cxl_errors() is using pcie_is_cxl_port() to distinguish CXL errors for VH cases. the implementation of pcie_is_cxl_port() is only checking if there is a DVSEC ID 3 exposed on the CXL RP/DSP/USP. I think it is not enough. For example, If a CXL device connected to a CXL RP, there is no problem, because the return value of handles_cxl_errors() will be 'true' then cxl_handle_error() will be invoked to handle the errors. If a PCIe device connected to a CXL RP, the CXL RP is working on PCIe mode, the CXL RP is possible to expose a DVSEC ID 3[1]. If the CXL RP has a DVSEC ID 3 in the case, the return value of handles_cxl_errors() is also 'true' and also invoking cxl_handle_error() to handle the error, I thinks it is not right, the CXL RP is working on PCIe mode, the error should be a PCIe error, and it should be handled by pcie_aer_handle_error(). So my suggestion is about checking if the CXL RP/DSP/USP is working on CXL mode in pcie_is_cxl_port() for VH cases. [1] CXL r3.1 - 9.12.3 Enumerating CXL RPs and DSPs "CXL root port or DSP connected to a PCIe device/switch may or may not expose theCXL DVSEC ID 3 and the CXL DVSEC ID 7 capability structures." > > First, the RCH Downstream Port (DP) is implemented as an RCRB and does not have a > SBDF.[1] The RCH AER error is reported with the RCEC SBDF in the AER SRC_ID register.[2] The > RCEC is used to find the RCH's handlers using a CXL unique procedure (see cxl_handle_error()). > > The logic in pci_aer_handle_error() operates on a 'struct pci_dev' type and pci_aer_handle_error() is not plumbed to support searching for the RCH handlers. > > Using pci_aer_handle_error would require significant changes to support a CXL RCH > in addition to a PCIe device. These changes are already in cxl_handle_error(). > > Another issue to note is the CXL RAS information will (should) not be logged with this > recommended change. pci_aer_handle_error is PCIe specific and is not aware of CXL RAS. As a result,pci_aer_handle_error() is not suited to log the CXL RAS. > > The example scenario was the RCH DP failed training. The user needs to know why training > failed and these details are stored in the CXL RAS registers. Again, CXL RAS needs to be logged > as well but CXL specific awareness shouldn't be added to pci_aer_handle_error(). For these two issues, handles_cxl_errors() is always using "pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl)" for RCH cases. I believe no change on this part, the return value of handles_cxl_errors() will be 'true' as expected in the cases you mentioned, cxl_handle_error() will help to handle these errors. Ming > > Terry > > [1] CXL r3.1 - 8.2 Memory Mapped Registers > [2] CXL r3.1 - 12.2.1.1 RCH Downstream Port-detected Errors+ >>>>> >>>>> return handles_cxl; >>>>> } >>>>> @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) >>>>> static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } >>>>> static inline void cxl_handle_error(struct pci_dev *dev, >>>>> struct aer_err_info *info) { } >>>>> +static bool handles_cxl_errors(struct pci_dev *dev) >>>>> +{ >>>>> + return false; >>>>> +} >>>>> #endif >>>>> >>>>> /** >>>>> @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) >>>>> >>>>> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) >>>>> { >>>>> - cxl_handle_error(dev, info); >>>>> - pci_aer_handle_error(dev, info); >>>>> + if (is_internal_error(info) && handles_cxl_errors(dev)) >>>>> + cxl_handle_error(dev, info); >>>>> + else >>>>> + pci_aer_handle_error(dev, info); >>>>> + >>>>> pci_dev_put(dev); >>>>> } >>>>>
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index f8b3350fcbb4..62be599e3bee 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent, return true; } -#ifdef CONFIG_PCIEAER_CXL +static bool is_internal_error(struct aer_err_info *info) +{ + if (info->severity == AER_CORRECTABLE) + return info->status & PCI_ERR_COR_INTERNAL; + return info->status & PCI_ERR_UNC_INTN; +} + +#ifdef CONFIG_PCIEAER_CXL /** * pci_aer_unmask_internal_errors - unmask internal errors * @dev: pointer to the pcie_dev data structure @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev) return (pcie_ports_native || host->native_aer); } -static bool is_internal_error(struct aer_err_info *info) -{ - if (info->severity == AER_CORRECTABLE) - return info->status & PCI_ERR_COR_INTERNAL; - - return info->status & PCI_ERR_UNC_INTN; -} - static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) { struct aer_err_info *info = (struct aer_err_info *)data; @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) { - /* - * Internal errors of an RCEC indicate an AER error in an - * RCH's downstream port. Check and handle them in the CXL.mem - * device driver. - */ - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && - is_internal_error(info)) - pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) + return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info); + + if (info->severity == AER_CORRECTABLE) { + struct pci_driver *pdrv = dev->driver; + int aer = dev->aer_cap; + + if (aer) + pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, + info->status); + + if (pdrv && pdrv->cxl_err_handler && + pdrv->cxl_err_handler->cor_error_detected) + pdrv->cxl_err_handler->cor_error_detected(dev); + + pcie_clear_device_status(dev); + } } static int handles_cxl_error_iter(struct pci_dev *dev, void *data) @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev) { bool handles_cxl = false; - if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC && - pcie_aer_is_native(dev)) + if (!pcie_aer_is_native(dev)) + return false; + + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl); + else + handles_cxl = pcie_is_cxl_port(dev); return handles_cxl; } @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev) static inline void cxl_enable_internal_errors(struct pci_dev *dev) { } static inline void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) { } +static bool handles_cxl_errors(struct pci_dev *dev) +{ + return false; +} #endif /** @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info) static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) { - cxl_handle_error(dev, info); - pci_aer_handle_error(dev, info); + if (is_internal_error(info) && handles_cxl_errors(dev)) + cxl_handle_error(dev, info); + else + pci_aer_handle_error(dev, info); + pci_dev_put(dev); }