|SUCCESS| common/cnxk: fix VFIO MSI-X interrupt setup

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164225

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-21

Commit: <sha not directly visible in patch>
Author: Pavan Nikhilesh <pbhagavatula at marvell.com>
Subject: [PATCH] common/cnxk: fix VFIO MSI-X interrupt setup

This patch modifies the VFIO MSI-X interrupt setup in the cnxk common
driver to use heap allocation instead of a fixed stack buffer, and
adjusts the handling when irq.count is zero.

> diff --git a/drivers/common/cnxk/roc_platform.c b/drivers/common/cnxk/roc_platform.c
> index 1fdbf8f05143..59d662fab43b 100644
> --- a/drivers/common/cnxk/roc_platform.c
> +++ b/drivers/common/cnxk/roc_platform.c

[ ... ]

> @@ -39,11 +37,12 @@ irq_get_info(struct plt_intr_handle *intr_handle)
>  		     irq.count, PLT_MAX_RXTX_INTR_VEC_ID);
>  
>  	if (irq.count == 0) {
> -		plt_err("HW max=%d > PLT_MAX_RXTX_INTR_VEC_ID: %d", irq.count,
> -			PLT_MAX_RXTX_INTR_VEC_ID);
> -		plt_intr_max_intr_set(intr_handle, PLT_MAX_RXTX_INTR_VEC_ID);
> +		plt_warn("VFIO MSI-X irq.count is 0; using PLT_MAX_RXTX_INTR_VEC_ID=%u",
> +			 PLT_MAX_RXTX_INTR_VEC_ID);
> +		if (plt_intr_max_intr_set(intr_handle, PLT_MAX_RXTX_INTR_VEC_ID))
> +			return -1;

What is the error handling policy here? When plt_intr_max_intr_set()
fails with irq.count == 0, does the caller always handle the -1 return
code correctly? The previous code ignored the return value.

>  	} else {
> -		if (plt_intr_max_intr_set(intr_handle, irq.count))
> +		if (plt_intr_max_intr_set(intr_handle, (int)irq.count))
>  			return -1;
>  	}
>  

[ ... ]

> @@ -89,23 +88,34 @@ irq_config(struct plt_intr_handle *intr_handle, unsigned int vec)
>  static int
>  irq_init(struct plt_intr_handle *intr_handle)
>  {
> -	char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
> +	int max_intr, len, rc, vfio_dev_fd;
>  	struct vfio_irq_set *irq_set;
> -	int len, rc, vfio_dev_fd;
> +	char *irq_set_buf = NULL;
>  	int32_t *fd_ptr;
> -	uint32_t i;
> +	int i;
>  
> -	len = sizeof(struct vfio_irq_set) + sizeof(int32_t) * plt_intr_max_intr_get(intr_handle);
> +	max_intr = plt_intr_max_intr_get(intr_handle);
> +	if (max_intr <= 0) {
> +		plt_err("Invalid max_intr %d for irq init", max_intr);
> +		return -EINVAL;
> +	}
> +
> +	len = sizeof(struct vfio_irq_set) + sizeof(int32_t) * max_intr;

Does this multiplication check for integer overflow? When max_intr is
sufficiently large, could this calculation overflow len, then cause
malloc() below to allocate less memory than the code will later access
through irq_set->data[]?

> +	irq_set_buf = malloc((size_t)len);
> +	if (irq_set_buf == NULL) {
> +		plt_err("Failed to alloc irq_set_buf len=%d", len);
> +		return -ENOMEM;
> +	}
>  
>  	irq_set = (struct vfio_irq_set *)irq_set_buf;
>  	irq_set->argsz = len;
>  	irq_set->start = 0;
> -	irq_set->count = plt_intr_max_intr_get(intr_handle);
> +	irq_set->count = (uint32_t)max_intr;

The max_intr variable is checked to be > 0, but what is the upper bound
validation for it? Can max_intr exceed UINT32_MAX before the cast here?

>  	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
>  	irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
>  
>  	fd_ptr = (int32_t *)&irq_set->data[0];
> -	for (i = 0; i < irq_set->count; i++)
> +	for (i = 0; i < max_intr; i++)
>  		fd_ptr[i] = -1;
>  
>  	vfio_dev_fd = plt_intr_dev_fd_get(intr_handle);
> @@ -113,6 +123,7 @@ irq_init(struct plt_intr_handle *intr_handle)
>  	if (rc)
>  		plt_err("Failed to set irqs vector rc=%d", rc);
>  
> +	free(irq_set_buf);
>  	return rc;
>  }