Re: [PATCH v10 07/15] cxl/pci: Add RCH downstream port AER register discovery

[Terry Bowman]

Hi Dan,

Thanks for reviewing. I added response below.

On 8/31/23 13:51, 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 the error logging
>> is the AER and RAS registers are not accessible. The CXL driver requires
>> changes to find RCH downstream port AER and RAS 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 and RAS registers 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. Use 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@xxxxxxx>
>> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>> ---
>>  drivers/cxl/core/core.h |  1 +
>>  drivers/cxl/core/port.c |  6 ++++++
>>  drivers/cxl/core/regs.c | 35 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 45e7e044cf4a..f470ef5c0a6a 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -73,6 +73,7 @@ struct cxl_rcrb_info;
>>  resource_size_t __rcrb_to_component(struct device *dev,
>>  				    struct cxl_rcrb_info *ri,
>>  				    enum cxl_rcrb which);
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
>>  
>>  extern struct rw_semaphore cxl_dpa_rwsem;
>>  
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 9151ec5b879b..da4f1b303d6c 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -979,6 +979,8 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  		return ERR_PTR(-ENOMEM);
>>  
>>  	if (rcrb != CXL_RESOURCE_NONE) {
>> +		struct pci_host_bridge *host_bridge;
>> +
>>  		dport->rcrb.base = rcrb;
>>  		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>>  							 CXL_RCRB_DOWNSTREAM);
>> @@ -987,6 +989,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  			return ERR_PTR(-ENXIO);
>>  		}
>>  
>> +		host_bridge = to_pci_host_bridge(dport_dev);
>> +		if (host_bridge->native_cxl_error)
>> +			dport->rcrb.aer_cap = cxl_rcrb_to_aer(dport_dev, dport->rcrb.base);
>> +
> 
> Minor comment...
> 
> Is there a need to gate the discovery of the registers on the ACPI
> setting? For example cxl_pci unconditionally enumerates the RAS
> component register block but gates *using* them by ->native_cxl_error.

This is to prevent RCH AER and RAS accesses in CXL device driver if CXL error handling was not 
OSC enabled. Would you like this check moved to immediately before the logging?

Regards,
Terry