Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX

qemu-devel
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration

From:	Jonathan Cameron
Subject:	Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration
Date:	Thu, 16 Jan 2025 10:50:04 +0000
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?
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  
> >
[Prev in Thread]
Current Thread
[Next in Thread]
[PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration, Li Zhijian, 2025/01/15
- Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration, Jonathan Cameron, 2025/01/15
  - Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration, Zhijian Li (Fujitsu), 2025/01/15
    - Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration, Jonathan Cameron <=
Prev by Date: Re: [ISSUE] memdev cannot be enabled after reboot due to failed dvsec range check [QEMU setup]
Next by Date: Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs
Previous by thread: Re: [PATCH] hw/cxl: Introduce CXL_T3_MSIX_VECTOR enumeration
Next by thread: [PATCH] MAINTAINERS: Mark me as reviewer only for 9pfs
Index(es):
- Date
- Thread