Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration

[Zhijian Li (Fujitsu)]

On 15/01/2025 21:51, Jonathan Cameron wrote:
> On Wed, 15 Jan 2025 15:58:46 +0800
> Li Zhijian <lizhijian@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 <lizhijian@fujitsu.com>
> A few things inline. I'll apply this with the following tweak.

Thank you!

These changes are good to me.

Thanks
Zhijian


BTW, during making this patch, i noticed that are a few abused cleanup in the 
error path
of xxx_realize() vs xxx_exit()

For example:
ct3_realize()
{
...
err1:
   cleanup1;

err2:
   cleanup2;
}

ct3_exit()
{
   cleanup1;
   cleanup2;
}

These cleanups will function correctly only if both cleanup1 and cleanup2 are 
reentrant.
However, this is not the case in `ct3_realize()` and other CXL-related 
`xxx_realize()` functions.

When examining other devices, we rarely observe any cleanups in the 
`xxx_realize()`
error path, as the cleanup in `xxx_exit()` alone suffices.

I intend to address these cleanups if you have not yet done so.  @Jonathan


Thanks
Zhijian


> 
> 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 = &regs->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
>