On Wed, 15 Jan 2025 15:58:46 +0800
Li Zhijian <lizhij...@fujitsu.com> wrote:
> Introduce the `CXL_T3_MSIX_VECTOR` enumeration to specify MSIX vector
> assignments specific to the Type 3 (T3) CXL device.
>
> The primary goal of this change is to encapsulate the MSIX vector uses
> that are unique to the T3 device within an enumeration, improving code
> readability and maintenance by avoiding magic numbers. This organizational
> change allows for more explicit references to each vector’s role, thereby
> reducing the potential for misconfiguration.
>
> It also modified `mailbox_reg_init_common` to accept the `msi_n` parameter,
> reflecting the new MSIX vector setup.
>
> This pertains to the T3 device privately; other endpoints should refrain from
> using it, despite its public accessibility to all of them.
>
> Signed-off-by: Li Zhijian <lizhij...@fujitsu.com>
A few things inline. I'll apply this with the following tweak.
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index e765edd227..3a0ee7e8e7 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -133,14 +133,6 @@ typedef enum {
CXL_MBOX_MAX = 0x20
} CXLRetCode;
-/* type3 device private */
-enum CXL_T3_MSIX_VECTOR {
- CXL_T3_MSIX_PCIE_DOE_TABLE_ACCESS = 0,
- CXL_T3_MSIX_EVENT_START = 2,
- CXL_T3_MSIX_MBOX = CXL_T3_MSIX_EVENT_START + CXL_EVENT_TYPE_MAX,
- CXL_T3_MSIX_VECTOR_NR
-};
-
typedef struct CXLCCI CXLCCI;
typedef struct cxl_device_state CXLDeviceState;
struct cxl_cmd;
diff --git a/hw/cxl/switch-mailbox-cci.c b/hw/cxl/switch-mailbox-cci.c
index 762d881588..833b824619 100644
--- a/hw/cxl/switch-mailbox-cci.c
+++ b/hw/cxl/switch-mailbox-cci.c
@@ -17,7 +17,7 @@
#include "hw/qdev-properties.h"
#include "hw/cxl/cxl.h"
-#define CXL_SWCCI_MSIX_MBOX CXL_T3_MSIX_MBOX
+#define CXL_SWCCI_MSIX_MBOX 3
static void cswmbcci_reset(DeviceState *dev)
{
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index ba1c79afe8..dc163f5b63 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -30,6 +30,14 @@
#include "hw/cxl/cxl.h"
#include "hw/pci/msix.h"
+/* type3 device private */
+enum CXL_T3_MSIX_VECTOR {
+ CXL_T3_MSIX_PCIE_DOE_TABLE_ACCESS = 0,
+ CXL_T3_MSIX_EVENT_START = 2,
+ CXL_T3_MSIX_MBOX = CXL_T3_MSIX_EVENT_START + CXL_EVENT_TYPE_MAX,
+ CXL_T3_MSIX_VECTOR_NR
+};
+
#define DWORD_BYTE 4
#define CXL_CAPACITY_MULTIPLIER (256 * MiB)
> ---
> V3: new patch to introduce a T3 specific enumeration # Jonathan
> ---
> hw/cxl/cxl-device-utils.c | 12 +++++-------
> hw/cxl/switch-mailbox-cci.c | 4 +++-
> hw/mem/cxl_type3.c | 12 ++++++------
> include/hw/cxl/cxl_device.h | 12 ++++++++++--
> 4 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 035d034f6dd8..52ad1e4c3f7a 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -352,10 +352,8 @@ static void device_reg_init_common(CXLDeviceState
> *cxl_dstate)
> }
> }
>
> -static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
> +static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate, int msi_n)
> {
> - const uint8_t msi_n = 9;
> -
> /* 2048 payload size */
> ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
> @@ -382,7 +380,7 @@ static void memdev_reg_init_common(CXLDeviceState
> *cxl_dstate)
> cxl_dstate->memdev_status = memdev_status_reg;
> }
>
> -void cxl_device_register_init_t3(CXLType3Dev *ct3d)
> +void cxl_device_register_init_t3(CXLType3Dev *ct3d, int msi_n)
> {
> CXLDeviceState *cxl_dstate = &ct3d->cxl_dstate;
> uint64_t *cap_h = cxl_dstate->caps_reg_state64;
> @@ -398,7 +396,7 @@ void cxl_device_register_init_t3(CXLType3Dev *ct3d)
> device_reg_init_common(cxl_dstate);
>
> cxl_device_cap_init(cxl_dstate, MAILBOX, 2, CXL_DEV_MAILBOX_VERSION);
> - mailbox_reg_init_common(cxl_dstate);
> + mailbox_reg_init_common(cxl_dstate, msi_n);
>
> cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000,
> CXL_MEM_DEV_STATUS_VERSION);
> @@ -408,7 +406,7 @@ void cxl_device_register_init_t3(CXLType3Dev *ct3d)
> CXL_MAILBOX_MAX_PAYLOAD_SIZE);
> }
>
> -void cxl_device_register_init_swcci(CSWMBCCIDev *sw)
> +void cxl_device_register_init_swcci(CSWMBCCIDev *sw, int msi_n)
> {
> CXLDeviceState *cxl_dstate = &sw->cxl_dstate;
> uint64_t *cap_h = cxl_dstate->caps_reg_state64;
> @@ -423,7 +421,7 @@ void cxl_device_register_init_swcci(CSWMBCCIDev *sw)
> device_reg_init_common(cxl_dstate);
>
> cxl_device_cap_init(cxl_dstate, MAILBOX, 2, 1);
> - mailbox_reg_init_common(cxl_dstate);
> + mailbox_reg_init_common(cxl_dstate, msi_n);
>
> cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000, 1);
> memdev_reg_init_common(cxl_dstate);
> diff --git a/hw/cxl/switch-mailbox-cci.c b/hw/cxl/switch-mailbox-cci.c
> index 65cdac6cc139..762d8815880d 100644
> --- a/hw/cxl/switch-mailbox-cci.c
> +++ b/hw/cxl/switch-mailbox-cci.c
> @@ -17,10 +17,12 @@
> #include "hw/qdev-properties.h"
> #include "hw/cxl/cxl.h"
>
> +#define CXL_SWCCI_MSIX_MBOX CXL_T3_MSIX_MBOX
Hard code this value. There is no logical reason it has
to match with the type 3 one. That is just an artifact of how
the code evolved.
> +
> static void cswmbcci_reset(DeviceState *dev)
> {
> CSWMBCCIDev *cswmb = CXL_SWITCH_MAILBOX_CCI(dev);
> - cxl_device_register_init_swcci(cswmb);
> + cxl_device_register_init_swcci(cswmb, CXL_SWCCI_MSIX_MBOX);
> }
>
> static void cswbcci_realize(PCIDevice *pci_dev, Error **errp)
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 0ae1704a345c..f64af19ed6ae 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -843,7 +843,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> ComponentRegisters *regs = &cxl_cstate->crb;
> MemoryRegion *mr = ®s->component_registers;
> uint8_t *pci_conf = pci_dev->config;
> - unsigned short msix_num = 10;
> int i, rc;
> uint16_t count;
>
> @@ -884,16 +883,17 @@ static void ct3_realize(PCIDevice *pci_dev, Error
> **errp)
> &ct3d->cxl_dstate.device_registers);
>
> /* MSI(-X) Initialization */
> - rc = msix_init_exclusive_bar(pci_dev, msix_num, 4, NULL);
> + rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL);
> if (rc) {
> goto err_address_space_free;
> }
> - for (i = 0; i < msix_num; i++) {
> + for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) {
> msix_vector_use(pci_dev, i);
> }
>
> /* DOE Initialization */
> - pcie_doe_init(pci_dev, &ct3d->doe_cdat, 0x190, doe_cdat_prot, true, 0);
> + pcie_doe_init(pci_dev, &ct3d->doe_cdat, 0x190, doe_cdat_prot, true,
> + CXL_T3_MSIX_PCIE_DOE_TABLE_ACCESS);
>
> cxl_cstate->cdat.build_cdat_table = ct3_build_cdat_table;
> cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
> @@ -908,7 +908,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> if (rc) {
> goto err_release_cdat;
> }
> - cxl_event_init(&ct3d->cxl_dstate, 2);
> + cxl_event_init(&ct3d->cxl_dstate, CXL_T3_MSIX_EVENT_START);
>
> /* Set default value for patrol scrub attributes */
> ct3d->patrol_scrub_attrs.scrub_cycle_cap =
> @@ -1202,7 +1202,7 @@ static void ct3d_reset(DeviceState *dev)
>
> pcie_cap_fill_link_ep_usp(PCI_DEVICE(dev), ct3d->width, ct3d->speed);
> cxl_component_register_init_common(reg_state, write_msk,
> CXL2_TYPE3_DEVICE);
> - cxl_device_register_init_t3(ct3d);
> + cxl_device_register_init_t3(ct3d, CXL_T3_MSIX_MBOX);
>
> /*
> * Bring up an endpoint to target with MCTP over VDM.
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 561b375dc86d..e765edd22772 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -133,6 +133,14 @@ typedef enum {
> CXL_MBOX_MAX = 0x20
> } CXLRetCode;
>
> +/* type3 device private */
> +enum CXL_T3_MSIX_VECTOR {
> + CXL_T3_MSIX_PCIE_DOE_TABLE_ACCESS = 0,
> + CXL_T3_MSIX_EVENT_START = 2,
> + CXL_T3_MSIX_MBOX = CXL_T3_MSIX_EVENT_START + CXL_EVENT_TYPE_MAX,
> + CXL_T3_MSIX_VECTOR_NR
> +};
> +
Push this down in to the cxl_type3.c file.
It it is not a global thing but specific choices for the type 3 emulation.
> typedef struct CXLCCI CXLCCI;
> typedef struct cxl_device_state CXLDeviceState;
> struct cxl_cmd;
> @@ -264,8 +272,8 @@ void cxl_device_register_block_init(Object *obj,
> CXLDeviceState *dev,
> typedef struct CXLType3Dev CXLType3Dev;
> typedef struct CSWMBCCIDev CSWMBCCIDev;
> /* Set up default values for the register block */
> -void cxl_device_register_init_t3(CXLType3Dev *ct3d);
> -void cxl_device_register_init_swcci(CSWMBCCIDev *sw);
> +void cxl_device_register_init_t3(CXLType3Dev *ct3d, int msi_n);
> +void cxl_device_register_init_swcci(CSWMBCCIDev *sw, int msi_n);
>
> /*
> * CXL r3.1 Section 8.2.8.1: CXL Device Capabilities Array Register
