Re: [PATCH v3 1/6] cxl/pci: Add RCH downstream port AER and RAS register discovery

Terry Bowman
2023-04-13T19:13:29Z

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);
>