On Thu, 12 Dec 2024 16:55:33 +0800
Li Zhijian via <qemu-devel@nongnu.org> 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 to 10 and introduce the `request_msix_number()`
> helper function to dynamically request an available MSIX number.
>
> Fixes: 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
> completion")
> Signed-off-by: Li Zhijian <lizhij...@fujitsu.com>
Hi.
Thanks for testing + the fix.
This looks like a mess up by me due to reordering patches.
In the first instance, the fix should just be to increase msi_n (keep it
minimal)
The refactor to use an allocator may makes sense as a follow up, but needs
to be used universally for allocation of each msix, not just for the later
ones. However, it may be simpler to just use an enum with a _MAX final
entry to ensure we allocate the right overall number. These are fixed
numbers and a restricted resource, so dynamic allocator is probably unnecessary.
Longer term we need to spend some time on automated tests so this sort of silly
bug doesn't happen in future :(
Thanks,
Jonathan
> ---
> hw/cxl/cxl-device-utils.c | 3 ++-
> hw/mem/cxl_type3.c | 15 ++++++++++++++-
> include/hw/cxl/cxl_device.h | 2 ++
> 3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index 035d034f6d..8e52af6813 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -354,8 +354,9 @@ static void device_reg_init_common(CXLDeviceState
> *cxl_dstate)
>
> static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
> {
> - const uint8_t msi_n = 9;
> + uint8_t msi_n = cxl_request_msi_number();
>
> + assert(msi_n > 0);
> /* 2048 payload size */
> ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CAP,
> PAYLOAD_SIZE, CXL_MAILBOX_PAYLOAD_SHIFT);
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 5cf754b38f..dbb1368736 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -835,6 +835,19 @@ static DOEProtocol doe_cdat_prot[] = {
> { }
> };
>
> +#define CT3_MSIX_NUM 10
> +unsigned short cxl_request_msi_number(void)
> +{
> + const unsigned short start = 6;
> + static unsigned short next = start;
> +
> + if (next + 1 >= CT3_MSIX_NUM) {
> + return -1;
> + }
> +
> + return ++next;
> +}
> +
> static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> {
> ERRP_GUARD();
> @@ -843,7 +856,7 @@ 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 = 6;
> + unsigned short msix_num = CT3_MSIX_NUM;
> int i, rc;
> uint16_t count;
>
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 561b375dc8..622265f50e 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -680,4 +680,6 @@ void ct3_clear_region_block_backed(CXLType3Dev *ct3d,
> uint64_t dpa,
> uint64_t len);
> bool ct3_test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> uint64_t len);
> +unsigned short cxl_request_msi_number(void);
> +
> #endif
