Re: [PATCH 0/4 v3] Multi-Region and Volatile Memory support for CXL Type-3 Devices
On Thu, Oct 27, 2022 at 11:58:54AM +0100, Jonathan Cameron wrote: > On Wed, 26 Oct 2022 16:47:18 -0400 > Gregory Price wrote: > > > On Wed, Oct 26, 2022 at 08:13:24PM +, Adam Manzanares wrote: > > > On Tue, Oct 25, 2022 at 08:47:33PM -0400, Gregory Price wrote: > > > > Submitted as an extention to the multi-feature branch maintained > > > > by Jonathan Cameron at: > > > > https://urldefense.com/v3/__https://gitlab.com/jic23/qemu/-/tree/cxl-2022-10-24__;!!EwVzqGoTKBqv-0DWAJBm!RyiGL5B1XmQnVFwgxikKJeosPMKtoO1cTr61gIq8fwqfju8l4cbGZGwAEkKXIJB-Dbkfi_LNN2rGCbzMISz65cTxpAxI9pQ$ > > > > > > > > > > > > > > > > Summary of Changes: > > > > 1) E820 CFMW Bug fix. > > > > 2) Add CXL_CAPACITY_MULTIPLIER definition to replace magic numbers > > > > 3) Multi-Region and Volatile Memory support for CXL Type-3 Devices > > > > 4) CXL Type-3 SRAT Generation when NUMA node is attached to memdev > > +CC Dan for a question on status of Generic Ports ACPI code first ECN. > Given that was done on the mailing list I can find any public tracking > of whether it was accepted or not - hence whether we can get on with > implementation. There hasn't been a release ACPI spec since before > that was proposed so we need that confirmation of the code first proposal > being accepted to get things moving. > > > > > > > > > > > > > Regarding the E820 fix > > > > * This bugfix is required for memory regions to work on x86 > > > > * input from Dan Williams and others suggest that E820 entry for > > > > the CFMW should not exist, as it is expected to be dynamically > > > > assigned at runtime. If this entry exists, it instead blocks > > > > region creation by nature of the memory region being marked as > > > > reserved. > > > > > > For CXL 2.0 it is my understanding that volatile capacity present at boot > > > will > > > be advertised by the firmware. In the absence of EFI I would assume this > > > would > > > be provided in the e820 map. > > Whilst this is one option, it is certainly not the case that all CXL 2.0 > platforms will decide to do any setup of CXL memory (volatile or not) in the > firmware. They can leave it entirely to the OS, so a cold plug flow. > Early platforms will do the setup in BIOS to support unware OSes, once > that's not a problem any more the only reason you'd want to do this is if > you don't have other RAM to boot the system, or for some reason you want > your host kernel etc in the CXL attached memory. > > I'd expect to see BIOS having OS managed configuration as an option in the > intermediate period where some OSes are aware, others not. > OS knows more about usecase / policy so can make better choices on > interleaving > etc of volatile CXL type 3 memory (let alone the fun corner of devices > where you can dynamically change the mix of volatile and non volatile > memory). > > > > > > The issue in this case is very explicitly that a double-mapping occurs > > for the same region. An E820 mapping for RESERVED is set *and* the > > region driver allocates a CXL CFMW mapping. As a result the region > > drive straight up fails to allocate regions. > > > > So in either case - at least from my view - the entry added as RESERVED > > is just wrong. > > > > This is separate from type-3 device SRAT entries and default mappings > > for volatile regions. For this situation, if you explicitly assign the > > memdev backing a type-3 device to a numa node, then an SRAT area is > > generated and an explicit e820 entry is generated and marked usable - > > though I think there are likely issues with this kind of > > double-referencing. > > SRAT setup for CXL type 3 devices is to my mind is a job for a full BIOS, > not QEMU level of faking a few things. That BIOS should also > be doing the full configuration (HDM Decoders and all the rest). ARM has > a prototype for one of the fixed virtual platforms that does this (talk > at Plumbers Uconf), we should look to do the same if we want to test > those flows using QEMU via appropriate changes in EDK2 to walk topology > and configure everything. Until the devices are configured there is no > way to configure the SLIT, HMAT entries that align with the SRAT ones > (in theory those can be updated at runtime via _SLI, _HMA but in > practice, I'm fairly sure Linux doesn't support doing that.) > > > > > > > > > > Is the region driver meant to cover volatile capacity present at boot? I > &
Re: [PATCH 0/4 v3] Multi-Region and Volatile Memory support for CXL Type-3 Devices
On Tue, Oct 25, 2022 at 08:47:33PM -0400, Gregory Price wrote: > Submitted as an extention to the multi-feature branch maintained > by Jonathan Cameron at: > https://urldefense.com/v3/__https://gitlab.com/jic23/qemu/-/tree/cxl-2022-10-24__;!!EwVzqGoTKBqv-0DWAJBm!RyiGL5B1XmQnVFwgxikKJeosPMKtoO1cTr61gIq8fwqfju8l4cbGZGwAEkKXIJB-Dbkfi_LNN2rGCbzMISz65cTxpAxI9pQ$ > > > > Summary of Changes: > 1) E820 CFMW Bug fix. > 2) Add CXL_CAPACITY_MULTIPLIER definition to replace magic numbers > 3) Multi-Region and Volatile Memory support for CXL Type-3 Devices > 4) CXL Type-3 SRAT Generation when NUMA node is attached to memdev > > > Regarding the E820 fix > * This bugfix is required for memory regions to work on x86 > * input from Dan Williams and others suggest that E820 entry for > the CFMW should not exist, as it is expected to be dynamically > assigned at runtime. If this entry exists, it instead blocks > region creation by nature of the memory region being marked as > reserved. For CXL 2.0 it is my understanding that volatile capacity present at boot will be advertised by the firmware. In the absence of EFI I would assume this would be provided in the e820 map. Is the region driver meant to cover volatile capacity present at boot? I was under the impression that it would be used for hot added volatile memory. It would be good to cover all of these assumptions for the e820 fix. Lastly it is my understanding that the region driver does not have support for volatile memory. It would be great to add that functionality if we make this change in QEMU. > > Regarding Multi-Region and Volatile Memory > * Developed with input from Jonathan Cameron and Davidlohr Bueso. > > Regarding SRAT Generation for Type-3 Devices > * Co-Developed by Davidlohr Bueso. Built from his base patch and > extended to work with both volatile and persistent regions. > * This can be used to demonstrate static type-3 device mapping and > testing numa-access to type-3 device memory regions. > > > This series brings 3 features to CXL Type-3 Devices: > 1) Volatile Memory Region support > 2) Multi-Region support (1 Volatile, 1 Persistent) > 3) (optional) SRAT Entry generation for type-3 device regions > > In this series we implement multi-region and volatile region support > through 7 major changes to CXL devices > 1) The HostMemoryBackend [hostmem] has been replaced by two >[hostvmem] and [hostpmem] to store volatile and persistent >memory respectively > 2) The single AddressSpace has been replaced by two AddressSpaces >[hostvmem_as] and [hostpmem_as] to map respective memdevs. > 3) Each memory region size and total region are stored separately > 4) The CDAT and DVSEC memory map entries have been updated: >a) if vmem is present, vmem is mapped at DPA(0) >b) if pmem is present > i) and vmem is present, pmem is mapped at DPA(vmem->size) > ii) else, pmem is mapped at DPA(0) >c) partitioning of pmem is not supported in this patch set but > has been discussed and this design should suffice. > 5) Read/Write functions have been updated to access AddressSpaces >according to the mapping described in #4 > 6) cxl-mailbox has been updated to report the respective size of >volatile and persistent memory regions > 7) SRAT entries may optionally be generated by manually assigning >memdevs to a cpuless numa node > > To support the Device Physical Address (DPA) Mapping decisions, see > CXL Spec (3.0) Section 8.2.9.8.2.0 - Get Partition Info: > Active Volatile Memory > The device shall provide this volatile capacity starting at DPA 0 > Active Persistent Memory > The device shall provide this persistent capacity starting at the > DPA immediately following the volatile capacity > > Partitioning of Persistent Memory regions may be supported on > following patch sets. > > > Gregory Price (4): > hw/i386/pc.c: CXL Fixed Memory Window should not reserve e820 in bios > hw/cxl: Add CXL_CAPACITY_MULTIPLIER definition > hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent) > hw/acpi/cxl.c: Fill in SRAT for vmem/pmem if NUMA node is assigned > > docs/system/devices/cxl.rst | 74 -- > hw/acpi/cxl.c | 67 + > hw/cxl/cxl-mailbox-utils.c | 23 +-- > hw/i386/acpi-build.c| 4 + > hw/i386/pc.c| 2 - > hw/mem/cxl_type3.c | 274 +++- > include/hw/acpi/cxl.h | 1 + > include/hw/cxl/cxl_device.h | 11 +- > tests/qtest/cxl-test.c | 111 +++ > 9 files changed, 443 insertions(+), 124 deletions(-) > > -- > 2.37.3 >
Re: [PATCH v8 04/46] hw/cxl/device: Introduce a CXL device (8.2.8)
On Fri, Apr 01, 2022 at 02:30:34PM +0100, Jonathan Cameron wrote: > On Thu, 31 Mar 2022 22:13:20 + > Adam Manzanares wrote: > > > On Wed, Mar 30, 2022 at 06:48:48PM +0100, Jonathan Cameron wrote: > > > On Tue, 29 Mar 2022 18:13:59 +0000 > > > Adam Manzanares wrote: > > > > > > > On Fri, Mar 18, 2022 at 03:05:53PM +, Jonathan Cameron wrote: > > > > > From: Ben Widawsky > > > > > > > > > > A CXL device is a type of CXL component. Conceptually, a CXL device > > > > > would be a leaf node in a CXL topology. From an emulation perspective, > > > > > CXL devices are the most complex and so the actual implementation is > > > > > reserved for discrete commits. > > > > > > > > > > This new device type is specifically catered towards the eventual > > > > > implementation of a Type3 CXL.mem device, 8.2.8.5 in the CXL 2.0 > > > > > specification. > > > > > > > > > > Signed-off-by: Ben Widawsky > > > > > Signed-off-by: Jonathan Cameron > > > > > Reviewed-by: Alex Bennée > > > > > > ... > > > > > > > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > > > > new file mode 100644 > > > > > index 00..b2416e45bf > > > > > --- /dev/null > > > > > +++ b/include/hw/cxl/cxl_device.h > > > > > @@ -0,0 +1,165 @@ > > > > > +/* > > > > > + * QEMU CXL Devices > > > > > + * > > > > > + * Copyright (c) 2020 Intel > > > > > + * > > > > > + * This work is licensed under the terms of the GNU GPL, version 2. > > > > > See the > > > > > + * COPYING file in the top-level directory. > > > > > + */ > > > > > + > > > > > +#ifndef CXL_DEVICE_H > > > > > +#define CXL_DEVICE_H > > > > > + > > > > > +#include "hw/register.h" > > > > > + > > > > > +/* > > > > > + * The following is how a CXL device's MMIO space is laid out. The > > > > > only > > > > > + * requirement from the spec is that the capabilities array and the > > > > > capability > > > > > + * headers start at offset 0 and are contiguously packed. The > > > > > headers themselves > > > > > + * provide offsets to the register fields. For this emulation, > > > > > registers will > > > > > + * start at offset 0x80 (m == 0x80). No secondary mailbox is > > > > > implemented which > > > > > + * means that n = m + sizeof(mailbox registers) + sizeof(device > > > > > registers). > > > > > > > > What is n here, the start offset of the mailbox registers, this > > > > question is > > > > based on the figure below? > > > > > > I'll expand on this to say > > > > > > means that the offset of the start of the mailbox payload (n) is given by > > > n = m + sizeof > > > > > > Which means the diagram below is wrong as should align with top > > > of mailbox registers. > > > > > > > > > > > > + * > > > > > + * This is roughly described in 8.2.8 Figure 138 of the CXL 2.0 spec > > > > > > > > I'm going drop this comment as that figure appears unrelated to me. > > > > > > > > + * > > > > > + * +-+ > > > > > + * | | > > > > > + * |Memory Device Registers | > > > > > + * | | > > > > > + * n + PAYLOAD_SIZE_MAX --- > > > > > + * ^| | > > > > > + * || | > > > > > + * || | > > > > > + * || | > > > > > + * || | > > > > > + *
Re: [PATCH v8 04/46] hw/cxl/device: Introduce a CXL device (8.2.8)
On Wed, Mar 30, 2022 at 06:48:48PM +0100, Jonathan Cameron wrote: > On Tue, 29 Mar 2022 18:13:59 + > Adam Manzanares wrote: > > > On Fri, Mar 18, 2022 at 03:05:53PM +, Jonathan Cameron wrote: > > > From: Ben Widawsky > > > > > > A CXL device is a type of CXL component. Conceptually, a CXL device > > > would be a leaf node in a CXL topology. From an emulation perspective, > > > CXL devices are the most complex and so the actual implementation is > > > reserved for discrete commits. > > > > > > This new device type is specifically catered towards the eventual > > > implementation of a Type3 CXL.mem device, 8.2.8.5 in the CXL 2.0 > > > specification. > > > > > > Signed-off-by: Ben Widawsky > > > Signed-off-by: Jonathan Cameron > > > Reviewed-by: Alex Bennée > > ... > > > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > > new file mode 100644 > > > index 00..b2416e45bf > > > --- /dev/null > > > +++ b/include/hw/cxl/cxl_device.h > > > @@ -0,0 +1,165 @@ > > > +/* > > > + * QEMU CXL Devices > > > + * > > > + * Copyright (c) 2020 Intel > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > > the > > > + * COPYING file in the top-level directory. > > > + */ > > > + > > > +#ifndef CXL_DEVICE_H > > > +#define CXL_DEVICE_H > > > + > > > +#include "hw/register.h" > > > + > > > +/* > > > + * The following is how a CXL device's MMIO space is laid out. The only > > > + * requirement from the spec is that the capabilities array and the > > > capability > > > + * headers start at offset 0 and are contiguously packed. The headers > > > themselves > > > + * provide offsets to the register fields. For this emulation, registers > > > will > > > + * start at offset 0x80 (m == 0x80). No secondary mailbox is implemented > > > which > > > + * means that n = m + sizeof(mailbox registers) + sizeof(device > > > registers). > > > > What is n here, the start offset of the mailbox registers, this question is > > based on the figure below? > > I'll expand on this to say > > means that the offset of the start of the mailbox payload (n) is given by > n = m + sizeof > > Which means the diagram below is wrong as should align with top > of mailbox registers. > > > > > > + * > > > + * This is roughly described in 8.2.8 Figure 138 of the CXL 2.0 spec > I'm going drop this comment as that figure appears unrelated to me. > > > > + * > > > + * +-+ > > > + * | | > > > + * |Memory Device Registers | > > > + * | | > > > + * n + PAYLOAD_SIZE_MAX --- > > > + * ^| | > > > + * || | > > > + * || | > > > + * || | > > > + * || | > > > + * || Mailbox Payload | > > > + * || | > > > + * || | > > > + * || | > > > + * |--- > > > + * || Mailbox Registers | > > > + * || | > > > + * n--- > > > + * ^| | > > > + * ||Device Registers | > > > + * || | > > > + * m--> > > > + * ^| Memory Device Capability Header| > > > + * |--- > > > + * || Mailbox Capability Header | > > > + * |-- >
Re: [PATCH v8 04/46] hw/cxl/device: Introduce a CXL device (8.2.8)
On Wed, Mar 30, 2022 at 01:15:58PM +0100, Jonathan Cameron wrote:
> On Tue, 29 Mar 2022 12:53:51 -0700
> Davidlohr Bueso wrote:
>
> > On Tue, 29 Mar 2022, Adam Manzanares wrote:
> > >> +typedef struct cxl_device_state {
> > >> +MemoryRegion device_registers;
> > >> +
> > >> +/* mmio for device capabilities array - 8.2.8.2 */
> > >> +MemoryRegion device;
> > >> +MemoryRegion caps;
> > >> +
> > >> +/* mmio for the mailbox registers 8.2.8.4 */
> > >> +MemoryRegion mailbox;
> > >> +
> > >> +/* memory region for persistent memory, HDM */
> > >> +uint64_t pmem_size;
> > >
> > >Can we switch this to mem_size and drop the persistent comment? It is my
> > >understanding that HDM is independent of persistence.
> >
> > Agreed, but ideally both volatile and persistent capacities would have been
> > supported in this patchset. I'm also probably missing specific reasons as to
> > why this isn't the case.
>
> Whilst it doesn't add a huge amount of complexity it does add some
> and the software paths in Linux we were developing this for are pmem focused.
> Hence volatile is on the todo list rather than in this first patch set.
> Not sensible to aim for feature complete in one go.
Makes complete sense. We can help with the Linux development for the volatile
side. I will add a couple of folks on cc. In addition, we would like to help
the CXL ecosystem in general so I anticipate we will have more reviews and
patches for CXL in general.
>
> >
> > Looking at it briefly could it be just a matter of adding to cxl_type3_dev
> > a new hostmem along with it's AddressSpace for the volatile? If so, I'm
> > thinking something along these lines:
> >
> > @@ -123,8 +123,8 @@ typedef struct cxl_device_state {
> > uint64_t host_set;
> > } timestamp;
> >
> > -/* memory region for persistent memory, HDM */
> > -uint64_t pmem_size;
> > +/* memory region for persistent and volatile memory, HDM */
> > +uint64_t pmem_size, mem_size;
> > } CXLDeviceState;
> >
> > /* Initialize the register block for a device */
> > @@ -235,9 +235,9 @@ typedef struct cxl_type3_dev {
> > PCIDevice parent_obj;
> >
> > /* Properties */
> > -AddressSpace hostmem_as;
> > +AddressSpace hostmem_as, hostmemv_as;
> > uint64_t size;
> > -HostMemoryBackend *hostmem;
> > +HostMemoryBackend *hostmem, *hostmemv;
> > HostMemoryBackend *lsa;
> > uint64_t sn;
> >
> > Then for cxl_setup_memory(), with ct3d->hostmem and/or ct3d->hostmemv
> > non-nil, set the respective MemoryRegions:
> >
> > +if (ct3d->hostmem) {
> > +memory_region_set_nonvolatile(mr, true);
> > +memory_region_set_enabled(mr, true);
> > +host_memory_backend_set_mapped(ct3d->hostmem, true);
> > +address_space_init(>hostmem_as, mr, name);
> > +ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
> > +}
> > +if (ct3d->hostmemv) {
> > +memory_region_set_nonvolatile(mrv, false);
> > +memory_region_set_enabled(mrv, true);
> > +host_memory_backend_set_mapped(ct3d->hostmemv, true);
> > +address_space_init(>hostmem_as, mrv, name);
> > +ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
> > +}
> >
> > For corresponding MB commands, it's mostly IDENTIFY_MEMORY_DEVICE that needs
> > updating:
> >
> > @@ -281,7 +281,7 @@ static ret_code cmd_identify_memory_device(struct
> > cxl_cmd *cmd,
> >
> > CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> > CXLType3Class *cvc = CXL_TYPE3_DEV_GET_CLASS(ct3d);
> > -uint64_t size = cxl_dstate->pmem_size;
> > +uint64_t size = cxl_dstate->pmem_size + cxl_dstate->mem_size;
> >
> > if (!QEMU_IS_ALIGNED(size, 256 << 20)) {
> > return CXL_MBOX_INTERNAL_ERROR;
> > @@ -290,11 +290,11 @@ static ret_code cmd_identify_memory_device(struct
> > cxl_cmd *cmd,
> > id = (void *)cmd->payload;
> > memset(id, 0, sizeof(*id));
> >
> > -/* PMEM only */
> > snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
> >
> > id->total_capacity = size / (256 << 20);
> > -id->persistent_capacity = size / (256 << 20);
> > +id->persistent
Re: [PATCH v8 04/46] hw/cxl/device: Introduce a CXL device (8.2.8)
ine CXL_DEVICE_CAP_HDR1_OFFSET 0x10 /* Figure 138 */
> +#define CXL_DEVICE_CAP_REG_SIZE 0x10 /* 8.2.8.2 */
> +#define CXL_DEVICE_CAPS_MAX 4 /* 8.2.8.2.1 + 8.2.8.5 */
> +
> +#define CXL_DEVICE_REGISTERS_OFFSET 0x80 /* Read comment above */
Is this to plan for future capabilities? If we have CAPS MAX doesn't this
allow us to remove the slack space.
> +#define CXL_DEVICE_REGISTERS_LENGTH 0x8 /* 8.2.8.3.1 */
Should we add status to the name here, or would it get too long?
> +
> +#define CXL_MAILBOX_REGISTERS_OFFSET \
> +(CXL_DEVICE_REGISTERS_OFFSET + CXL_DEVICE_REGISTERS_LENGTH)
> +#define CXL_MAILBOX_REGISTERS_SIZE 0x20 /* 8.2.8.4, Figure 139 */
> +#define CXL_MAILBOX_PAYLOAD_SHIFT 11
I see 20 in the spec.
> +#define CXL_MAILBOX_MAX_PAYLOAD_SIZE (1 << CXL_MAILBOX_PAYLOAD_SHIFT)
> +#define CXL_MAILBOX_REGISTERS_LENGTH \
> +(CXL_MAILBOX_REGISTERS_SIZE + CXL_MAILBOX_MAX_PAYLOAD_SIZE)
> +
> +typedef struct cxl_device_state {
> +MemoryRegion device_registers;
> +
> +/* mmio for device capabilities array - 8.2.8.2 */
> +MemoryRegion device;
> +MemoryRegion caps;
> +
> +/* mmio for the mailbox registers 8.2.8.4 */
> +MemoryRegion mailbox;
> +
> +/* memory region for persistent memory, HDM */
> +uint64_t pmem_size;
Can we switch this to mem_size and drop the persistent comment? It is my
understanding that HDM is independent of persistence.
> +} CXLDeviceState;
> +
> +/* Initialize the register block for a device */
> +void cxl_device_register_block_init(Object *obj, CXLDeviceState *dev);
> +
> +/* Set up default values for the register block */
> +void cxl_device_register_init_common(CXLDeviceState *dev);
> +
> +/*
> + * CXL 2.0 - 8.2.8.1 including errata F4
> + * Documented as a 128 bit register, but 64 bit accesses and the second
> + * 64 bits are currently reserved.
> + */
> +REG64(CXL_DEV_CAP_ARRAY, 0) /* Documented as 128 bit register but 64 byte
> accesses */
> +FIELD(CXL_DEV_CAP_ARRAY, CAP_ID, 0, 16)
> +FIELD(CXL_DEV_CAP_ARRAY, CAP_VERSION, 16, 8)
> +FIELD(CXL_DEV_CAP_ARRAY, CAP_COUNT, 32, 16)
> +
> +/*
> + * Helper macro to initialize capability headers for CXL devices.
> + *
> + * In the 8.2.8.2, this is listed as a 128b register, but in 8.2.8, it says:
> + * > No registers defined in Section 8.2.8 are larger than 64-bits wide so
> that
> + * > is the maximum access size allowed for these registers. If this rule is
> not
> + * > followed, the behavior is undefined
> + *
> + * CXL 2.0 Errata F4 states futher that the layouts in the specification are
> + * shown as greater than 128 bits, but implementations are expected to
> + * use any size of access up to 64 bits.
> + *
> + * Here we've chosen to make it 4 dwords. The spec allows any pow2 multiple
> + * access to be used for a register up to 64 bits.
> + */
> +#define CXL_DEVICE_CAPABILITY_HEADER_REGISTER(n, offset) \
> +REG32(CXL_DEV_##n##_CAP_HDR0, offset) \
> +FIELD(CXL_DEV_##n##_CAP_HDR0, CAP_ID, 0, 16) \
> +FIELD(CXL_DEV_##n##_CAP_HDR0, CAP_VERSION, 16, 8) \
> +REG32(CXL_DEV_##n##_CAP_HDR1, offset + 4) \
> +FIELD(CXL_DEV_##n##_CAP_HDR1, CAP_OFFSET, 0, 32) \
> +REG32(CXL_DEV_##n##_CAP_HDR2, offset + 8) \
> +FIELD(CXL_DEV_##n##_CAP_HDR2, CAP_LENGTH, 0, 32)
> +
> +CXL_DEVICE_CAPABILITY_HEADER_REGISTER(DEVICE, CXL_DEVICE_CAP_HDR1_OFFSET)
> +CXL_DEVICE_CAPABILITY_HEADER_REGISTER(MAILBOX, CXL_DEVICE_CAP_HDR1_OFFSET + \
> + CXL_DEVICE_CAP_REG_SIZE)
> +
Fig139 for the following registers.
8.2.8.4.3
> +REG32(CXL_DEV_MAILBOX_CAP, 0)
> +FIELD(CXL_DEV_MAILBOX_CAP, PAYLOAD_SIZE, 0, 5)
> +FIELD(CXL_DEV_MAILBOX_CAP, INT_CAP, 5, 1)
> +FIELD(CXL_DEV_MAILBOX_CAP, BG_INT_CAP, 6, 1)
> +FIELD(CXL_DEV_MAILBOX_CAP, MSI_N, 7, 4)
> +
8.2.8.4.4
> +REG32(CXL_DEV_MAILBOX_CTRL, 4)
> +FIELD(CXL_DEV_MAILBOX_CTRL, DOORBELL, 0, 1)
> +FIELD(CXL_DEV_MAILBOX_CTRL, INT_EN, 1, 1)
> +FIELD(CXL_DEV_MAILBOX_CTRL, BG_INT_EN, 2, 1)
> +
8.2.8.4.5 + 8.2.9
> +REG64(CXL_DEV_MAILBOX_CMD, 8)
> +FIELD(CXL_DEV_MAILBOX_CMD, COMMAND, 0, 8)
> +FIELD(CXL_DEV_MAILBOX_CMD, COMMAND_SET, 8, 8)
> +FIELD(CXL_DEV_MAILBOX_CMD, LENGTH, 16, 20)
> +
8.2.8.4.6
> +REG64(CXL_DEV_MAILBOX_STS, 0x10)
> +FIELD(CXL_DEV_MAILBOX_STS, BG_OP, 0, 1)
> +FIELD(CXL_DEV_MAILBOX_STS, ERRNO, 32, 16)
> +FIELD(CXL_DEV_MAILBOX_STS, VENDOR_ERRNO, 48, 16)
> +
8.2.8.4.7
> +REG64(CXL_DEV_BG_CMD_STS, 0x18)
> +FIELD(CXL_DEV_BG_CMD_STS, BG, 0, 16)
Should we call this OP since it is implied that we are BG given the register?
> +FIELD(CXL_DEV_BG_CMD_STS, DONE, 16, 7)
NUM_DONE? since this is a percentage.
> +FIELD(CXL_DEV_BG_CMD_STS, ERRNO, 32, 16)
Isn't this a RET_CODE since it is only valid if previous field is 100%
> +FIELD(CXL_DEV_BG_CMD_STS, VENDOR_ERRNO, 48, 16)
VENDOR_RET_CODE since the same rule for the previous field applies here.
> +
> +REG32(CXL_DEV_CMD_PAYLOAD, 0x20)
> +
> +#endif
> --
> 2.32.0
>
>
+cc Dave, Klaus, Tong
Other than the minor issues raised.
Looks good.
Reviewed by: Adam Manzanares
Re: [PATCH v8 02/46] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)
EC
> +};
> +
> +struct dvsec_header {
> +uint32_t cap_hdr;
> +uint32_t dv_hdr1;
> +uint16_t dv_hdr2;
> +} QEMU_PACKED;
> +QEMU_BUILD_BUG_ON(sizeof(struct dvsec_header) != 10);
> +
> +/*
> + * 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 cxl_dvsec_port_extensions {
> +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;
Limit high?
> +uint32_t rcrb_base;
> +uint32_t rcrb_base_high;
> +};
> +QEMU_BUILD_BUG_ON(sizeof(struct cxl_dvsec_port_extensions) != 0x28);
> +
> +#define PORT_CONTROL_OFFSET 0xc
> +#define PORT_CONTROL_UNMASK_SBR 1
> +#define PORT_CONTROL_ALT_MEMID_EN4
> +
> +/* CXL 2.0 - 8.1.6 GPF DVSEC (ID 0004) */
> +struct cxl_dvsec_port_gpf {
> +struct dvsec_header hdr;
> +uint16_t rsvd;
> +uint16_t phase1_ctrl;
> +uint16_t phase2_ctrl;
> +};
> +QEMU_BUILD_BUG_ON(sizeof(struct cxl_dvsec_port_gpf) != 0x10);
> +
> +/* CXL 2.0 - 8.1.8/8.2.1.3 Flexbus DVSEC (ID 0007) */
> +struct cxl_dvsec_port_flexbus {
> +struct dvsec_header hdr;
> +uint16_t cap;
> +uint16_t ctrl;
> +uint16_t status;
> +uint32_t rcvd_mod_ts_data_phase1;
> +};
> +QEMU_BUILD_BUG_ON(sizeof(struct cxl_dvsec_port_flexbus) != 0x14);
> +
> +/* CXL 2.0 - 8.1.9 Register Locator DVSEC (ID 0008) */
> +struct cxl_dvsec_register_locator {
> +struct dvsec_header hdr;
> +uint16_t rsvd;
> +uint32_t reg0_base_lo;
> +uint32_t reg0_base_hi;
> +uint32_t reg1_base_lo;
> +uint32_t reg1_base_hi;
> +uint32_t reg2_base_lo;
> +uint32_t reg2_base_hi;
> +};
> +QEMU_BUILD_BUG_ON(sizeof(struct cxl_dvsec_register_locator) != 0x24);
> +
> +/* BAR Equivalence Indicator */
> +#define BEI_BAR_10H 0
> +#define BEI_BAR_14H 1
> +#define BEI_BAR_18H 2
> +#define BEI_BAR_1cH 3
> +#define BEI_BAR_20H 4
> +#define BEI_BAR_24H 5
> +
> +/* Register Block Identifier */
> +#define RBI_EMPTY 0
> +#define RBI_COMPONENT_REG (1 << 8)
> +#define RBI_BAR_VIRT_ACL (2 << 8)
> +#define RBI_CXL_DEVICE_REG (3 << 8)
> +
> +#endif
> --
> 2.32.0
>
>
+cc (Klaus, Dave, Tong)
Other than the minor cleanups/nits.
Looks good.
Reviewed by: Adam Manzanares
Re: [PATCH v8 01/46] hw/pci/cxl: Add a CXL component type (interface)
On Fri, Mar 18, 2022 at 03:05:50PM +, Jonathan Cameron wrote:
> From: Ben Widawsky
>
> A CXL component is a hardware entity that implements CXL component
> registers from the CXL 2.0 spec (8.2.3). Currently these represent 3
> general types.
> 1. Host Bridge
> 2. Ports (root, upstream, downstream)
> 3. Devices (memory, other)
>
> A CXL component can be conceptually thought of as a PCIe device with
> extra functionality when enumerated and enabled. For this reason, CXL
> does here, and will continue to add on to existing PCI code paths.
>
> Host bridges will typically need to be handled specially and so they can
> implement this newly introduced interface or not. All other components
> should implement this interface. Implementing this interface allows the
> core PCI code to treat these devices as special where appropriate.
>
> Signed-off-by: Ben Widawsky
> Signed-off-by: Jonathan Cameron
> Reviewed-by: Alex Bennée
> ---
> hw/pci/pci.c | 10 ++
> include/hw/pci/pci.h | 8
> 2 files changed, 18 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 5cb1232e27..7883778a99 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -201,6 +201,11 @@ static const TypeInfo pci_bus_info = {
> .class_init = pci_bus_class_init,
> };
>
> +static const TypeInfo cxl_interface_info = {
> +.name = INTERFACE_CXL_DEVICE,
> +.parent= TYPE_INTERFACE,
> +};
> +
> static const TypeInfo pcie_interface_info = {
> .name = INTERFACE_PCIE_DEVICE,
> .parent= TYPE_INTERFACE,
> @@ -2182,6 +2187,10 @@ static void pci_qdev_realize(DeviceState *qdev, Error
> **errp)
> pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> }
>
> +if (object_class_dynamic_cast(klass, INTERFACE_CXL_DEVICE)) {
> +pci_dev->cap_present |= QEMU_PCIE_CAP_CXL;
> +}
> +
> pci_dev = do_pci_register_device(pci_dev,
> object_get_typename(OBJECT(qdev)),
> pci_dev->devfn, errp);
> @@ -2938,6 +2947,7 @@ static void pci_register_types(void)
> type_register_static(_bus_info);
> type_register_static(_bus_info);
> type_register_static(_pci_interface_info);
> +type_register_static(_interface_info);
> type_register_static(_interface_info);
> type_register_static(_device_type_info);
> }
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 3a32b8dd40..98f0d1b844 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -194,6 +194,8 @@ enum {
> QEMU_PCIE_LNKSTA_DLLLA = (1 << QEMU_PCIE_LNKSTA_DLLLA_BITNR),
> #define QEMU_PCIE_EXTCAP_INIT_BITNR 9
> QEMU_PCIE_EXTCAP_INIT = (1 << QEMU_PCIE_EXTCAP_INIT_BITNR),
> +#define QEMU_PCIE_CXL_BITNR 10
> +QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR),
> };
>
> #define TYPE_PCI_DEVICE "pci-device"
> @@ -201,6 +203,12 @@ typedef struct PCIDeviceClass PCIDeviceClass;
> DECLARE_OBJ_CHECKERS(PCIDevice, PCIDeviceClass,
> PCI_DEVICE, TYPE_PCI_DEVICE)
>
> +/*
> + * Implemented by devices that can be plugged on CXL buses. In the spec,
> this is
> + * actually a "CXL Component, but we name it device to match the PCI naming.
> + */
> +#define INTERFACE_CXL_DEVICE "cxl-device"
> +
> /* Implemented by devices that can be plugged on PCI Express buses */
> #define INTERFACE_PCIE_DEVICE "pci-express-device"
>
> --
> 2.32.0
>
>
Looks good.
Reviewed by: Adam Manzanares
