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

[Zhijian Li (Fujitsu)]

On 16/01/2025 18:50, Jonathan Cameron wrote:
> On Thu, 16 Jan 2025 01:18:28 +0000
> "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:
> 
>> 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
> 
> You've lost me.  If realize() fails we should exit hard.  Why is ct3_exit() 
> called
> as well.  

> Perhaps the issue is failure to set errp in some error paths?

Yes, the problem seems to be that some error paths do not set `errp`.
In the `qdev` scheme, the upper-level caller will assume that `realize()` 
succeeded if `errp` is `NULL`.


Thanks
Zhijian

> So we give the impression that it is fine to carry on when we should not.
> 
> 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
>>>   
>