Re: [RFC PATCH 03/25] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 16 Nov 2020 11:19:36 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 20-11-16 12:03:52, Jonathan Cameron wrote:
> > On Tue, 10 Nov 2020 21:47:02 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >   
> > > A CXL 2.0 component is any entity in the CXL topology. All components
> > > have a analogous function in PCIe. Except for the CXL host bridge, all
> > > have a PCIe config space that is accessible via the common PCIe
> > > mechanisms. CXL components are enumerated via DVSEC fields in the
> > > extended PCIe header space. CXL components will minimally implement some
> > > subset of CXL.mem and CXL.cache registers defined in 8.2.5 of the CXL
> > > 2.0 specification. Two headers and a utility library are introduced to
> > > support the minimum functionality needed to enumerate components.
> > > 
> > > The cxl_pci header manages bits associated with PCI, specifically the
> > > DVSEC and related fields. The cxl_component.h variant has data
> > > structures and APIs that are useful for drivers implementing any of the
> > > CXL 2.0 components. The library takes care of making use of the DVSEC
> > > bits and the CXL.[mem|cache] regisetrs.
> > > 
> > > None of the mechanisms required to enumerate a CXL capable hostbridge
> > > are introduced at this point.
> > > 
> > > Note that the CXL.mem and CXL.cache registers used are always 4B wide.
> > > It's possible in the future that this constraint will not hold.
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > 
> > > --
> > > It's tempting to have a more generalized DVSEC infrastructure. As far as
> > > I can tell, the amount this would actually save in terms of code is
> > > minimal because most of DVESC is vendor specific.  
> > 
> > Agreed.  Probably not worth bothering with generic infrastructure for 2.5 DW.
> > 
> > A few comments inline.
> > 
> > Jonathan
> >   
> 
> Anything I didn't respond to is accepted and will be in v2.
> 
> Thanks.
> Ben
> 
Hi Ben,

...

> >   
> > > +
> > > +/* 8.2.5.10 - CXL Security Capability Structure */
> > > +#define CXL_SEC_REGISTERS_OFFSET (CXL_RAS_REGISTERS_OFFSET + CXL_RAS_REGISTERS_SIZE)
> > > +#define CXL_SEC_REGISTERS_SIZE   0 /* We don't implement 1.1 downstream ports */
> > > +
> > > +/* 8.2.5.11 - CXL Link Capability Structure */
> > > +#define CXL_LINK_REGISTERS_OFFSET (CXL_SEC_REGISTERS_OFFSET + CXL_SEC_REGISTERS_SIZE)
> > > +#define CXL_LINK_REGISTERS_SIZE   0x38  
> > 
> > Bit odd to introduce this but not define anything... Can't we bring these
> > in when we need them later?  
> 
> Repeating my comment from 00/25...
> 
> For this specific patch series I liked providing #defines in bulk so that it's
> easy enough to just bring up the spec and review them. Not sure if there is a
> preference from others in the community on this.

Personally I'd prefer to see the lot if you are going to do that, on basis
should only need reviewing against the spec once.
Not sure others will agree though as "the lot" is an awful lot.

