On 30.11.22 12:39:00, Dan Williams wrote:
> Robert Richter wrote:
> > On 28.11.22 16:25:18, Dan Williams wrote:
> > > Robert Richter wrote:
> > > > On 24.11.22 10:35:32, Dan Williams wrote:
> > > > > Unlike a CXL memory expander in a VH topology that has at least one
> > > > > intervening 'struct cxl_port' instance between itself and the CXL root
> > > > > device, an RCD attaches one-level higher. For example:
> > > > >
> > > > > VH
> > > > > ┌──────────┐
> > > > > │ ACPI0017 │
> > > > > │ root0 │
> > > > > └─────┬────┘
> > > > > │
> > > > > ┌─────┴────┐
> > > > > │ dport0 │
> > > > > ┌─────┤ ACPI0016 ├─────┐
> > > > > │ │ port1 │ │
> > > > > │ └────┬─────┘ │
> > > > > │ │ │
> > > > > ┌──┴───┐ ┌──┴───┐ ┌───┴──┐
> > > > > │dport0│ │dport1│ │dport2│
> > > > > │ RP0 │ │ RP1 │ │ RP2 │
> > > > > └──────┘ └──┬───┘ └──────┘
> > > > > │
> > > > > ┌───┴─────┐
> > > > > │endpoint0│
> > > > > │ port2 │
> > > > > └─────────┘
> > > > >
> > > > > ...vs:
> > > > >
> > > > > RCH
> > > > > ┌──────────┐
> > > > > │ ACPI0017 │
> > > > > │ root0 │
> > > > > └────┬─────┘
> > > > > │
> > > > > ┌───┴────┐
> > > > > │ dport0 │
> > > > > │ACPI0016│
> > > > > └───┬────┘
> > > > > │
> > > > > ┌────┴─────┐
> > > > > │endpoint0 │
> > > > > │ port1 │
> > > > > └──────────┘
> > > > >
> > > > > So arrange for endpoint port in the RCH/RCD case to appear directly
> > > > > connected to the host-bridge in its singular role as a dport. Compare
> > > > > that to the VH case where the host-bridge serves a dual role as a
> > > > > 'cxl_dport' for the CXL root device *and* a 'cxl_port' upstream port
> > > > > for
> > > > > the Root Ports in the Root Complex that are modeled as 'cxl_dport'
> > > > > instances in the CXL topology.
> > > > >
> > > > > Another deviation from the VH case is that RCDs may need to look up
> > > > > their component registers from the Root Complex Register Block (RCRB).
> > > > > That platform firmware specified RCRB area is cached by the cxl_acpi
> > > > > driver and conveyed via the host-bridge dport to the cxl_mem driver to
> > > > > perform the cxl_rcrb_to_component() lookup for the endpoint port
> > > > > (See 9.11.8 CXL Devices Attached to an RCH for the lookup of the
> > > > > upstream port component registers).
> > > > >
> > > > > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> > > > > ---
> > > > > drivers/cxl/core/port.c | 11 +++++++++--
> > > > > drivers/cxl/cxlmem.h | 2 ++
> > > > > drivers/cxl/mem.c | 31 ++++++++++++++++++++++++-------
> > > > > drivers/cxl/pci.c | 10 ++++++++++
> > > > > 4 files changed, 45 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > > > > index c7f58282b2c1..2385ee00eb9a 100644
> > > > > --- a/drivers/cxl/core/port.c
> > > > > +++ b/drivers/cxl/core/port.c
> > > > > @@ -1358,8 +1358,17 @@ int devm_cxl_enumerate_ports(struct cxl_memdev
> > > > > *cxlmd)
> > > > > {
> > > > > struct device *dev = &cxlmd->dev;
> > > > > struct device *iter;
> > > > > + struct cxl_dport *dport;
> > > > > + struct cxl_port *port;
> > > >
> > > > There is no direct need to move that code here.
> > > >
> > > > If you want to clean that up in this patch too, then leave a comment
> > > > in the change log?
> > >
> > > Oh, good point, must have been left over from an earlier revision of the
> > > patch, dropped it.
> > >
> > > >
> > > > > int rc;
> > > > >
> > > > > + /*
> > > > > + * Skip intermediate port enumeration in the RCH case, there
> > > > > + * are no ports in between a host bridge and an endpoint.
> > > > > + */
> > > > > + if (cxlmd->cxlds->rcd)
> > > > > + return 0;
> > > > > +
> > > > > rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep,
> > > > > cxlmd);
> > > > > if (rc)
> > > > > return rc;
> > > > > @@ -1373,8 +1382,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev
> > > > > *cxlmd)
> > > > > for (iter = dev; iter; iter = grandparent(iter)) {
> > > > > struct device *dport_dev = grandparent(iter);
> > > > > struct device *uport_dev;
> > > > > - struct cxl_dport *dport;
> > > > > - struct cxl_port *port;
> > > > >
> > > > > if (!dport_dev)
> > > > > return 0;
> > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > > > index e082991bc58c..35d485d041f0 100644
> > > > > --- a/drivers/cxl/cxlmem.h
> > > > > +++ b/drivers/cxl/cxlmem.h
> > > > > @@ -201,6 +201,7 @@ struct cxl_endpoint_dvsec_info {
> > > > > * @dev: The device associated with this CXL state
> > > > > * @regs: Parsed register blocks
> > > > > * @cxl_dvsec: Offset to the PCIe device DVSEC
> > > > > + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached
> > > > > to an RCH)
> > > > > * @payload_size: Size of space for payload
> > > > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register)
> > > > > * @lsa_size: Size of Label Storage Area
> > > > > @@ -235,6 +236,7 @@ struct cxl_dev_state {
> > > > > struct cxl_regs regs;
> > > > > int cxl_dvsec;
> > > > >
> > > > > + bool rcd;
> > > > > size_t payload_size;
> > > > > size_t lsa_size;
> > > > > struct mutex mbox_mutex; /* Protects device mailbox and
> > > > > firmware */
> > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > > > index aa63ce8c7ca6..9a655b4b5e52 100644
> > > > > --- a/drivers/cxl/mem.c
> > > > > +++ b/drivers/cxl/mem.c
> > > > > @@ -45,12 +45,13 @@ static int cxl_mem_dpa_show(struct seq_file
> > > > > *file, void *data)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static int devm_cxl_add_endpoint(struct cxl_memdev *cxlmd,
> > > > > +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;
> > > > >
> > > > > /*
> > > > > @@ -65,8 +66,18 @@ static int devm_cxl_add_endpoint(struct cxl_memdev
> > > > > *cxlmd,
> > > > > ep->next = down;
> > > > > }
> > > > >
> > > > > - endpoint = devm_cxl_add_port(&parent_port->dev, &cxlmd->dev,
> > > > > - cxlds->component_reg_phys,
> > > > > parent_dport);
> > > > > + /*
> > > > > + * 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_DOWNSTREAM);
> > > >
> > > > As already commented: this must be the upstream RCRB here.
> > > >
> > > > > + else
> > > > > + component_reg_phys = cxlds->component_reg_phys;
> > > > > + endpoint = devm_cxl_add_port(host, &cxlmd->dev,
> > > > > component_reg_phys,
> > > > > + parent_dport);
> > > >
> > > > Looking at CXL 3.0 spec, table 8-22, there are the various sources of
> > > > component registers listed. For RCD we need: D1, DP1, UP1 (optional
> > > > R).
> > > >
> > > > D1: endpoint->component_reg_phys;
> > > > UP1: parent_port-component_reg_phys; (missing in RCH
> > > > topology)
> > > > DP1: parent_dport->component_reg_phys;
> > > >
> > > > I don't see how all of them could be stored in this data layout as the
> > > > cxl host port is missing.
> > >
> > > If I am understanding your concern correctly, that's handled here:
> > >
> > > if (parent_dport->rch && cxlds->component_reg_phys ==
> > > CXL_RESOURCE_NONE)
> > >
> > > In the D1 case cxlds->component_reg_phys will be valid since the
> > > component registers were visible via the register locator DVSEC and
> > > retrieved by cxl_pci. In the UP1 case cxlds->component_reg_phys is
> > > invalid and the driver falls back to the RCRB. DP1 is handled in
> > > cxl_acpi. I.e. the D1 and UP1 cases do not co-exist.
> >
> > What I mean is we must store all 3 component reg base addresses for
> > later access. E.g., if there is an AER error of a pci dev or the host,
> > we must (depending on the error details) access CXL RAS status of
> > either D1, UP1 or DP1. So for all 3 of them there must be a way to
> > determine this walking through the port hierarchy. In the above list
> > of locations I don't where UP1's component reg base address is stored.
>
> So I think we are reading the specification differently. I am comparing
> Figure 9-7 "CXL Device Remaps Upstream Port and Component Registers" and
> Figure Figure 9-8 "CXL Device that Does Not Remap Upstream Port and
> Component Registers" and noting that there is never a case where three
> sets of component registers are visible. It is either DP1 connected to
> UP1 (Figure 9-7) or DP1 connected to D1 (Figure 9-8). There is never a
> case where the code needs to consider UP1 and D1 component registers at
> the same time because those things are identical just enumerated
> differently depending on how the endpoint is implemented.
Yes, the spec is ambiguous here. Looking at CXL 3.0, Figure 12-3 I
also tend to agree the RCiEP's CXL RAS cap is used for that ("UP Z
sends an error message to all CXL.io Functions that are affected by
this error."). Unfortunately 12.2.1.2 does not state where the CXL RAS
Capability that logs the error resides.
-Robert
