Restricted CXL host (RCH) downstream port AER information is not currently
logged while in the error state. One problem preventing existing PCIe AER
functions from logging errors is the AER registers are not accessible. The
CXL driver requires changes to find RCH downstream port AER registers for
purpose of error logging.
RCH downstream ports are not enumerated during a PCI bus scan and are
instead discovered using system firmware, ACPI in this case.[1] The
downstream port is implemented as a Root Complex Register Block (RCRB).
The RCRB is a 4k memory block containing PCIe registers based on the PCIe
root port.[2] The RCRB includes AER extended capability registers used for
reporting errors. Note, the RCH's AER Capability is located in the RCRB
memory space instead of PCI configuration space, thus its register access
is different. Existing kernel PCIe AER functions can not be used to manage
the downstream port AER capabilities because the port was not enumerated
during PCI scan and the registers are not PCI config accessible.
Discover RCH downstream port AER extended capability registers. This
requires using MMIO accesses to search for extended AER capability in
RCRB register space.
[1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
[2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
Co-developed-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
drivers/cxl/core/regs.c | 93 +++++++++++++++++++++++++++++++++++------
drivers/cxl/cxl.h | 5 +++
drivers/cxl/mem.c | 39 +++++++++++------
3 files changed, 113 insertions(+), 24 deletions(-)
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 1476a0299c9b..bde1fffab09e 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -332,10 +332,36 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
}
EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
+static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
+ char *name)
+{
+
+ if (!request_mem_region(map->resource, map->max_size, name))
+ return NULL;
+
+ map->base = ioremap(map->resource, map->max_size);
+ if (!map->base) {
+ release_mem_region(map->resource, map->max_size);
+ return NULL;
+ }
+
+ return map->base;
+}
+
+static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
+{
+ iounmap(map->base);
+ release_mem_region(map->resource, map->max_size);
+}
+
resource_size_t cxl_rcrb_to_component(struct device *dev,
resource_size_t rcrb,
enum cxl_rcrb which)
{
+ struct cxl_register_map map = {
+ .resource = rcrb,
+ .max_size = SZ_4K
+ };
resource_size_t component_reg_phys;
void __iomem *addr;
u32 bar0, bar1;
@@ -343,7 +369,10 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
u32 id;
if (which == CXL_RCRB_UPSTREAM)
- rcrb += SZ_4K;
+ map.resource += SZ_4K;
+
+ if (!cxl_map_reg(dev, &map, "CXL RCRB"))
+ return CXL_RESOURCE_NONE;
/*
* RCRB's BAR[0..1] point to component block containing CXL
@@ -351,21 +380,12 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
* the PCI Base spec here, esp. 64 bit extraction and memory
* ranges alignment (6.0, 7.5.1.2.1).
*/
- if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
- return CXL_RESOURCE_NONE;
- addr = ioremap(rcrb, SZ_4K);
- if (!addr) {
- dev_err(dev, "Failed to map region %pr\n", addr);
- release_mem_region(rcrb, SZ_4K);
- return CXL_RESOURCE_NONE;
- }
-
+ addr = map.base;
id = readl(addr + PCI_VENDOR_ID);
cmd = readw(addr + PCI_COMMAND);
bar0 = readl(addr + PCI_BASE_ADDRESS_0);
bar1 = readl(addr + PCI_BASE_ADDRESS_1);
- iounmap(addr);
- release_mem_region(rcrb, SZ_4K);
+ cxl_unmap_reg(dev, &map);
/*
* Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
@@ -396,3 +416,52 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
return component_reg_phys;
}
EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
+
+u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb)
+{
+ struct cxl_register_map map = {
+ .resource = rcrb,
+ .max_size = SZ_4K,
+ };
+ u32 cap_hdr;
+ u16 offset = 0;
+
+ if (!cxl_map_reg(dev, &map, "CXL RCRB"))
+ return 0;
+
+ cap_hdr = readl(map.base + offset);
+ while (PCI_EXT_CAP_ID(cap_hdr) != PCI_EXT_CAP_ID_ERR) {
+
+ offset = PCI_EXT_CAP_NEXT(cap_hdr);
+ if (!offset) {
+ cxl_unmap_reg(dev, &map);
+ return 0;
+ }
+ cap_hdr = readl(map.base + offset);
+ }
+
+ dev_dbg(dev, "found AER extended capability (0x%x)\n", offset);
+ cxl_unmap_reg(dev, &map);
+
+ return offset;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_aer, CXL);
+
+u16 cxl_component_to_ras(struct device *dev, resource_size_t component_reg_phys)
+{
+ struct cxl_register_map map = {
+ .resource = component_reg_phys,
+ .max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
+ };
+
+ if (!cxl_map_reg(dev, &map, "component"))
+ return 0;
+
+ cxl_probe_component_regs(dev, map.base, &map.component_map);
+ cxl_unmap_reg(dev, &map);
+ if (!map.component_map.ras.valid)
+ return 0;
+
+ return map.component_map.ras.offset;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_component_to_ras, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 044a92d9813e..df64c402e6e6 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -270,6 +270,9 @@ enum cxl_rcrb {
resource_size_t cxl_rcrb_to_component(struct device *dev,
resource_size_t rcrb,
enum cxl_rcrb which);
+u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
+u16 cxl_component_to_ras(struct device *dev,
+ resource_size_t component_reg_phys);
#define CXL_RESOURCE_NONE ((resource_size_t) -1)
#define CXL_TARGET_STRLEN 20
@@ -601,6 +604,8 @@ struct cxl_dport {
int port_id;
resource_size_t component_reg_phys;
resource_size_t rcrb;
+ u16 aer_cap;
+ u16 ras_cap;
bool rch;
struct cxl_port *port;
};
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 39c4b54f0715..014295ab6bc6 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -45,13 +45,36 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
return 0;
}
+static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
+ struct cxl_dport *parent_dport)
+{
+ struct cxl_memdev *cxlmd = cxlds->cxlmd;
+
+ if (!parent_dport->rch)
+ return;
+
+ /*
+ * The component registers for an RCD might come from the
+ * host-bridge RCRB if they are not already mapped via the
+ * typical register locator mechanism.
+ */
+ if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
+ cxlds->component_reg_phys = cxl_rcrb_to_component(
+ &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
+
+ parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
+ parent_dport->rcrb);
+
+ parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
+ parent_dport->component_reg_phys);
+}
+
static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
struct cxl_dport *parent_dport)
{
struct cxl_port *parent_port = parent_dport->port;
struct cxl_dev_state *cxlds = cxlmd->cxlds;
struct cxl_port *endpoint, *iter, *down;
- resource_size_t component_reg_phys;
int rc;
/*
@@ -66,17 +89,9 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
ep->next = down;
}
- /*
- * The component registers for an RCD might come from the
- * host-bridge RCRB if they are not already mapped via the
- * typical register locator mechanism.
- */
- if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
- component_reg_phys = cxl_rcrb_to_component(
- &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
- else
- component_reg_phys = cxlds->component_reg_phys;
- endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
+ cxl_setup_rcrb(cxlds, parent_dport);
+
+ endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys,
parent_dport);
if (IS_ERR(endpoint))
return PTR_ERR(endpoint);
--
2.34.1On Tue, 11 Apr 2023 13:02:57 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> Restricted CXL host (RCH) downstream port AER information is not currently
> logged while in the error state. One problem preventing existing PCIe AER
> functions from logging errors is the AER registers are not accessible. The
> CXL driver requires changes to find RCH downstream port AER registers for
> purpose of error logging.
>
> RCH downstream ports are not enumerated during a PCI bus scan and are
> instead discovered using system firmware, ACPI in this case.[1] The
> downstream port is implemented as a Root Complex Register Block (RCRB).
> The RCRB is a 4k memory block containing PCIe registers based on the PCIe
> root port.[2] The RCRB includes AER extended capability registers used for
> reporting errors. Note, the RCH's AER Capability is located in the RCRB
> memory space instead of PCI configuration space, thus its register access
> is different. Existing kernel PCIe AER functions can not be used to manage
> the downstream port AER capabilities because the port was not enumerated
> during PCI scan and the registers are not PCI config accessible.
>
> Discover RCH downstream port AER extended capability registers. This
> requires using MMIO accesses to search for extended AER capability in
> RCRB register space.
>
> [1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
> [2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
>
> Co-developed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Hi Terry,
Sorry I missed first few versions. Playing catch up.
A few minor comments only inline.
> ---
> drivers/cxl/core/regs.c | 93 +++++++++++++++++++++++++++++++++++------
> drivers/cxl/cxl.h | 5 +++
> drivers/cxl/mem.c | 39 +++++++++++------
> 3 files changed, 113 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 1476a0299c9b..bde1fffab09e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -332,10 +332,36 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>
> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
> + char *name)
dev isn't used.
> +{
> +
Trivial but no point in blank line here.
> + if (!request_mem_region(map->resource, map->max_size, name))
> + return NULL;
> +
> + map->base = ioremap(map->resource, map->max_size);
> + if (!map->base) {
> + release_mem_region(map->resource, map->max_size);
> + return NULL;
> + }
> +
> + return map->base;
Why return a value you've already stashed in map->base?
> +}
> +
This is similar enough to devm_cxl_iomap_block() that I'd kind
of like them them take the same parameters. That would mean
moving the map structure outside of the calls and instead passing
in the 3 relevant parameters. Perhaps not worth it.
> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
> +{
dev isn't used here either. Makes little sense to pass it in to either funtion.
> + iounmap(map->base);
> + release_mem_region(map->resource, map->max_size);
> +}
> +
> resource_size_t cxl_rcrb_to_component(struct device *dev,
> resource_size_t rcrb,
> enum cxl_rcrb which)
> {
> + struct cxl_register_map map = {
> + .resource = rcrb,
> + .max_size = SZ_4K
> + };
> resource_size_t component_reg_phys;
> void __iomem *addr;
> u32 bar0, bar1;
> @@ -343,7 +369,10 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> u32 id;
>
> if (which == CXL_RCRB_UPSTREAM)
> - rcrb += SZ_4K;
> + map.resource += SZ_4K;
> +
> + if (!cxl_map_reg(dev, &map, "CXL RCRB"))
> + return CXL_RESOURCE_NONE;
>
> /*
> * RCRB's BAR[0..1] point to component block containing CXL
> @@ -351,21 +380,12 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> * the PCI Base spec here, esp. 64 bit extraction and memory
> * ranges alignment (6.0, 7.5.1.2.1).
> */
> - if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> - return CXL_RESOURCE_NONE;
> - addr = ioremap(rcrb, SZ_4K);
> - if (!addr) {
> - dev_err(dev, "Failed to map region %pr\n", addr);
> - release_mem_region(rcrb, SZ_4K);
> - return CXL_RESOURCE_NONE;
> - }
> -
> + addr = map.base;
I'd have preferred to see this refactor as a precursor patch to the
'real changes' that follow.
> id = readl(addr + PCI_VENDOR_ID);
> cmd = readw(addr + PCI_COMMAND);
> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> - iounmap(addr);
> - release_mem_region(rcrb, SZ_4K);
> + cxl_unmap_reg(dev, &map);
>
> /*
> * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> @@ -396,3 +416,52 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> return component_reg_phys;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
...
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 044a92d9813e..df64c402e6e6 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -270,6 +270,9 @@ enum cxl_rcrb {
> resource_size_t cxl_rcrb_to_component(struct device *dev,
> resource_size_t rcrb,
> enum cxl_rcrb which);
> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
> +u16 cxl_component_to_ras(struct device *dev,
> + resource_size_t component_reg_phys);
>
> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> #define CXL_TARGET_STRLEN 20
> @@ -601,6 +604,8 @@ struct cxl_dport {
> int port_id;
> resource_size_t component_reg_phys;
> resource_size_t rcrb;
> + u16 aer_cap;
> + u16 ras_cap;
This structure has kernel-doc that needs to be updated for these new entries.
> bool rch;
> struct cxl_port *port;
> };
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 39c4b54f0715..014295ab6bc6 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -45,13 +45,36 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> return 0;
> }
>
> +static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
> + struct cxl_dport *parent_dport)
> +{
> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
extra space before =
> +
> + if (!parent_dport->rch)
> + return;
> +
> + /*
> + * The component registers for an RCD might come from the
> + * host-bridge RCRB if they are not already mapped via the
> + * typical register locator mechanism.
> + */
> + if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> + cxlds->component_reg_phys = cxl_rcrb_to_component(
> + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> +
> + parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
> + parent_dport->rcrb);
> +
> + parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
> + parent_dport->component_reg_phys);
> +}
> +
> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> struct cxl_dport *parent_dport)
> {
> struct cxl_port *parent_port = parent_dport->port;
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct cxl_port *endpoint, *iter, *down;
> - resource_size_t component_reg_phys;
> int rc;
>
> /*
> @@ -66,17 +89,9 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> ep->next = down;
> }
>
> - /*
> - * The component registers for an RCD might come from the
> - * host-bridge RCRB if they are not already mapped via the
> - * typical register locator mechanism.
> - */
> - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> - component_reg_phys = cxl_rcrb_to_component(
> - &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> - else
> - component_reg_phys = cxlds->component_reg_phys;
> - endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
> + cxl_setup_rcrb(cxlds, parent_dport);
> +
> + endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys,
> parent_dport);
As above, I'd prefer to see this refactor done in a precursor patch before the new
stuff is added. I like reviewing noop patches as I don't have to think much (so
can do it when I'm supposedly in a meeting ;)
Jonathan
> if (IS_ERR(endpoint))
> return PTR_ERR(endpoint);
Hi Jonathan,
Thanks for the review. I added comments below.
On 4/13/23 10:30, Jonathan Cameron wrote:
> On Tue, 11 Apr 2023 13:02:57 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
>
>> Restricted CXL host (RCH) downstream port AER information is not currently
>> logged while in the error state. One problem preventing existing PCIe AER
>> functions from logging errors is the AER registers are not accessible. The
>> CXL driver requires changes to find RCH downstream port AER registers for
>> purpose of error logging.
>>
>> RCH downstream ports are not enumerated during a PCI bus scan and are
>> instead discovered using system firmware, ACPI in this case.[1] The
>> downstream port is implemented as a Root Complex Register Block (RCRB).
>> The RCRB is a 4k memory block containing PCIe registers based on the PCIe
>> root port.[2] The RCRB includes AER extended capability registers used for
>> reporting errors. Note, the RCH's AER Capability is located in the RCRB
>> memory space instead of PCI configuration space, thus its register access
>> is different. Existing kernel PCIe AER functions can not be used to manage
>> the downstream port AER capabilities because the port was not enumerated
>> during PCI scan and the registers are not PCI config accessible.
>>
>> Discover RCH downstream port AER extended capability registers. This
>> requires using MMIO accesses to search for extended AER capability in
>> RCRB register space.
>>
>> [1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
>> [2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
>>
>> Co-developed-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>
> Hi Terry,
>
> Sorry I missed first few versions. Playing catch up.
>
> A few minor comments only inline.
>
>
>
>> ---
>> drivers/cxl/core/regs.c | 93 +++++++++++++++++++++++++++++++++++------
>> drivers/cxl/cxl.h | 5 +++
>> drivers/cxl/mem.c | 39 +++++++++++------
>> 3 files changed, 113 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index 1476a0299c9b..bde1fffab09e 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -332,10 +332,36 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>>
>> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
>> + char *name)
>
> dev isn't used.
>
'dev' was used earlier for logging that is since removed.
>> +{
>> +
>
> Trivial but no point in blank line here.
>
I'll remove it.
>> + if (!request_mem_region(map->resource, map->max_size, name))
>> + return NULL;
>> +
>> + map->base = ioremap(map->resource, map->max_size);
>> + if (!map->base) {
>> + release_mem_region(map->resource, map->max_size);
>> + return NULL;
>> + }
>> +
>> + return map->base;
>
> Why return a value you've already stashed in map->base?
>
This allowed for a clean return check where cxl_map_reg() is called.
This could/should have been a boolean. This will be fixed with the refactoring
mentioned below.
>> +}
>> +
>
> This is similar enough to devm_cxl_iomap_block() that I'd kind
> of like them them take the same parameters. That would mean
> moving the map structure outside of the calls and instead passing
> in the 3 relevant parameters. Perhaps not worth it.
>
The intent was to cleanup the cxl_map_reg() callers. Using a 'struct
cxl_register_map' carries all the variables required for mapping and reduces
the number of variables otherwise declared in the callers. But, I understand
why a common interface is preferred in this case.
Ok. I'll change the parameters and return value to match devm_cxl_iomap_block().
>> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
>> +{
>
> dev isn't used here either. Makes little sense to pass it in to either funtion.
>
>> + iounmap(map->base);
>> + release_mem_region(map->resource, map->max_size);
>> +}
>> +
>> resource_size_t cxl_rcrb_to_component(struct device *dev,
>> resource_size_t rcrb,
>> enum cxl_rcrb which)
>> {
>> + struct cxl_register_map map = {
>> + .resource = rcrb,
>> + .max_size = SZ_4K
>> + };
>> resource_size_t component_reg_phys;
>> void __iomem *addr;
>> u32 bar0, bar1;
>> @@ -343,7 +369,10 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>> u32 id;
>>
>> if (which == CXL_RCRB_UPSTREAM)
>> - rcrb += SZ_4K;
>> + map.resource += SZ_4K;
>> +
>> + if (!cxl_map_reg(dev, &map, "CXL RCRB"))
>> + return CXL_RESOURCE_NONE;
>>
>> /*
>> * RCRB's BAR[0..1] point to component block containing CXL
>> @@ -351,21 +380,12 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>> * the PCI Base spec here, esp. 64 bit extraction and memory
>> * ranges alignment (6.0, 7.5.1.2.1).
>> */
>> - if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
>> - return CXL_RESOURCE_NONE;
>> - addr = ioremap(rcrb, SZ_4K);
>> - if (!addr) {
>> - dev_err(dev, "Failed to map region %pr\n", addr);
>> - release_mem_region(rcrb, SZ_4K);
>> - return CXL_RESOURCE_NONE;
>> - }
>> -
>> + addr = map.base;
>
> I'd have preferred to see this refactor as a precursor patch to the
> 'real changes' that follow.
>
Ok. I can make the cxl_map_reg() addition and cxl_rcrb_to_component() refactor
to a separate patch.
>> id = readl(addr + PCI_VENDOR_ID);
>> cmd = readw(addr + PCI_COMMAND);
>> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
>> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
>> - iounmap(addr);
>> - release_mem_region(rcrb, SZ_4K);
>> + cxl_unmap_reg(dev, &map);
>>
>> /*
>> * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
>> @@ -396,3 +416,52 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>> return component_reg_phys;
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
>
>
> ...
>
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 044a92d9813e..df64c402e6e6 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -270,6 +270,9 @@ enum cxl_rcrb {
>> resource_size_t cxl_rcrb_to_component(struct device *dev,
>> resource_size_t rcrb,
>> enum cxl_rcrb which);
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
>> +u16 cxl_component_to_ras(struct device *dev,
>> + resource_size_t component_reg_phys);
>>
>> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>> #define CXL_TARGET_STRLEN 20
>> @@ -601,6 +604,8 @@ struct cxl_dport {
>> int port_id;
>> resource_size_t component_reg_phys;
>> resource_size_t rcrb;
>> + u16 aer_cap;
>> + u16 ras_cap;
>
> This structure has kernel-doc that needs to be updated for these new entries.
>
I'll add.
>> bool rch;
>> struct cxl_port *port;
>> };
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 39c4b54f0715..014295ab6bc6 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -45,13 +45,36 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>> return 0;
>> }
>>
>> +static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
>> + struct cxl_dport *parent_dport)
>> +{
>> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
>
> extra space before =
>
Ok. Ill remove the extra space.
>> +
>> + if (!parent_dport->rch)
>> + return;
>> +
>> + /*
>> + * The component registers for an RCD might come from the
>> + * host-bridge RCRB if they are not already mapped via the
>> + * typical register locator mechanism.
>> + */
>> + if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
>> + cxlds->component_reg_phys = cxl_rcrb_to_component(
>> + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
>> +
>> + parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
>> + parent_dport->rcrb);
>> +
>> + parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
>> + parent_dport->component_reg_phys);
>> +}
>> +
>> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>> struct cxl_dport *parent_dport)
>> {
>> struct cxl_port *parent_port = parent_dport->port;
>> struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> struct cxl_port *endpoint, *iter, *down;
>> - resource_size_t component_reg_phys;
>> int rc;
>>
>> /*
>> @@ -66,17 +89,9 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>> ep->next = down;
>> }
>>
>> - /*
>> - * The component registers for an RCD might come from the
>> - * host-bridge RCRB if they are not already mapped via the
>> - * typical register locator mechanism.
>> - */
>> - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
>> - component_reg_phys = cxl_rcrb_to_component(
>> - &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
>> - else
>> - component_reg_phys = cxlds->component_reg_phys;
>> - endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
>> + cxl_setup_rcrb(cxlds, parent_dport);
>> +
>> + endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys,
>> parent_dport);
> As above, I'd prefer to see this refactor done in a precursor patch before the new
> stuff is added. I like reviewing noop patches as I don't have to think much (so
> can do it when I'm supposedly in a meeting ;)
>
Ok. I'll add an earlier patch that introduces cxl_setup_rcrb() and first moves this
chunk into cxl_setup_rcrb(). The following patch will replace the cxl_setup_rcrb()
logic with the AER and RAS discovery.
My understanding is the requested refactoring changes then splits this patch into
the 3 patches listed below (using git log latest first order):
- Add RCH downstream port AER and RAS register discovery
- Refactor RCD component discovery into separate function
- Refactor RCRB register mapping into separate function
Regards,
Terry
> Jonathan
>> if (IS_ERR(endpoint))
>> return PTR_ERR(endpoint);
>
On 13.04.23 14:13:16, Terry Bowman wrote:
> On 4/13/23 10:30, Jonathan Cameron wrote:
> > On Tue, 11 Apr 2023 13:02:57 -0500
> > Terry Bowman <terry.bowman@amd.com> wrote:
> >> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
> >> + char *name)
> >
> > dev isn't used.
> >
>
> 'dev' was used earlier for logging that is since removed.
>
> >> +{
> >> +
> >
> > Trivial but no point in blank line here.
> >
>
> I'll remove it.
>
> >> + if (!request_mem_region(map->resource, map->max_size, name))
> >> + return NULL;
> >> +
> >> + map->base = ioremap(map->resource, map->max_size);
> >> + if (!map->base) {
> >> + release_mem_region(map->resource, map->max_size);
> >> + return NULL;
> >> + }
> >> +
> >> + return map->base;
> >
> > Why return a value you've already stashed in map->base?
> >
> This allowed for a clean return check where cxl_map_reg() is called.
> This could/should have been a boolean. This will be fixed with the refactoring
> mentioned below.
The intention was to have a shortcut to get the base addr directly
which could be often the case. While the remaining struct map is only
used to unmap things. To be precise, we do not check a bool here but
instead an address to be non-zero. Please to not change the return
value.
We did not use devm_* here to allow temporary mappings during init
(which might happen multiple times). With devm_* only one permanent
mapping would be possible and we would need to store and maintain the
base addr in some struct. This implementation here allows a local
usage.
>
> >> +}
> >> +
> >
> > This is similar enough to devm_cxl_iomap_block() that I'd kind
> > of like them them take the same parameters. That would mean
> > moving the map structure outside of the calls and instead passing
> > in the 3 relevant parameters. Perhaps not worth it.
> >
> The intent was to cleanup the cxl_map_reg() callers. Using a 'struct
> cxl_register_map' carries all the variables required for mapping and reduces
> the number of variables otherwise declared in the callers. But, I understand
> why a common interface is preferred in this case.
>
> Ok. I'll change the parameters and return value to match devm_cxl_iomap_block().
See my comment above.
Struct cxl_register_map was choosen to keep data in one place and also
for paired use with cxl_map_reg() and cxl_unmap_reg() (in the sense of
an object-oriented programming style). The struct is widespread used
in CXL code for similar reasons. I would prefer to keep the struct as
argument.
>
> >> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
> >> +{
> >
> > dev isn't used here either. Makes little sense to pass it in to either funtion.
Yes, dev should be removed for both functions. Thanks for catching
this.
-Robert
> >
> >> + iounmap(map->base);
> >> + release_mem_region(map->resource, map->max_size);
> >> +}
> >> +
> >> + endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys, > >> parent_dport); > > As above, I'd prefer to see this refactor done in a precursor patch before the new > > stuff is added. I like reviewing noop patches as I don't have to think much (so > > can do it when I'm supposedly in a meeting ;) > > > > Ok. I'll add an earlier patch that introduces cxl_setup_rcrb() and first moves this > chunk into cxl_setup_rcrb(). The following patch will replace the cxl_setup_rcrb() > logic with the AER and RAS discovery. > > My understanding is the requested refactoring changes then splits this patch into > the 3 patches listed below (using git log latest first order): > - Add RCH downstream port AER and RAS register discovery > - Refactor RCD component discovery into separate function > - Refactor RCRB register mapping into separate function Spot on I think. Thanks, Jonathan
Terry Bowman wrote:
> Restricted CXL host (RCH) downstream port AER information is not currently
> logged while in the error state. One problem preventing existing PCIe AER
> functions from logging errors is the AER registers are not accessible. The
> CXL driver requires changes to find RCH downstream port AER registers for
> purpose of error logging.
>
> RCH downstream ports are not enumerated during a PCI bus scan and are
> instead discovered using system firmware, ACPI in this case.[1] The
> downstream port is implemented as a Root Complex Register Block (RCRB).
> The RCRB is a 4k memory block containing PCIe registers based on the PCIe
> root port.[2] The RCRB includes AER extended capability registers used for
> reporting errors. Note, the RCH's AER Capability is located in the RCRB
> memory space instead of PCI configuration space, thus its register access
> is different. Existing kernel PCIe AER functions can not be used to manage
> the downstream port AER capabilities because the port was not enumerated
> during PCI scan and the registers are not PCI config accessible.
>
> Discover RCH downstream port AER extended capability registers. This
> requires using MMIO accesses to search for extended AER capability in
> RCRB register space.
>
> [1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
> [2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
>
> Co-developed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
> drivers/cxl/core/regs.c | 93 +++++++++++++++++++++++++++++++++++------
> drivers/cxl/cxl.h | 5 +++
> drivers/cxl/mem.c | 39 +++++++++++------
> 3 files changed, 113 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 1476a0299c9b..bde1fffab09e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -332,10 +332,36 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>
> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
> + char *name)
> +{
> +
> + if (!request_mem_region(map->resource, map->max_size, name))
> + return NULL;
> +
> + map->base = ioremap(map->resource, map->max_size);
> + if (!map->base) {
> + release_mem_region(map->resource, map->max_size);
> + return NULL;
> + }
> +
> + return map->base;
> +}
> +
> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
> +{
> + iounmap(map->base);
> + release_mem_region(map->resource, map->max_size);
> +}
Not clear why these new functions are needed vs cxl_map_regblock() /
cxl_unmap_regblock(), and this refactoring looks unrelated to the
claimed changes in the patch changelog.
...oh, I think I see why you went this way, a potential counter-proposal
below.
> +
> resource_size_t cxl_rcrb_to_component(struct device *dev,
> resource_size_t rcrb,
> enum cxl_rcrb which)
> {
> + struct cxl_register_map map = {
> + .resource = rcrb,
> + .max_size = SZ_4K
> + };
> resource_size_t component_reg_phys;
> void __iomem *addr;
> u32 bar0, bar1;
> @@ -343,7 +369,10 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> u32 id;
>
> if (which == CXL_RCRB_UPSTREAM)
> - rcrb += SZ_4K;
> + map.resource += SZ_4K;
> +
> + if (!cxl_map_reg(dev, &map, "CXL RCRB"))
> + return CXL_RESOURCE_NONE;
>
> /*
> * RCRB's BAR[0..1] point to component block containing CXL
> @@ -351,21 +380,12 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> * the PCI Base spec here, esp. 64 bit extraction and memory
> * ranges alignment (6.0, 7.5.1.2.1).
> */
> - if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> - return CXL_RESOURCE_NONE;
> - addr = ioremap(rcrb, SZ_4K);
> - if (!addr) {
> - dev_err(dev, "Failed to map region %pr\n", addr);
> - release_mem_region(rcrb, SZ_4K);
> - return CXL_RESOURCE_NONE;
> - }
> -
> + addr = map.base;
> id = readl(addr + PCI_VENDOR_ID);
> cmd = readw(addr + PCI_COMMAND);
> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> - iounmap(addr);
> - release_mem_region(rcrb, SZ_4K);
> + cxl_unmap_reg(dev, &map);
>
> /*
> * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> @@ -396,3 +416,52 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> return component_reg_phys;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> +
> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb)
> +{
> + struct cxl_register_map map = {
> + .resource = rcrb,
> + .max_size = SZ_4K,
> + };
> + u32 cap_hdr;
> + u16 offset = 0;
> +
> + if (!cxl_map_reg(dev, &map, "CXL RCRB"))
> + return 0;
> +
> + cap_hdr = readl(map.base + offset);
> + while (PCI_EXT_CAP_ID(cap_hdr) != PCI_EXT_CAP_ID_ERR) {
> +
> + offset = PCI_EXT_CAP_NEXT(cap_hdr);
> + if (!offset) {
> + cxl_unmap_reg(dev, &map);
> + return 0;
> + }
> + cap_hdr = readl(map.base + offset);
> + }
> +
> + dev_dbg(dev, "found AER extended capability (0x%x)\n", offset);
> + cxl_unmap_reg(dev, &map);
> +
> + return offset;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_aer, CXL);
> +
> +u16 cxl_component_to_ras(struct device *dev, resource_size_t component_reg_phys)
> +{
> + struct cxl_register_map map = {
> + .resource = component_reg_phys,
> + .max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> + };
> +
> + if (!cxl_map_reg(dev, &map, "component"))
> + return 0;
> +
> + cxl_probe_component_regs(dev, map.base, &map.component_map);
> + cxl_unmap_reg(dev, &map);
> + if (!map.component_map.ras.valid)
> + return 0;
> +
> + return map.component_map.ras.offset;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_component_to_ras, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 044a92d9813e..df64c402e6e6 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -270,6 +270,9 @@ enum cxl_rcrb {
> resource_size_t cxl_rcrb_to_component(struct device *dev,
> resource_size_t rcrb,
> enum cxl_rcrb which);
> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
> +u16 cxl_component_to_ras(struct device *dev,
> + resource_size_t component_reg_phys);
>
> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> #define CXL_TARGET_STRLEN 20
> @@ -601,6 +604,8 @@ struct cxl_dport {
> int port_id;
> resource_size_t component_reg_phys;
> resource_size_t rcrb;
> + u16 aer_cap;
> + u16 ras_cap;
> bool rch;
> struct cxl_port *port;
> };
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 39c4b54f0715..014295ab6bc6 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -45,13 +45,36 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> return 0;
> }
>
> +static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
> + struct cxl_dport *parent_dport)
> +{
> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +
> + if (!parent_dport->rch)
> + return;
> +
> + /*
> + * The component registers for an RCD might come from the
> + * host-bridge RCRB if they are not already mapped via the
> + * typical register locator mechanism.
> + */
> + if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> + cxlds->component_reg_phys = cxl_rcrb_to_component(
> + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> +
> + parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
> + parent_dport->rcrb);
Hmm, how about just retrieve this as part of cxl_rcrb_to_component()
(renamed to cxl_probe_rcrb()), and make an rch dport its own distinct
object? Otherwise it feels odd to be retrieving downstream port
properties this late at upstream port component register detection time.
It also feels awkward to keep adding more RCH dport specific details to
the common 'struct cxl_dport'. So, I'm thinking something like the
following (compiled and cxl_test regression passed):
-- >8 --
From 18fbc72f98655d10301c7a35f614b6152f46c44b Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Mon, 17 Apr 2023 15:45:50 -0700
Subject: [PATCH] cxl/rch: Prepare for caching the MMIO mapped PCIe AER
capability
Prepare cxl_probe_rcrb() for retrieving more than just the component
register block. The RCH AER handling code wants to get back to the AER
capability that happens to be MMIO mapped rather then configuration
cycles.
Move rcrb specific dport data, like the RCRB base and the AER capability
offset, into its own data structure ('struct cxl_rcrb_info') for
cxl_probe_rcrb() to fill. Introduce 'struct cxl_rch_dport' to wrap a
'struct cxl_dport' with a 'struct cxl_rcrb_info' attribute.
This centralizes all RCRB scanning in one routine.
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/cxl/acpi.c | 16 ++++++++--------
drivers/cxl/core/port.c | 33 +++++++++++++++++++++------------
drivers/cxl/core/regs.c | 12 ++++++++----
drivers/cxl/cxl.h | 21 +++++++++++++++------
drivers/cxl/mem.c | 15 ++++++++++-----
tools/testing/cxl/Kbuild | 2 +-
tools/testing/cxl/test/cxl.c | 10 ++++++----
tools/testing/cxl/test/mock.c | 12 ++++++------
tools/testing/cxl/test/mock.h | 7 ++++---
9 files changed, 79 insertions(+), 49 deletions(-)
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 4e66483f1fd3..2647eb04fcdb 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -375,7 +375,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
struct cxl_chbs_context {
struct device *dev;
unsigned long long uid;
- resource_size_t rcrb;
+ struct cxl_rcrb_info rcrb;
resource_size_t chbcr;
u32 cxl_version;
};
@@ -395,7 +395,7 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
return 0;
ctx->cxl_version = chbs->cxl_version;
- ctx->rcrb = CXL_RESOURCE_NONE;
+ ctx->rcrb.base = CXL_RESOURCE_NONE;
ctx->chbcr = CXL_RESOURCE_NONE;
if (!chbs->base)
@@ -409,9 +409,8 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
if (chbs->length != CXL_RCRB_SIZE)
return 0;
- ctx->rcrb = chbs->base;
- ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
- CXL_RCRB_DOWNSTREAM);
+ ctx->chbcr = cxl_probe_rcrb(ctx->dev, chbs->base, &ctx->rcrb,
+ CXL_RCRB_DOWNSTREAM);
return 0;
}
@@ -451,8 +450,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
return 0;
}
- if (ctx.rcrb != CXL_RESOURCE_NONE)
- dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
+ if (ctx.rcrb.base != CXL_RESOURCE_NONE)
+ dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid,
+ &ctx.rcrb.base);
if (ctx.chbcr == CXL_RESOURCE_NONE) {
dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n",
@@ -466,7 +466,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
bridge = pci_root->bus->bridge;
if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
- ctx.chbcr, ctx.rcrb);
+ ctx.chbcr, &ctx.rcrb);
else
dport = devm_cxl_add_dport(root_port, bridge, uid,
ctx.chbcr);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 4003f445320c..d194f48259ff 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -920,7 +920,7 @@ static void cxl_dport_unlink(void *data)
static struct cxl_dport *
__devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
int port_id, resource_size_t component_reg_phys,
- resource_size_t rcrb)
+ struct cxl_rcrb_info *ri)
{
char link_name[CXL_TARGET_STRLEN];
struct cxl_dport *dport;
@@ -942,17 +942,26 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
CXL_TARGET_STRLEN)
return ERR_PTR(-EINVAL);
- dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
- if (!dport)
- return ERR_PTR(-ENOMEM);
+ if (ri && ri->base != CXL_RESOURCE_NONE) {
+ struct cxl_rch_dport *rdport;
+
+ rdport = devm_kzalloc(host, sizeof(*rdport), GFP_KERNEL);
+ if (!rdport)
+ return ERR_PTR(-ENOMEM);
+
+ rdport->rcrb.base = ri->base;
+ dport = &rdport->dport;
+ dport->rch = true;
+ } else {
+ dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
+ if (!dport)
+ return ERR_PTR(-ENOMEM);
+ }
dport->dport = dport_dev;
dport->port_id = port_id;
dport->component_reg_phys = component_reg_phys;
dport->port = port;
- if (rcrb != CXL_RESOURCE_NONE)
- dport->rch = true;
- dport->rcrb = rcrb;
cond_cxl_root_lock(port);
rc = add_dport(port, dport);
@@ -994,7 +1003,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
struct cxl_dport *dport;
dport = __devm_cxl_add_dport(port, dport_dev, port_id,
- component_reg_phys, CXL_RESOURCE_NONE);
+ component_reg_phys, NULL);
if (IS_ERR(dport)) {
dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
dev_name(&port->dev), PTR_ERR(dport));
@@ -1013,24 +1022,24 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
* @dport_dev: firmware or PCI device representing the dport
* @port_id: identifier for this dport in a decoder's target list
* @component_reg_phys: optional location of CXL component registers
- * @rcrb: mandatory location of a Root Complex Register Block
+ * @ri: mandatory data about the Root Complex Register Block layout
*
* See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
*/
struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
struct device *dport_dev, int port_id,
resource_size_t component_reg_phys,
- resource_size_t rcrb)
+ struct cxl_rcrb_info *ri)
{
struct cxl_dport *dport;
- if (rcrb == CXL_RESOURCE_NONE) {
+ if (!ri || ri->base == CXL_RESOURCE_NONE) {
dev_dbg(&port->dev, "failed to add RCH dport, missing RCRB\n");
return ERR_PTR(-EINVAL);
}
dport = __devm_cxl_add_dport(port, dport_dev, port_id,
- component_reg_phys, rcrb);
+ component_reg_phys, ri);
if (IS_ERR(dport)) {
dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
dev_name(&port->dev), PTR_ERR(dport));
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 52d1dbeda527..b1c0db898a50 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -332,9 +332,8 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
}
EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
-resource_size_t cxl_rcrb_to_component(struct device *dev,
- resource_size_t rcrb,
- enum cxl_rcrb which)
+resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
+ struct cxl_rcrb_info *ri, enum cxl_rcrb which)
{
resource_size_t component_reg_phys;
void __iomem *addr;
@@ -344,6 +343,8 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
if (which == CXL_RCRB_UPSTREAM)
rcrb += SZ_4K;
+ else
+ ri->base = rcrb;
/*
* RCRB's BAR[0..1] point to component block containing CXL
@@ -364,6 +365,9 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
cmd = readw(addr + PCI_COMMAND);
bar0 = readl(addr + PCI_BASE_ADDRESS_0);
bar1 = readl(addr + PCI_BASE_ADDRESS_1);
+
+ /* TODO: retrieve rcrb->aer_cap here */
+
iounmap(addr);
release_mem_region(rcrb, SZ_4K);
@@ -395,4 +399,4 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
return component_reg_phys;
}
-EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
+EXPORT_SYMBOL_NS_GPL(cxl_probe_rcrb, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 1503ccec9a84..b0807f54e9fd 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -267,9 +267,9 @@ enum cxl_rcrb {
CXL_RCRB_DOWNSTREAM,
CXL_RCRB_UPSTREAM,
};
-resource_size_t cxl_rcrb_to_component(struct device *dev,
- resource_size_t rcrb,
- enum cxl_rcrb which);
+struct cxl_rcrb_info;
+resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
+ struct cxl_rcrb_info *ri, enum cxl_rcrb which);
#define CXL_RESOURCE_NONE ((resource_size_t) -1)
#define CXL_TARGET_STRLEN 20
@@ -589,12 +589,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
return xa_load(&port->dports, (unsigned long)dport_dev);
}
+
/**
* struct cxl_dport - CXL downstream port
* @dport: PCI bridge or firmware device representing the downstream link
* @port_id: unique hardware identifier for dport in decoder target list
* @component_reg_phys: downstream port component registers
- * @rcrb: base address for the Root Complex Register Block
* @rch: Indicate whether this dport was enumerated in RCH or VH mode
* @port: reference to cxl_port that contains this downstream port
*/
@@ -602,11 +602,20 @@ struct cxl_dport {
struct device *dport;
int port_id;
resource_size_t component_reg_phys;
- resource_size_t rcrb;
bool rch;
struct cxl_port *port;
};
+struct cxl_rcrb_info {
+ resource_size_t base;
+ u16 aer_cap;
+};
+
+struct cxl_rch_dport {
+ struct cxl_dport dport;
+ struct cxl_rcrb_info rcrb;
+};
+
/**
* struct cxl_ep - track an endpoint's interest in a port
* @ep: device that hosts a generic CXL endpoint (expander or accelerator)
@@ -674,7 +683,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
struct device *dport_dev, int port_id,
resource_size_t component_reg_phys,
- resource_size_t rcrb);
+ struct cxl_rcrb_info *ri);
struct cxl_decoder *to_cxl_decoder(struct device *dev);
struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 097d86dd2a8e..7da6135e0b17 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -71,10 +71,15 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
* host-bridge RCRB if they are not already mapped via the
* typical register locator mechanism.
*/
- if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
- component_reg_phys = cxl_rcrb_to_component(
- &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
- else
+ if (parent_dport->rch &&
+ cxlds->component_reg_phys == CXL_RESOURCE_NONE) {
+ struct cxl_rch_dport *rdport =
+ container_of(parent_dport, typeof(*rdport), dport);
+
+ component_reg_phys =
+ cxl_probe_rcrb(&cxlmd->dev, rdport->rcrb.base,
+ &rdport->rcrb, CXL_RCRB_UPSTREAM);
+ } else
component_reg_phys = cxlds->component_reg_phys;
endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
parent_dport);
@@ -92,7 +97,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
}
return 0;
-}
+ }
static int cxl_mem_probe(struct device *dev)
{
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index fba7bec96acd..bef1bc3bd912 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -11,7 +11,7 @@ ldflags-y += --wrap=devm_cxl_enumerate_decoders
ldflags-y += --wrap=cxl_await_media_ready
ldflags-y += --wrap=cxl_hdm_decode_init
ldflags-y += --wrap=cxl_dvsec_rr_decode
-ldflags-y += --wrap=cxl_rcrb_to_component
+ldflags-y += --wrap=cxl_probe_rcrb
DRIVERS := ../../../drivers
CXL_SRC := $(DRIVERS)/cxl
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 385cdeeab22c..805c79491485 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -983,12 +983,14 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
return 0;
}
-resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
- resource_size_t rcrb,
- enum cxl_rcrb which)
+resource_size_t mock_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
+ struct cxl_rcrb_info *ri, enum cxl_rcrb which)
{
dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
+ if (which == CXL_RCRB_DOWNSTREAM)
+ ri->base = rcrb;
+
return (resource_size_t) which + 1;
}
@@ -1000,7 +1002,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
.is_mock_dev = is_mock_dev,
.acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
.acpi_evaluate_integer = mock_acpi_evaluate_integer,
- .cxl_rcrb_to_component = mock_cxl_rcrb_to_component,
+ .cxl_probe_rcrb = mock_cxl_probe_rcrb,
.acpi_pci_find_root = mock_acpi_pci_find_root,
.devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
.devm_cxl_setup_hdm = mock_cxl_setup_hdm,
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index c4e53f22e421..148bd4f184f5 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -244,9 +244,9 @@ int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
}
EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_rr_decode, CXL);
-resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
- resource_size_t rcrb,
- enum cxl_rcrb which)
+resource_size_t __wrap_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
+ struct cxl_rcrb_info *ri,
+ enum cxl_rcrb which)
{
int index;
resource_size_t component_reg_phys;
@@ -254,14 +254,14 @@ resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
if (ops && ops->is_mock_port(dev))
component_reg_phys =
- ops->cxl_rcrb_to_component(dev, rcrb, which);
+ ops->cxl_probe_rcrb(dev, rcrb, ri, which);
else
- component_reg_phys = cxl_rcrb_to_component(dev, rcrb, which);
+ component_reg_phys = cxl_probe_rcrb(dev, rcrb, ri, which);
put_cxl_mock_ops(index);
return component_reg_phys;
}
-EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcrb_to_component, CXL);
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_probe_rcrb, CXL);
MODULE_LICENSE("GPL v2");
MODULE_IMPORT_NS(ACPI);
diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
index bef8817b01f2..7ef21356d052 100644
--- a/tools/testing/cxl/test/mock.h
+++ b/tools/testing/cxl/test/mock.h
@@ -15,9 +15,10 @@ struct cxl_mock_ops {
acpi_string pathname,
struct acpi_object_list *arguments,
unsigned long long *data);
- resource_size_t (*cxl_rcrb_to_component)(struct device *dev,
- resource_size_t rcrb,
- enum cxl_rcrb which);
+ resource_size_t (*cxl_probe_rcrb)(struct device *dev,
+ resource_size_t rcrb,
+ struct cxl_rcrb_info *ri,
+ enum cxl_rcrb which);
struct acpi_pci_root *(*acpi_pci_find_root)(acpi_handle handle);
bool (*is_mock_bus)(struct pci_bus *bus);
bool (*is_mock_port)(struct device *dev);
--
2.39.2
-- >8 --
> +
> + parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
> + parent_dport->component_reg_phys);
Since this is component register offset based can it not be shared with
the VH case? I have been expecting that RCH RAS capability and VH RAS
capability scanning would need to be unified in the cxl_port driver.Hi Dan,
see comment to your patch below.
On 17.04.23 16:00:47, Dan Williams wrote:
> Terry Bowman wrote:
> > + /*
> > + * The component registers for an RCD might come from the
> > + * host-bridge RCRB if they are not already mapped via the
> > + * typical register locator mechanism.
> > + */
> > + if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> > + cxlds->component_reg_phys = cxl_rcrb_to_component(
> > + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> > +
> > + parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
> > + parent_dport->rcrb);
>
> Hmm, how about just retrieve this as part of cxl_rcrb_to_component()
> (renamed to cxl_probe_rcrb()), and make an rch dport its own distinct
> object? Otherwise it feels odd to be retrieving downstream port
> properties this late at upstream port component register detection time.
> It also feels awkward to keep adding more RCH dport specific details to
> the common 'struct cxl_dport'. So, I'm thinking something like the
> following (compiled and cxl_test regression passed):
>
> -- >8 --
> From 18fbc72f98655d10301c7a35f614b6152f46c44b Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Mon, 17 Apr 2023 15:45:50 -0700
> Subject: [PATCH] cxl/rch: Prepare for caching the MMIO mapped PCIe AER
> capability
>
> Prepare cxl_probe_rcrb() for retrieving more than just the component
> register block. The RCH AER handling code wants to get back to the AER
> capability that happens to be MMIO mapped rather then configuration
> cycles.
>
> Move rcrb specific dport data, like the RCRB base and the AER capability
> offset, into its own data structure ('struct cxl_rcrb_info') for
> cxl_probe_rcrb() to fill. Introduce 'struct cxl_rch_dport' to wrap a
> 'struct cxl_dport' with a 'struct cxl_rcrb_info' attribute.
>
> This centralizes all RCRB scanning in one routine.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/acpi.c | 16 ++++++++--------
> drivers/cxl/core/port.c | 33 +++++++++++++++++++++------------
> drivers/cxl/core/regs.c | 12 ++++++++----
> drivers/cxl/cxl.h | 21 +++++++++++++++------
> drivers/cxl/mem.c | 15 ++++++++++-----
> tools/testing/cxl/Kbuild | 2 +-
> tools/testing/cxl/test/cxl.c | 10 ++++++----
> tools/testing/cxl/test/mock.c | 12 ++++++------
> tools/testing/cxl/test/mock.h | 7 ++++---
> 9 files changed, 79 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 4e66483f1fd3..2647eb04fcdb 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -375,7 +375,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> struct cxl_chbs_context {
> struct device *dev;
> unsigned long long uid;
> - resource_size_t rcrb;
> + struct cxl_rcrb_info rcrb;
> resource_size_t chbcr;
> u32 cxl_version;
> };
> @@ -395,7 +395,7 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> return 0;
>
> ctx->cxl_version = chbs->cxl_version;
> - ctx->rcrb = CXL_RESOURCE_NONE;
> + ctx->rcrb.base = CXL_RESOURCE_NONE;
> ctx->chbcr = CXL_RESOURCE_NONE;
>
> if (!chbs->base)
> @@ -409,9 +409,8 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> if (chbs->length != CXL_RCRB_SIZE)
> return 0;
>
> - ctx->rcrb = chbs->base;
> - ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
> - CXL_RCRB_DOWNSTREAM);
> + ctx->chbcr = cxl_probe_rcrb(ctx->dev, chbs->base, &ctx->rcrb,
> + CXL_RCRB_DOWNSTREAM);
Let's just extract the rcrb base here and do the probe later in
__devm_cxl_add_dport(). Which means chbcr will be extracted there and
we completely remove the cxl_rcrb_to_component() here. The code here
becomes much simpler and the ACPI table parser no longer contains mmio
mapping calls etc.
>
> return 0;
> }
> @@ -451,8 +450,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> return 0;
> }
>
> - if (ctx.rcrb != CXL_RESOURCE_NONE)
> - dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> + if (ctx.rcrb.base != CXL_RESOURCE_NONE)
> + dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid,
> + &ctx.rcrb.base);
>
> if (ctx.chbcr == CXL_RESOURCE_NONE) {
> dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n",
> @@ -466,7 +466,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> bridge = pci_root->bus->bridge;
> if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
> dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
> - ctx.chbcr, ctx.rcrb);
> + ctx.chbcr, &ctx.rcrb);
> else
> dport = devm_cxl_add_dport(root_port, bridge, uid,
> ctx.chbcr);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 4003f445320c..d194f48259ff 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -920,7 +920,7 @@ static void cxl_dport_unlink(void *data)
> static struct cxl_dport *
> __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> int port_id, resource_size_t component_reg_phys,
> - resource_size_t rcrb)
> + struct cxl_rcrb_info *ri)
> {
> char link_name[CXL_TARGET_STRLEN];
> struct cxl_dport *dport;
> @@ -942,17 +942,26 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> CXL_TARGET_STRLEN)
> return ERR_PTR(-EINVAL);
>
> - dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> - if (!dport)
> - return ERR_PTR(-ENOMEM);
> + if (ri && ri->base != CXL_RESOURCE_NONE) {
> + struct cxl_rch_dport *rdport;
> +
> + rdport = devm_kzalloc(host, sizeof(*rdport), GFP_KERNEL);
> + if (!rdport)
> + return ERR_PTR(-ENOMEM);
> +
> + rdport->rcrb.base = ri->base;
> + dport = &rdport->dport;
> + dport->rch = true;
> + } else {
> + dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> + if (!dport)
> + return ERR_PTR(-ENOMEM);
I think we can simlify the allocation part if we just move the struct
into 'struct cxl_dport', see below.
> + }
>
> dport->dport = dport_dev;
> dport->port_id = port_id;
> dport->component_reg_phys = component_reg_phys;
> dport->port = port;
> - if (rcrb != CXL_RESOURCE_NONE)
> - dport->rch = true;
> - dport->rcrb = rcrb;
>
> cond_cxl_root_lock(port);
> rc = add_dport(port, dport);
> @@ -994,7 +1003,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> struct cxl_dport *dport;
>
> dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> - component_reg_phys, CXL_RESOURCE_NONE);
> + component_reg_phys, NULL);
> if (IS_ERR(dport)) {
> dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
> dev_name(&port->dev), PTR_ERR(dport));
> @@ -1013,24 +1022,24 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
> * @dport_dev: firmware or PCI device representing the dport
> * @port_id: identifier for this dport in a decoder's target list
> * @component_reg_phys: optional location of CXL component registers
> - * @rcrb: mandatory location of a Root Complex Register Block
> + * @ri: mandatory data about the Root Complex Register Block layout
> *
> * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
> */
> struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> struct device *dport_dev, int port_id,
> resource_size_t component_reg_phys,
> - resource_size_t rcrb)
> + struct cxl_rcrb_info *ri)
> {
> struct cxl_dport *dport;
>
> - if (rcrb == CXL_RESOURCE_NONE) {
> + if (!ri || ri->base == CXL_RESOURCE_NONE) {
> dev_dbg(&port->dev, "failed to add RCH dport, missing RCRB\n");
> return ERR_PTR(-EINVAL);
> }
>
> dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> - component_reg_phys, rcrb);
> + component_reg_phys, ri);
> if (IS_ERR(dport)) {
> dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> dev_name(&port->dev), PTR_ERR(dport));
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 52d1dbeda527..b1c0db898a50 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -332,9 +332,8 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which)
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which)
> {
> resource_size_t component_reg_phys;
> void __iomem *addr;
> @@ -344,6 +343,8 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>
> if (which == CXL_RCRB_UPSTREAM)
> rcrb += SZ_4K;
> + else
> + ri->base = rcrb;
For upstream ports nothing is written to ri, allow NULL pointer for ri
then but check for NULL here.
>
> /*
> * RCRB's BAR[0..1] point to component block containing CXL
> @@ -364,6 +365,9 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> cmd = readw(addr + PCI_COMMAND);
> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +
> + /* TODO: retrieve rcrb->aer_cap here */
> +
Yes, very good. The aer cap of the RCRB would be very early available
then and independent of of other drivers than cxl_acpi, esp. the pci
subsystem.
> iounmap(addr);
> release_mem_region(rcrb, SZ_4K);
>
> @@ -395,4 +399,4 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>
> return component_reg_phys;
> }
> -EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_probe_rcrb, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1503ccec9a84..b0807f54e9fd 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -267,9 +267,9 @@ enum cxl_rcrb {
> CXL_RCRB_DOWNSTREAM,
> CXL_RCRB_UPSTREAM,
> };
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which);
> +struct cxl_rcrb_info;
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which);
>
> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> #define CXL_TARGET_STRLEN 20
> @@ -589,12 +589,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
> return xa_load(&port->dports, (unsigned long)dport_dev);
> }
>
> +
We will drop that.
> /**
> * struct cxl_dport - CXL downstream port
> * @dport: PCI bridge or firmware device representing the downstream link
> * @port_id: unique hardware identifier for dport in decoder target list
> * @component_reg_phys: downstream port component registers
> - * @rcrb: base address for the Root Complex Register Block
> * @rch: Indicate whether this dport was enumerated in RCH or VH mode
> * @port: reference to cxl_port that contains this downstream port
> */
> @@ -602,11 +602,20 @@ struct cxl_dport {
> struct device *dport;
> int port_id;
> resource_size_t component_reg_phys;
> - resource_size_t rcrb;
> bool rch;
> struct cxl_port *port;
> };
>
> +struct cxl_rcrb_info {
> + resource_size_t base;
> + u16 aer_cap;
> +};
> +
> +struct cxl_rch_dport {
> + struct cxl_dport dport;
> + struct cxl_rcrb_info rcrb;
> +};
How about including cxl_rcrb_info directly in cxl_dport? This
simplifies dport allocation and allows direct access in cxl_dport to
the cxl_rcrb_info without a container_of() call:
struct cxl_dport {
struct device *dport;
struct cxl_port *port;
int port_id;
resource_size_t component_reg_phys;
bool rch;
struct cxl_rcrb_info rcrb;
};
I know you were complaining about to many RCH dport specific details,
but all this is kept in cxl_rcrb_info and the struct itself is not too
big. The flat structure allows quick access, like:
if (dport->rch)
component_reg_phys = cxl_probe_rcrb(..., dport->rcrb.base, ...)
> +
> /**
> * struct cxl_ep - track an endpoint's interest in a port
> * @ep: device that hosts a generic CXL endpoint (expander or accelerator)
> @@ -674,7 +683,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> struct device *dport_dev, int port_id,
> resource_size_t component_reg_phys,
> - resource_size_t rcrb);
> + struct cxl_rcrb_info *ri);
>
> struct cxl_decoder *to_cxl_decoder(struct device *dev);
> struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 097d86dd2a8e..7da6135e0b17 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -71,10 +71,15 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> * host-bridge RCRB if they are not already mapped via the
> * typical register locator mechanism.
> */
> - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> - component_reg_phys = cxl_rcrb_to_component(
> - &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> - else
> + if (parent_dport->rch &&
> + cxlds->component_reg_phys == CXL_RESOURCE_NONE) {
> + struct cxl_rch_dport *rdport =
> + container_of(parent_dport, typeof(*rdport), dport);
> +
> + component_reg_phys =
> + cxl_probe_rcrb(&cxlmd->dev, rdport->rcrb.base,
> + &rdport->rcrb, CXL_RCRB_UPSTREAM);
This could overwrite the dport's contents with the upstream port info.
But since we only need the info and write to it in case of a
downstream port, let's set that to null here (plus adding a check in
cxl_probe_rcrb()).
Similar to the host case (cxl_acpi driver) where the rcrb is probed
early, this code should be moved to cxl_pci. But since RAS does not
use the upstream port's RCRB it is subject of a separate patch not
part of this series.
> + } else
> component_reg_phys = cxlds->component_reg_phys;
> endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
> parent_dport);
> @@ -92,7 +97,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> }
>
> return 0;
> -}
> + }
Dropping that change.
>
> static int cxl_mem_probe(struct device *dev)
> {
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index fba7bec96acd..bef1bc3bd912 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -11,7 +11,7 @@ ldflags-y += --wrap=devm_cxl_enumerate_decoders
> ldflags-y += --wrap=cxl_await_media_ready
> ldflags-y += --wrap=cxl_hdm_decode_init
> ldflags-y += --wrap=cxl_dvsec_rr_decode
> -ldflags-y += --wrap=cxl_rcrb_to_component
> +ldflags-y += --wrap=cxl_probe_rcrb
>
> DRIVERS := ../../../drivers
> CXL_SRC := $(DRIVERS)/cxl
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 385cdeeab22c..805c79491485 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -983,12 +983,14 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
> return 0;
> }
>
> -resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which)
> +resource_size_t mock_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which)
> {
> dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
>
> + if (which == CXL_RCRB_DOWNSTREAM)
> + ri->base = rcrb;
> +
> return (resource_size_t) which + 1;
> }
>
> @@ -1000,7 +1002,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
> .is_mock_dev = is_mock_dev,
> .acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
> .acpi_evaluate_integer = mock_acpi_evaluate_integer,
> - .cxl_rcrb_to_component = mock_cxl_rcrb_to_component,
> + .cxl_probe_rcrb = mock_cxl_probe_rcrb,
> .acpi_pci_find_root = mock_acpi_pci_find_root,
> .devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
> .devm_cxl_setup_hdm = mock_cxl_setup_hdm,
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index c4e53f22e421..148bd4f184f5 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -244,9 +244,9 @@ int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
> }
> EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_rr_decode, CXL);
>
> -resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which)
> +resource_size_t __wrap_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri,
> + enum cxl_rcrb which)
> {
> int index;
> resource_size_t component_reg_phys;
> @@ -254,14 +254,14 @@ resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
>
> if (ops && ops->is_mock_port(dev))
> component_reg_phys =
> - ops->cxl_rcrb_to_component(dev, rcrb, which);
> + ops->cxl_probe_rcrb(dev, rcrb, ri, which);
> else
> - component_reg_phys = cxl_rcrb_to_component(dev, rcrb, which);
> + component_reg_phys = cxl_probe_rcrb(dev, rcrb, ri, which);
> put_cxl_mock_ops(index);
>
> return component_reg_phys;
> }
> -EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(__wrap_cxl_probe_rcrb, CXL);
>
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(ACPI);
> diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
> index bef8817b01f2..7ef21356d052 100644
> --- a/tools/testing/cxl/test/mock.h
> +++ b/tools/testing/cxl/test/mock.h
> @@ -15,9 +15,10 @@ struct cxl_mock_ops {
> acpi_string pathname,
> struct acpi_object_list *arguments,
> unsigned long long *data);
> - resource_size_t (*cxl_rcrb_to_component)(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which);
> + resource_size_t (*cxl_probe_rcrb)(struct device *dev,
> + resource_size_t rcrb,
> + struct cxl_rcrb_info *ri,
> + enum cxl_rcrb which);
> struct acpi_pci_root *(*acpi_pci_find_root)(acpi_handle handle);
> bool (*is_mock_bus)(struct pci_bus *bus);
> bool (*is_mock_port)(struct device *dev);
> --
> 2.39.2
> -- >8 --
>
>
> > +
> > + parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
> > + parent_dport->component_reg_phys);
>
> Since this is component register offset based can it not be shared with
> the VH case? I have been expecting that RCH RAS capability and VH RAS
> capability scanning would need to be unified in the cxl_port driver.
I have a modified version of your patch with following changes:
* cxl_probe_rcrb():
* Moved cxl_probe_rcrb() out of ACPI CEDT parse to
__devm_cxl_add_dport() (separate patch).
* Set cxl_rcrb_info pointer to NULL for upstream ports including
NULL pointer check.
* Integrated 'struct cxl_rcrb_info' in 'struct cxl_dport'.
* Adressed comments above.
* Dropped unrelated newlines and whitespaces.
We will include the patches in v4.
Thanks,
-Robert
Hi Dan,
Thanks for the review comments. I added responses inline below.
On 4/17/23 18:00, Dan Williams wrote:
> Terry Bowman wrote:
>> Restricted CXL host (RCH) downstream port AER information is not currently
>> logged while in the error state. One problem preventing existing PCIe AER
>> functions from logging errors is the AER registers are not accessible. The
>> CXL driver requires changes to find RCH downstream port AER registers for
>> purpose of error logging.
>>
>> RCH downstream ports are not enumerated during a PCI bus scan and are
>> instead discovered using system firmware, ACPI in this case.[1] The
>> downstream port is implemented as a Root Complex Register Block (RCRB).
>> The RCRB is a 4k memory block containing PCIe registers based on the PCIe
>> root port.[2] The RCRB includes AER extended capability registers used for
>> reporting errors. Note, the RCH's AER Capability is located in the RCRB
>> memory space instead of PCI configuration space, thus its register access
>> is different. Existing kernel PCIe AER functions can not be used to manage
>> the downstream port AER capabilities because the port was not enumerated
>> during PCI scan and the registers are not PCI config accessible.
>>
>> Discover RCH downstream port AER extended capability registers. This
>> requires using MMIO accesses to search for extended AER capability in
>> RCRB register space.
>>
>> [1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
>> [2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
>>
>> Co-developed-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> ---
>> drivers/cxl/core/regs.c | 93 +++++++++++++++++++++++++++++++++++------
>> drivers/cxl/cxl.h | 5 +++
>> drivers/cxl/mem.c | 39 +++++++++++------
>> 3 files changed, 113 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index 1476a0299c9b..bde1fffab09e 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -332,10 +332,36 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>>
>> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
>> + char *name)
>> +{
>> +
>> + if (!request_mem_region(map->resource, map->max_size, name))
>> + return NULL;
>> +
>> + map->base = ioremap(map->resource, map->max_size);
>> + if (!map->base) {
>> + release_mem_region(map->resource, map->max_size);
>> + return NULL;
>> + }
>> +
>> + return map->base;
>> +}
>> +
>> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
>> +{
>> + iounmap(map->base);
>> + release_mem_region(map->resource, map->max_size);
>> +}
>
> Not clear why these new functions are needed vs cxl_map_regblock() /
> cxl_unmap_regblock(), and this refactoring looks unrelated to the
> claimed changes in the patch changelog.
>
> ...oh, I think I see why you went this way, a potential counter-proposal
> below.
>
>> +
>> resource_size_t cxl_rcrb_to_component(struct device *dev,
>> resource_size_t rcrb,
>> enum cxl_rcrb which)
>> {
>> + struct cxl_register_map map = {
>> + .resource = rcrb,
>> + .max_size = SZ_4K
>> + };
>> resource_size_t component_reg_phys;
>> void __iomem *addr;
>> u32 bar0, bar1;
>> @@ -343,7 +369,10 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>> u32 id;
>>
>> if (which == CXL_RCRB_UPSTREAM)
>> - rcrb += SZ_4K;
>> + map.resource += SZ_4K;
>> +
>> + if (!cxl_map_reg(dev, &map, "CXL RCRB"))
>> + return CXL_RESOURCE_NONE;
>>
>> /*
>> * RCRB's BAR[0..1] point to component block containing CXL
>> @@ -351,21 +380,12 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>> * the PCI Base spec here, esp. 64 bit extraction and memory
>> * ranges alignment (6.0, 7.5.1.2.1).
>> */
>> - if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
>> - return CXL_RESOURCE_NONE;
>> - addr = ioremap(rcrb, SZ_4K);
>> - if (!addr) {
>> - dev_err(dev, "Failed to map region %pr\n", addr);
>> - release_mem_region(rcrb, SZ_4K);
>> - return CXL_RESOURCE_NONE;
>> - }
>> -
>> + addr = map.base;
>> id = readl(addr + PCI_VENDOR_ID);
>> cmd = readw(addr + PCI_COMMAND);
>> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
>> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
>> - iounmap(addr);
>> - release_mem_region(rcrb, SZ_4K);
>> + cxl_unmap_reg(dev, &map);
>>
>> /*
>> * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
>> @@ -396,3 +416,52 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>> return component_reg_phys;
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
>> +
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb)
>> +{
>> + struct cxl_register_map map = {
>> + .resource = rcrb,
>> + .max_size = SZ_4K,
>> + };
>> + u32 cap_hdr;
>> + u16 offset = 0;
>> +
>> + if (!cxl_map_reg(dev, &map, "CXL RCRB"))
>> + return 0;
>> +
>> + cap_hdr = readl(map.base + offset);
>> + while (PCI_EXT_CAP_ID(cap_hdr) != PCI_EXT_CAP_ID_ERR) {
>> +
>> + offset = PCI_EXT_CAP_NEXT(cap_hdr);
>> + if (!offset) {
>> + cxl_unmap_reg(dev, &map);
>> + return 0;
>> + }
>> + cap_hdr = readl(map.base + offset);
>> + }
>> +
>> + dev_dbg(dev, "found AER extended capability (0x%x)\n", offset);
>> + cxl_unmap_reg(dev, &map);
>> +
>> + return offset;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_aer, CXL);
>
>> +
>> +u16 cxl_component_to_ras(struct device *dev, resource_size_t component_reg_phys)
>> +{
>> + struct cxl_register_map map = {
>> + .resource = component_reg_phys,
>> + .max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
>> + };
>> +
>> + if (!cxl_map_reg(dev, &map, "component"))
>> + return 0;
>> +
>> + cxl_probe_component_regs(dev, map.base, &map.component_map);
>> + cxl_unmap_reg(dev, &map);
>> + if (!map.component_map.ras.valid)
>> + return 0;
>> +
>> + return map.component_map.ras.offset;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_component_to_ras, CXL);
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 044a92d9813e..df64c402e6e6 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -270,6 +270,9 @@ enum cxl_rcrb {
>> resource_size_t cxl_rcrb_to_component(struct device *dev,
>> resource_size_t rcrb,
>> enum cxl_rcrb which);
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
>> +u16 cxl_component_to_ras(struct device *dev,
>> + resource_size_t component_reg_phys);
>>
>> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>> #define CXL_TARGET_STRLEN 20
>> @@ -601,6 +604,8 @@ struct cxl_dport {
>> int port_id;
>> resource_size_t component_reg_phys;
>> resource_size_t rcrb;
>> + u16 aer_cap;
>> + u16 ras_cap;
>> bool rch;
>> struct cxl_port *port;
>> };
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 39c4b54f0715..014295ab6bc6 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -45,13 +45,36 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>> return 0;
>> }
>>
>> +static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
>> + struct cxl_dport *parent_dport)
>> +{
>> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
>> +
>> + if (!parent_dport->rch)
>> + return;
>> +
>> + /*
>> + * The component registers for an RCD might come from the
>> + * host-bridge RCRB if they are not already mapped via the
>> + * typical register locator mechanism.
>> + */
>> + if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
>> + cxlds->component_reg_phys = cxl_rcrb_to_component(
>> + &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
>> +
>> + parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
>> + parent_dport->rcrb);
>
> Hmm, how about just retrieve this as part of cxl_rcrb_to_component()
> (renamed to cxl_probe_rcrb()), and make an rch dport its own distinct
> object? Otherwise it feels odd to be retrieving downstream port
> properties this late at upstream port component register detection time.
> It also feels awkward to keep adding more RCH dport specific details to
> the common 'struct cxl_dport'. So, I'm thinking something like the
> following (compiled and cxl_test regression passed):
>
Thanks. I applied to this patchset's base and will include in our next
revision.
> -- >8 --
> From 18fbc72f98655d10301c7a35f614b6152f46c44b Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Mon, 17 Apr 2023 15:45:50 -0700
> Subject: [PATCH] cxl/rch: Prepare for caching the MMIO mapped PCIe AER
> capability
>
> Prepare cxl_probe_rcrb() for retrieving more than just the component
> register block. The RCH AER handling code wants to get back to the AER
> capability that happens to be MMIO mapped rather then configuration
> cycles.
>
> Move rcrb specific dport data, like the RCRB base and the AER capability
> offset, into its own data structure ('struct cxl_rcrb_info') for
> cxl_probe_rcrb() to fill. Introduce 'struct cxl_rch_dport' to wrap a
> 'struct cxl_dport' with a 'struct cxl_rcrb_info' attribute.
>
> This centralizes all RCRB scanning in one routine.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/cxl/acpi.c | 16 ++++++++--------
> drivers/cxl/core/port.c | 33 +++++++++++++++++++++------------
> drivers/cxl/core/regs.c | 12 ++++++++----
> drivers/cxl/cxl.h | 21 +++++++++++++++------
> drivers/cxl/mem.c | 15 ++++++++++-----
> tools/testing/cxl/Kbuild | 2 +-
> tools/testing/cxl/test/cxl.c | 10 ++++++----
> tools/testing/cxl/test/mock.c | 12 ++++++------
> tools/testing/cxl/test/mock.h | 7 ++++---
> 9 files changed, 79 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 4e66483f1fd3..2647eb04fcdb 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -375,7 +375,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> struct cxl_chbs_context {
> struct device *dev;
> unsigned long long uid;
> - resource_size_t rcrb;
> + struct cxl_rcrb_info rcrb;
> resource_size_t chbcr;
> u32 cxl_version;
> };
> @@ -395,7 +395,7 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> return 0;
>
> ctx->cxl_version = chbs->cxl_version;
> - ctx->rcrb = CXL_RESOURCE_NONE;
> + ctx->rcrb.base = CXL_RESOURCE_NONE;
> ctx->chbcr = CXL_RESOURCE_NONE;
>
> if (!chbs->base)
> @@ -409,9 +409,8 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> if (chbs->length != CXL_RCRB_SIZE)
> return 0;
>
> - ctx->rcrb = chbs->base;
> - ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
> - CXL_RCRB_DOWNSTREAM);
> + ctx->chbcr = cxl_probe_rcrb(ctx->dev, chbs->base, &ctx->rcrb,
> + CXL_RCRB_DOWNSTREAM);
>
> return 0;
> }
> @@ -451,8 +450,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> return 0;
> }
>
> - if (ctx.rcrb != CXL_RESOURCE_NONE)
> - dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> + if (ctx.rcrb.base != CXL_RESOURCE_NONE)
> + dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid,
> + &ctx.rcrb.base);
>
> if (ctx.chbcr == CXL_RESOURCE_NONE) {
> dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n",
> @@ -466,7 +466,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
> bridge = pci_root->bus->bridge;
> if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
> dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
> - ctx.chbcr, ctx.rcrb);
> + ctx.chbcr, &ctx.rcrb);
> else
> dport = devm_cxl_add_dport(root_port, bridge, uid,
> ctx.chbcr);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 4003f445320c..d194f48259ff 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -920,7 +920,7 @@ static void cxl_dport_unlink(void *data)
> static struct cxl_dport *
> __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> int port_id, resource_size_t component_reg_phys,
> - resource_size_t rcrb)
> + struct cxl_rcrb_info *ri)
> {
> char link_name[CXL_TARGET_STRLEN];
> struct cxl_dport *dport;
> @@ -942,17 +942,26 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> CXL_TARGET_STRLEN)
> return ERR_PTR(-EINVAL);
>
> - dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> - if (!dport)
> - return ERR_PTR(-ENOMEM);
> + if (ri && ri->base != CXL_RESOURCE_NONE) {
> + struct cxl_rch_dport *rdport;
> +
> + rdport = devm_kzalloc(host, sizeof(*rdport), GFP_KERNEL);
> + if (!rdport)
> + return ERR_PTR(-ENOMEM);
> +
> + rdport->rcrb.base = ri->base;
> + dport = &rdport->dport;
> + dport->rch = true;
> + } else {
> + dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> + if (!dport)
> + return ERR_PTR(-ENOMEM);
> + }
>
> dport->dport = dport_dev;
> dport->port_id = port_id;
> dport->component_reg_phys = component_reg_phys;
> dport->port = port;
> - if (rcrb != CXL_RESOURCE_NONE)
> - dport->rch = true;
> - dport->rcrb = rcrb;
>
> cond_cxl_root_lock(port);
> rc = add_dport(port, dport);
> @@ -994,7 +1003,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> struct cxl_dport *dport;
>
> dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> - component_reg_phys, CXL_RESOURCE_NONE);
> + component_reg_phys, NULL);
> if (IS_ERR(dport)) {
> dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
> dev_name(&port->dev), PTR_ERR(dport));
> @@ -1013,24 +1022,24 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
> * @dport_dev: firmware or PCI device representing the dport
> * @port_id: identifier for this dport in a decoder's target list
> * @component_reg_phys: optional location of CXL component registers
> - * @rcrb: mandatory location of a Root Complex Register Block
> + * @ri: mandatory data about the Root Complex Register Block layout
> *
> * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
> */
> struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> struct device *dport_dev, int port_id,
> resource_size_t component_reg_phys,
> - resource_size_t rcrb)
> + struct cxl_rcrb_info *ri)
> {
> struct cxl_dport *dport;
>
> - if (rcrb == CXL_RESOURCE_NONE) {
> + if (!ri || ri->base == CXL_RESOURCE_NONE) {
> dev_dbg(&port->dev, "failed to add RCH dport, missing RCRB\n");
> return ERR_PTR(-EINVAL);
> }
>
> dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> - component_reg_phys, rcrb);
> + component_reg_phys, ri);
> if (IS_ERR(dport)) {
> dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> dev_name(&port->dev), PTR_ERR(dport));
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 52d1dbeda527..b1c0db898a50 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -332,9 +332,8 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> }
> EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which)
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which)
> {
> resource_size_t component_reg_phys;
> void __iomem *addr;
> @@ -344,6 +343,8 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>
> if (which == CXL_RCRB_UPSTREAM)
> rcrb += SZ_4K;
> + else
> + ri->base = rcrb;
>
> /*
> * RCRB's BAR[0..1] point to component block containing CXL
> @@ -364,6 +365,9 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> cmd = readw(addr + PCI_COMMAND);
> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +
> + /* TODO: retrieve rcrb->aer_cap here */
> +
Ack
> iounmap(addr);
> release_mem_region(rcrb, SZ_4K);
>
> @@ -395,4 +399,4 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>
> return component_reg_phys;
> }
> -EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_probe_rcrb, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1503ccec9a84..b0807f54e9fd 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -267,9 +267,9 @@ enum cxl_rcrb {
> CXL_RCRB_DOWNSTREAM,
> CXL_RCRB_UPSTREAM,
> };
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which);
> +struct cxl_rcrb_info;
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which);
>
> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> #define CXL_TARGET_STRLEN 20
> @@ -589,12 +589,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
> return xa_load(&port->dports, (unsigned long)dport_dev);
> }
>
> +
> /**
> * struct cxl_dport - CXL downstream port
> * @dport: PCI bridge or firmware device representing the downstream link
> * @port_id: unique hardware identifier for dport in decoder target list
> * @component_reg_phys: downstream port component registers
> - * @rcrb: base address for the Root Complex Register Block
> * @rch: Indicate whether this dport was enumerated in RCH or VH mode
> * @port: reference to cxl_port that contains this downstream port
> */
> @@ -602,11 +602,20 @@ struct cxl_dport {
> struct device *dport;
> int port_id;
> resource_size_t component_reg_phys;
> - resource_size_t rcrb;
> bool rch;
> struct cxl_port *port;
> };
>
> +struct cxl_rcrb_info {
> + resource_size_t base;
> + u16 aer_cap;
> +};
> +
> +struct cxl_rch_dport {
> + struct cxl_dport dport;
> + struct cxl_rcrb_info rcrb;
> +};
> +
> /**
> * struct cxl_ep - track an endpoint's interest in a port
> * @ep: device that hosts a generic CXL endpoint (expander or accelerator)
> @@ -674,7 +683,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> struct device *dport_dev, int port_id,
> resource_size_t component_reg_phys,
> - resource_size_t rcrb);
> + struct cxl_rcrb_info *ri);
>
> struct cxl_decoder *to_cxl_decoder(struct device *dev);
> struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 097d86dd2a8e..7da6135e0b17 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -71,10 +71,15 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> * host-bridge RCRB if they are not already mapped via the
> * typical register locator mechanism.
> */
> - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> - component_reg_phys = cxl_rcrb_to_component(
> - &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> - else
> + if (parent_dport->rch &&
> + cxlds->component_reg_phys == CXL_RESOURCE_NONE) {
> + struct cxl_rch_dport *rdport =
> + container_of(parent_dport, typeof(*rdport), dport);
> +
> + component_reg_phys =
> + cxl_probe_rcrb(&cxlmd->dev, rdport->rcrb.base,
> + &rdport->rcrb, CXL_RCRB_UPSTREAM);
> + } else
> component_reg_phys = cxlds->component_reg_phys;
> endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
> parent_dport);
> @@ -92,7 +97,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> }
>
> return 0;
> -}
> + }
>
> static int cxl_mem_probe(struct device *dev)
> {
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index fba7bec96acd..bef1bc3bd912 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -11,7 +11,7 @@ ldflags-y += --wrap=devm_cxl_enumerate_decoders
> ldflags-y += --wrap=cxl_await_media_ready
> ldflags-y += --wrap=cxl_hdm_decode_init
> ldflags-y += --wrap=cxl_dvsec_rr_decode
> -ldflags-y += --wrap=cxl_rcrb_to_component
> +ldflags-y += --wrap=cxl_probe_rcrb
>
> DRIVERS := ../../../drivers
> CXL_SRC := $(DRIVERS)/cxl
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 385cdeeab22c..805c79491485 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -983,12 +983,14 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
> return 0;
> }
>
> -resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which)
> +resource_size_t mock_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which)
> {
> dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
>
> + if (which == CXL_RCRB_DOWNSTREAM)
> + ri->base = rcrb;
> +
> return (resource_size_t) which + 1;
> }
>
> @@ -1000,7 +1002,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
> .is_mock_dev = is_mock_dev,
> .acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
> .acpi_evaluate_integer = mock_acpi_evaluate_integer,
> - .cxl_rcrb_to_component = mock_cxl_rcrb_to_component,
> + .cxl_probe_rcrb = mock_cxl_probe_rcrb,
> .acpi_pci_find_root = mock_acpi_pci_find_root,
> .devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
> .devm_cxl_setup_hdm = mock_cxl_setup_hdm,
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index c4e53f22e421..148bd4f184f5 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -244,9 +244,9 @@ int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
> }
> EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_rr_decode, CXL);
>
> -resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which)
> +resource_size_t __wrap_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri,
> + enum cxl_rcrb which)
> {
> int index;
> resource_size_t component_reg_phys;
> @@ -254,14 +254,14 @@ resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
>
> if (ops && ops->is_mock_port(dev))
> component_reg_phys =
> - ops->cxl_rcrb_to_component(dev, rcrb, which);
> + ops->cxl_probe_rcrb(dev, rcrb, ri, which);
> else
> - component_reg_phys = cxl_rcrb_to_component(dev, rcrb, which);
> + component_reg_phys = cxl_probe_rcrb(dev, rcrb, ri, which);
> put_cxl_mock_ops(index);
>
> return component_reg_phys;
> }
> -EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(__wrap_cxl_probe_rcrb, CXL);
>
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(ACPI);
> diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
> index bef8817b01f2..7ef21356d052 100644
> --- a/tools/testing/cxl/test/mock.h
> +++ b/tools/testing/cxl/test/mock.h
> @@ -15,9 +15,10 @@ struct cxl_mock_ops {
> acpi_string pathname,
> struct acpi_object_list *arguments,
> unsigned long long *data);
> - resource_size_t (*cxl_rcrb_to_component)(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which);
> + resource_size_t (*cxl_probe_rcrb)(struct device *dev,
> + resource_size_t rcrb,
> + struct cxl_rcrb_info *ri,
> + enum cxl_rcrb which);
> struct acpi_pci_root *(*acpi_pci_find_root)(acpi_handle handle);
> bool (*is_mock_bus)(struct pci_bus *bus);
> bool (*is_mock_port)(struct device *dev);
My email client chopped off the following:
--
2.39.2
-- >8 --
> +
> + parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
> + parent_dport->component_reg_phys);
[Dan] Since this is component register offset based can it not be shared with
the VH case? I have been expecting that RCH RAS capability and VH RAS
capability scanning would need to be unified in the cxl_port driver.
[Terry] The same probe function is called indirectly:
cxl_component_to_ras()
cxl_probe_component_regs() <<==
The VH has:
cxl_setup_regs() (cxl/pci.c)
cxl_probe_regs() (cxl/pci.c)
cxl_probe_component_regs() (cxl/core/regs.c) <<==
Would you like to see the RCH RAS discovery reuse cxl_probe_regs() or (parts of)
cxl_setup_regs() as well? I ask because cxl_probe_regs() and cxl_setup_regs()
are static in cxl/pci.c. Of course they can be moved out to cxl/core/regs.c to avoid
an unwanted dependancy from exported pci.c symbol. This would be a significant
change just for the RCH case.
You mentioned earlier it is late to add downstream port properties in mem device
initialization. Where would you prefer the downstream RAS is discovered and mapped?
I understand it needs to reuse as much existing code as possible but where to call from?
The optins I see are here in mem.c or in __devm_cxl_add_dport() but neither are ideal.
© 2016 - 2026 Red Hat, Inc.