> 
> I could also introduce the library that generates the capability headers with
> this. Either is fine, I just wanted to point out the intent.
> 
> >   
> > > +
> > > +/* 8.2.5.12 - CXL HDM Decoder Capability Structure */
> > > +#define HDM_DECODE_MAX 10 /* 8.2.5.12.1 */
> > > +#define CXL_HDM_REGISTERS_OFFSET \
> > > +    (CXL_LINK_REGISTERS_OFFSET + CXL_LINK_REGISTERS_SIZE) /* 8.2.5.12 */
> > > +#define CXL_HDM_REGISTERS_SIZE (0x20 + HDM_DECODE_MAX * 10)
> > > +#define HDM_DECODER_INIT(n, base)                          \
> > > +    REG32(CXL_HDM_DECODER##n##_BASE_LO, base + 0x10)       \  
> > 
> > Offset n should be included in the address calc.  It's always 0 at the moment
> > but might as well put it in now.  Mind you there is something a bit odd
> > in the spec I'm looking at. Nothing defined at 0x2c but no reserved line
> > either in the table.  
> 
> My guess is some earlier version of the spec had the decoder registers as 64b
> and so they wanted to keep natural alignment.

Agreed, but having a hole in the spec isn't great.  Looks like a reserved
field should be inserted.

> 
> > 
...

> >   
> > > +#define PCIE_DVSEC_ID_OFFSET     0x8
> > > +
> > > +#define PCIE_CXL_DEVICE_DVSEC_LENGTH 0x38
> > > +#define PCIE_CXL_DEVICE_DVSEC_REVID  1  
> > 
> > Make it clear this is the CXL 2.0 revid.
> > It would be 0 for CXL 1.1 I think? (8.1.3 of CXL 2.0 spec)  
> 
> Got it. BTW, you're correct. It is in the verbiage there
> "DVSEC Revision ID of 0h represents the structure as defined in CXL 1.1 specification."
> 
> A bit hidden IMO.

Yes, it's 'fun' finding some stuff in that doc, though most things you are looking
for turn out to be somewhere at least.

> 
> > 
> >   
> > > +
> > > +#define EXTENSIONS_PORT_DVSEC_LENGTH 0x28
> > > +#define EXTENSIONS_PORT_DVSEC_REVID  1  
> > 
> > I'm assuming this is the CXL 2.0 exensions DVSEC for ports
> > in figure 128?
> > 
> > If so table 128 has dvsec revision as 0. 
> >   
> 
> Good catch, btw a shortcut is to look at Table 124.

Good point - I'd missed the revision column in that :)

> 
> > > +
> > > +#define GPF_PORT_DVSEC_LENGTH 0x10
> > > +#define GPF_PORT_DVSEC_REVID  0
> > > +
> > > +#define PCIE_FLEXBUS_PORT_DVSEC_LENGTH_2_0 0x14
> > > +#define PCIE_FLEXBUS_PORT_DVSEC_REVID_2_0  1
> > > +
> > > +#define REG_LOC_DVSEC_LENGTH 0x24
> > > +#define REG_LOC_DVSEC_REVID  0  
> > 
> > Whilst I appreciate this is an RFC it would seem more logical
> > to me to only list things in the following enum if we
> > have also defined them here.  E.g. GPF_DEVICE_DVSEC doesn't
> > have length and revid defines.
> >   
> > > +
> > > +enum {
> > > +    PCIE_CXL_DEVICE_DVSEC      = 0,
> > > +    NON_CXL_FUNCTION_MAP_DVSEC = 2,
> > > +    EXTENSIONS_PORT_DVSEC      = 3,
> > > +    GPF_PORT_DVSEC             = 4,
> > > +    GPF_DEVICE_DVSEC           = 5,
> > > +    PCIE_FLEXBUS_PORT_DVSEC    = 7,
> > > +    REG_LOC_DVSEC              = 8,
> > > +    MLD_DVSEC                  = 9,
> > > +    CXL20_MAX_DVSEC
> > > +};
> > > +
> > > +struct dvsec_header {
> > > +    uint32_t cap_hdr;
> > > +    uint32_t dv_hdr1;
> > > +    uint16_t dv_hdr2;
> > > +} __attribute__((__packed__));
> > > +_Static_assert(sizeof(struct dvsec_header) == 10,
> > > +               "dvsec header size incorrect");
> > > +
> > > +/*
> > > + * CXL 2.0 devices must implement certain DVSEC IDs, and can [optionally]
> > > + * implement others.
> > > + *
> > > + * CXL 2.0 Device: 0, [2], 5, 8
> > > + * CXL 2.0 RP: 3, 4, 7, 8
> > > + * CXL 2.0 Upstream Port: [2], 7, 8
> > > + * CXL 2.0 Downstream Port: 3, 4, 7, 8
> > > + */
> > > +
> > > +/* CXL 2.0 - 8.1.5 (ID 0003) */
> > > +struct dvsec_port {  
> > 
> > I'd keep naming consistent.  It's called EXTENSIONS_PORT_DVSEC above
> > so extensions_dvsec_port here.
> >   
> > > +    struct dvsec_header hdr;
> > > +    uint16_t status;
> > > +    uint16_t control;
> > > +    uint8_t alt_bus_base;
> > > +    uint8_t alt_bus_limit;
> > > +    uint16_t alt_memory_base;
> > > +    uint16_t alt_memory_limit;
> > > +    uint16_t alt_prefetch_base;
> > > +    uint16_t alt_prefetch_limit;
> > > +    uint32_t alt_prefetch_base_high;
> > > +    uint32_t alt_prefetch_base_low;
> > > +    uint32_t rcrb_base;
> > > +    uint32_t rcrb_base_high;
> > > +};
> > > +_Static_assert(sizeof(struct dvsec_port) == 0x28, "dvsec port size incorrect");
> > > +#define PORT_CONTROL_OVERRIDE_OFFSET 0xc  
> > I'm not totally sure what this define is, but seems
> > like it's simply the offset of the control field above.
> > If so can't we get it from the there directly?  
> 
> Firstly, I only define these to show how one would handle DVSEC writes. I don't
> actually have a use for them as of now. It is just the offset, but I don't know
> what you mean by getting it from there directly. Could you elaborate a bit?

As you have a packed representation you should be able to do some
address arthmetic to get it.  offsetof(dvsec_port, control) I think....

Thanks,

Jonathan