Re: [PATCH v2] hw/cxl: Fix msix_notify: Assertion `vector < dev->msix_entries_nr`

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 13 Dec 2024 17:36:02 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:

> This assertion always happens when we sanitize the CXL memory device.
> $ echo 1 > /sys/bus/cxl/devices/mem0/security/sanitize
> 
> It is incorrect to register an MSIX number beyond the device's capability.
> 
> Expand the device's MSIX number and use the enum to maintain the *USED*
> and MAX MSIX number
> 
> Fixes: 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background completion")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V2: just increase msix number and add enum to maintainer their values #
> Jonathan

Ah. Sorry I was unclear. Two patches please

1. Make the number bigger to fix the bug. Only this one gets a fixes tag and
   is suitable for backporting.

2. Add an enum including all numbers currently used and use that throughout the
   type3 related code. That will prevent use accidentally introducing the
   bug in future but doesn't need to be backported. 
 
A few other comments inline.

Thanks

Jonathan

> ---
>  hw/cxl/cxl-device-utils.c   |  6 ++----
>  hw/mem/cxl_type3.c          | 10 +++++-----
>  include/hw/cxl/cxl_device.h |  7 +++++++
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 035d034f6d..bc2171e3d4 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -354,8 +354,6 @@ static void device_reg_init_common(CXLDeviceState *cxl_dstate)
>  
>  static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
>  {
> -    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);
> @@ -364,8 +362,8 @@ static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
>      ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
>                       BG_INT_CAP, 1);
>      ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> -                     MSI_N, msi_n);
> -    cxl_dstate->mbox_msi_n = msi_n;
> +                     MSI_N, CXL_MSIX_MBOX);

Should be passed in from the type 3 specific call so add a parameter to this
function and pass this from cxl_device_register_init_t3.
Even better pass it into there from ct3d_reset()

Will potentially be a different number for the switch CCI passed in from
the call of cxl_device_register_init_swcci() in switch-mailbox-cci.c


> +    cxl_dstate->mbox_msi_n = CXL_MSIX_MBOX;
>      ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
>                       MBOX_READY_TIME, 0); /* Not reported */
>      ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 5cf754b38f..f2f060ed9e 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 = 6;
>      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_MSIX_MAX, 4, NULL);
>      if (rc) {
>          goto err_address_space_free;
>      }
> -    for (i = 0; i < msix_num; i++) {
> +    for (i = 0; i < CXL_MSIX_MAX; 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_MSIX_PCIE_DOE);
>  
>      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_MSIX_EVENT_START);
>  
>      /* Set default value for patrol scrub attributes */
>      ct3d->patrol_scrub_attrs.scrub_cycle_cap =
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 561b375dc8..3f89b041ce 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -133,6 +133,13 @@ typedef enum {
>      CXL_MBOX_MAX = 0x20
>  } CXLRetCode;
>  
> +enum {

Maybe worth naming these to be type3 specific.

> +    CXL_MSIX_PCIE_DOE = 0,
Name it to include that this is specifically the DOE for the table access protocol.

   CXL_MSIX_PCIE_DOE_TABLE_ACCESS


This should be private to cxl_type3.c which should be possible by passing
it to a few more calls from there. 

> +    CXL_MSIX_EVENT_START = 2,
> +    CXL_MSIX_MBOX = CXL_MSIX_EVENT_START + CXL_EVENT_TYPE_MAX,
> +    CXL_MSIX_MAX
> +};
> +
>  typedef struct CXLCCI CXLCCI;
>  typedef struct cxl_device_state CXLDeviceState;
>  struct cxl_cmd;