Re: [PATCH v14 net-next 07/11] net/nebula-matrix: add intr resource implementation

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/nebula-matrix: add intr resource implementation
    
Implement dynamic allocation and management of MSI-X interrupt vectors,
including network interrupts and other interrupt types. Maintain the
MSI-X mapping table (msix_map_table) to establish interrupt associations
between hardware and software. Support enabling/disabling specific
interrupts through hardware operations.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> index 55d6ed70a9e60..612d6c9812b01 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> @@ -64,6 +64,76 @@ static void nbl_hw_wr_regs(struct nbl_hw_mgt *hw_mgt, u64 reg, const u32 *data,

[ ... ]

> +static void nbl_hw_configure_msix_map(struct nbl_hw_mgt *hw_mgt, u16 func_id,
> +				      bool valid, dma_addr_t dma_addr, u8 bus,
> +				      u8 devid, u8 function)
> +{
> +	union nbl_function_msix_map_u function_msix_map;
> +
> +	memset(&function_msix_map, 0, sizeof(function_msix_map));
> +	if (valid) {
> +		function_msix_map.info.msix_map_base_addr = dma_addr;
> +		/* use af's bdf, because dma memmory is alloc by af */
> +		function_msix_map.info.function = function;
> +		function_msix_map.info.devid = devid;
> +		function_msix_map.info.bus = bus;
> +		function_msix_map.info.valid = 1;
> +	}
> +
> +	nbl_hw_wr_regs(hw_mgt,
> +		       NBL_PCOMPLETER_FUNCTION_MSIX_MAP_REG_ARR(func_id),
> +		       function_msix_map.data, sizeof(function_msix_map));

Is there an out-of-bounds array access here when passing
function_msix_map.data?

union nbl_function_msix_map_u contains struct info which is 16 bytes long,
but its data array is defined as u32 data[NBL_FUNC_MSIX_MAP_DWLEN], where
NBL_FUNC_MSIX_MAP_DWLEN is 3 (12 bytes).

When nbl_hw_wr_regs() receives sizeof(function_msix_map) (16 bytes), it will
iterate 4 times and access data[3]. Since data is an array of size 3, this
triggers an out-of-bounds memory violation under CONFIG_UBSAN_BOUNDS.
Should NBL_FUNC_MSIX_MAP_DWLEN be updated to 4?

> +}

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h
> index 15dc7f78afc0b..b1f5724e727a9 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h
> @@ -18,6 +18,35 @@
>  
>  struct nbl_resource_mgt;
>  
> +/* --------- INTERRUPT ---------- */
> +#define NBL_MAX_OTHER_INTERRUPT			1024
> +#define NBL_MAX_NET_INTERRUPT			4096
> +
> +struct nbl_msix_map {
> +	u16 valid:1;
> +	u16 global_msix_index:13;
> +	u16 rsv:2;
> +};

Does this code use C bitfields for hardware-shared structures?

This structure is used for DMA-coherent memory in
nbl_res_intr_configure_msix_map():

drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c:nbl_res_intr_configure_msix_map() {
    ...
	msix_map_table->size =
		sizeof(struct nbl_msix_map) * NBL_MSIX_MAP_TABLE_MAX_ENTRIES;
	msix_map_table->base_addr = dma_alloc_coherent(dev,
						       msix_map_table->size,
    ...
}

Bitfield packing order is implementation-defined in C. On Big Endian
architectures, compilers pack bitfields in the opposite direction from Little
Endian, which will cause the hardware to read misaligned or reversed bit
positions. Standard kernel practice mandates using explicitly sized types
like __le16 and bitwise operations (FIELD_PREP, etc.) for hardware-facing
memory.

The same applies to MMIO registers written using unions containing bitfields,
such as union nbl_function_msix_map_u and union nbl_mailbox_qinfo_map_table_u.
Could these be converted to standard bitwise operations as well?