Re: [PATCH 3/3] hw/cxl: introduce CXL type-2 device emulation

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Thu, 12 Dec 2024 05:04:22 -0800
Zhi Wang <zhiw@nvidia.com> wrote:

> From: Zhi Wang <zhiwang@kernel.org>
> 
> Introduce a CXL type-2 device emulation that provides a minimum base for
> testing kernel CXL core type-2 support and CXL type-2 virtualization. It
> is also a good base for introducing the more emulated features.
> 
> Currently, it only supports:
> 
> - Emulating component registers with HDM decoders.
> - Volatile memory backend and emualtion of region access.
> 
> The emulation is aimed to not tightly coupled with the current CXL type-3
> emulation since many advanced CXL type-3 emulation features are not
> implemented in a CXL type-2 device.
> 
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Zhi Wang <zhiwang@kernel.org>

Hi Zhi,

A few passing comments.

Jonathan

> diff --git a/hw/mem/cxl_accel.c b/hw/mem/cxl_accel.c
> new file mode 100644
> index 0000000000..770072126d
> --- /dev/null
> +++ b/hw/mem/cxl_accel.c
> @@ -0,0 +1,319 @@

> +
> +static void update_dvsecs(CXLAccelDev *acceld)

Just to make them easier to search for and void clashes, good to prefix
all functions with cxlacc or something like that.

> +{

/...


> +static Property cxl_accel_props[] = {
> +    DEFINE_PROP_LINK("volatile-memdev", CXLAccelDev, hostvmem,
> +                     TYPE_MEMORY_BACKEND, HostMemoryBackend *),

Does backing a type 2 device with a memdev provide any advantages?
I'd have thought a device specific memory allocation would make more
sense, like we'd do for a memory BAR on a PCI device.  That might
complicate the cxl-host handling though so perhaps this is a good
way to go for now.


> +    DEFINE_PROP_END_OF_LIST(),

When you get time, rebase as these have gone away recently.
I aim to get a fresher staging tree out shortly.

> +};
> +
> +static void cxl_accel_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
> +
> +    pc->realize = cxl_accel_realize;
> +    pc->exit = cxl_accel_exit;
> +
> +    pc->class_id = PCI_CLASS_CXL_QEMU_ACCEL;
> +    pc->vendor_id = PCI_VENDOR_ID_INTEL;
> +    pc->device_id = 0xd94;

If you are posting these I hope you have those IDs reserved
(which seems unlikely ;)
We need to be absolutely sure we never hit an existing ID which generally
means you need to find whoever controls those allocations in your company
and get them to give you an ID for this.

> +    pc->revision = 1;
> +
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +    dc->desc = "CXL Accelerator Device (Type 2)";
> +    device_class_set_legacy_reset(dc, cxl_accel_reset);
> +    device_class_set_props(dc, cxl_accel_props);
> +}

>  void cxl_event_init(CXLDeviceState *cxlds, int start_msg_num);
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index f1a53fea8d..08bc469316 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -55,6 +55,7 @@
>  #define PCI_CLASS_MEMORY_RAM             0x0500
>  #define PCI_CLASS_MEMORY_FLASH           0x0501
>  #define PCI_CLASS_MEMORY_CXL             0x0502
> +#define PCI_CLASS_CXL_QEMU_ACCEL         0x0503

Either this is a real device class (which seems unlikely given the name)
or you need to choose something else.  PCI maintains a big list of
class codes and currently 0x0502 is the highest one define in baseclass 05h
(memory controllers)

https://members.pcisig.com/wg/PCI-SIG/document/20113
(behind a pcisig login)

>  #define PCI_CLASS_MEMORY_OTHER           0x0580
>  
>  #define PCI_BASE_CLASS_BRIDGE            0x06