Re: [PATCH 02/15] yunsilicon: add the register HAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] net: xsc_get_db_addr returns hardcoded physical address instead of PCI BAR
- [High] net: global state corruption via shared static xsc_hw_reg structures
- [High] net: endianness breakage from C bitfields mapped to hardware descriptors
- [High] net: hardcoded 0xf clear of W1C interrupt register causes interrupt storm
- [High] net: silent fallback to 1GB page size for unhandled page shifts
- [Medium] net: uninitialized stack padding leaked to hardware doorbell MMIO registers
- [Medium] net: stale in_line bit retained in recycled DMA data segments
- [Medium] net: integer overflow in xsc_get_max_mr_size on 32-bit architectures
--

commit ab5e7ef7c47a715df12f926749f1e49fe112c55d
Author: Xin Tian <tianx@yunsilicon.com>
yunsilicon: add the register HAL

This commit adds the register access layer and per-device register map for
doorbells, interrupts, and command queues. The implementation provides
register offsets and configurations for the Andes device.

diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c b/drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c
new file mode 100644
index 0000000000000..46fc045f92c02
--- /dev/null
+++ b/drivers/net/ethernet/yunsilicon/xsc/pci/hal/andes_impl.c
@@ -0,0 +1,708 @@
[ ... ]
> +static void andes_ring_rx_doorbell(struct xsc_hw_reg *regs,
> +				   void __iomem *bar, u32 rqn, u32 next_pid)
> +{
> +	union xsc_recv_doorbell {
> +		struct{
> +			u32  next_pid : 13;
> +			u32 qp_id : 15;
> +		};
> +		u32 raw;
> +	} db;
> +
> +	db.next_pid = next_pid;
> +	db.qp_id = rqn;
> +
> +	/* Make sure that descriptors are written before
> +	 * updating doorbell record and ringing the doorbell
> +	 */
> +	wmb();
> +
> +	writel(db.raw, REG_ADDR(bar, regs->rx_db));

[Severity: Medium]
Does this leak uninitialized kernel stack memory to the hardware? The union
instantiated on the stack is not fully zero-initialized before its raw 32-bit
value is written to MMIO, leaving padding bits uninitialized.

[ ... ]
> +static void andes_set_eq_ci(struct xsc_hw_reg *regs,
> +			    void __iomem *bar, u32 eqn, u32 next_cid,
> +			    u8 arm)
> +{
> +	union andes_eq_doorbell {
> +		struct{
> +			u32 eq_next_cid : 11;
> +			u32 eq_id : 11;
> +			u32 arm : 1;
> +		};
> +		u32 val;
> +	} db;
> +
> +	db.eq_next_cid = next_cid;
> +	db.eq_id = eqn;
> +	db.arm = !!arm;
> +
> +	/* make sure val write to memory done */
> +	wmb();
> +	writel(db.val, REG_ADDR(bar, regs->event_db));

[Severity: Medium]
Could this leak uninitialized stack memory to the event doorbell register,
similar to the rx doorbell above?

[ ... ]
> +static u8 andes_get_mr_page_mode(u8 page_shift)
> +{
> +enum {
> +	XSC_PAGE_SHIFT_4K	= 12,
> +	XSC_PAGE_SHIFT_64K	= 16,
> +	XSC_PAGE_SHIFT_2M	= 21,
> +	XSC_PAGE_SHIFT_1G	= 30,
> +};
> +
> +enum {
> +	XSC_PAGE_MODE_4K	= 0,
> +	XSC_PAGE_MODE_64K	= 1,
> +	XSC_PAGE_MODE_2M	= 2,
> +	XSC_PAGE_MODE_1G	= 3,
> +};
> +
> +	return (page_shift == XSC_PAGE_SHIFT_4K ? XSC_PAGE_MODE_4K :
> +		(page_shift == XSC_PAGE_SHIFT_64K ? XSC_PAGE_MODE_64K :
> +		(page_shift == XSC_PAGE_SHIFT_2M ? XSC_PAGE_MODE_2M : XSC_PAGE_MODE_1G)));

[Severity: High]
Does this silently fall back to XSC_PAGE_MODE_1G for unhandled page shifts
(such as 8K or 16K)? Could this cause out-of-bounds DMA accesses if the
hardware incorrectly maps 8K/16K buffers as 1GB pages?

[ ... ]
> +struct andes_cqe {
> +	union {
> +		u8		msg_opcode;
> +		struct {
> +			u8		error_code:7;
> +			u8		is_error:1;
> +		};
> +	};
> +	__le32		qp_id:15;
> +	u8		rsv1:1;
> +	u8		se:1;
> +	u8		has_pph:1;
> +	u8		type:1;
> +	u8		with_immdt:1;
> +	u8		csum_err:4;
> +	__le32		imm_data;
> +	__le32		msg_len;
> +	__le32		vni;
> +	__le64		ts:48;
> +	__le16		wqe_id;
> +	__le16		rsv[3];
> +	__le16		rsv2:15;
> +	u8		owner:1;
> +};
> +
> +static int andes_get_cqe_wqe_id(void *cqe)
> +{
> +	return ((struct andes_cqe *)cqe)->wqe_id;

[Severity: High]
Does mapping C bitfields directly to hardware descriptors break on big-endian
systems? The compiler layout of bitfields is implementation-defined, which
frequently results in reversed bit packing.

Also, shouldn't le16_to_cpu() be used here when returning the __le16 wqe_id
as a native integer to avoid returning out-of-bounds indices on big-endian
architectures?

[ ... ]
> +static void andes_set_data_seg(void *data_seg, u32 length, u32 key, u64 addr)
> +{
> +	struct andes_data_seg *seg = data_seg;
> +
> +	seg->length = length;
> +	seg->key = key;
> +	seg->addr = addr;

[Severity: Medium]
If a data segment descriptor is recycled from a ring buffer, does failing to
explicitly clear the in_line bitfield here cause the hardware to misinterpret
the address and key as inline data?

[ ... ]
> +struct andes_send_wqe_ctrl_seg {
> +	__le32		msg_opcode:8;
> +	__le32		with_immdt:1;
> +	__le32		csum_en:2;
> +	__le32		ds_data_num:5;
> +	__le32		wqe_id:16;
> +	__le32		msg_len;
[ ... ]
> +static void andes_set_wqe_id(void *cseg, u32 wqe_id)
> +{
> +	u16 _wqe_id = wqe_id;
> +
> +	((struct andes_send_wqe_ctrl_seg *)cseg)->wqe_id = cpu_to_le16(_wqe_id);

[Severity: High]
Is it safe to assign an explicitly byte-swapped value directly to a bitfield?
This could doubly corrupt the data on big-endian systems.

[ ... ]
> +static struct xsc_hw_reg andes_pf_regs = {
> +	.req_pid_addr = HIF_CMDQM_HOST_REQ_PID_MEM_ADDR - 0xa0000000,
> +	.req_cid_addr = HIF_CMDQM_HOST_REQ_CID_MEM_ADDR - 0xa0000000,
[ ... ]
> +static struct xsc_hw_reg andes_compressed_regs = {
> +	.tx_db = TX_DB_FUNC_MEM_ADDR,
> +	.rx_db = RX_DB_FUNC_MEM_ADDR,
[ ... ]
> +void xsc_init_hal(struct xsc_core_device *xdev, u32 device_id)
> +{
> +	switch (device_id) {
> +	case XSC_MC_VF_DEV_ID:
> +	case XSC_MF_HOST_VF_DEV_ID:
> +	case XSC_MS_VF_DEV_ID:
> +	case XSC_MV_HOST_VF_DEV_ID:
> +		/* VFs expose the same BAR-relative window as a bar-compressed PF */
> +		xdev->hal_regs = &andes_compressed_regs;
> +		break;
> +	default:
> +		if (is_pf_bar_compressed(xdev))
> +			xdev->hal_regs = &andes_compressed_regs;
> +		else
> +			xdev->hal_regs = &andes_pf_regs;

[Severity: High]
Could multiple devices probed at the same time overwrite each other's hardware
capability offsets? It appears xdev->hal_regs is assigned to a globally shared
static structure rather than allocating a separate configuration per-device.

[ ... ]
> +void xsc_set_mtt_info(struct xsc_core_device *xdev)
> +{
> +	struct xsc_hw_reg *regs = xdev->hal_regs;
> +
> +	regs->mpt_tbl_addr = xdev->caps.mpt_tbl_addr - 0xa0000000;
> +	regs->mpt_tbl_depth = xdev->caps.mpt_tbl_depth;
> +	regs->mpt_tbl_width = xdev->caps.mpt_tbl_width;
> +	regs->mtt_inst_base_addr = xdev->caps.mtt_inst_base_addr - 0xa0000000;

[Severity: High]
Does this mutate the global shared static structure assigned during
xsc_init_hal() with device-specific values?

[ ... ]
> +void xsc_get_db_addr(struct xsc_core_device *xdev,
> +		     u64 *tx_db, u64 *rx_db, u64 *cq_db, u64 *cq_reg, u64 *eq_db)
> +{
> +	struct xsc_hw_reg *regs = xdev->hal_regs;
> +
> +	if (!xsc_core_is_pf(xdev) || is_pf_bar_compressed(xdev)) {
[ ... ]
> +	} else {
> +		if (tx_db)
> +			*tx_db = regs->tx_db + 0xa0000000;
> +		if (rx_db)
> +			*rx_db = regs->rx_db + 0xa0000000;
> +		if (cq_db)
> +			*cq_db = regs->complete_db + 0xa0000000;
> +		if (cq_reg)
> +			*cq_reg = regs->complete_reg + 0xa0000000;
> +		if (eq_db)
> +			*eq_db = regs->event_db + 0xa0000000;
> +	}

[Severity: Critical]
Does adding a hardcoded offset of 0xa0000000 to the doorbell register offsets
yield a valid physical address? PCI device BAR physical addresses are typically
dynamically assigned and should be retrieved via pci_resource_start().
Returning a hardcoded physical address to upper layers could map arbitrary
physical memory.

[ ... ]
> +int xsc_handle_cmdq_interrupt(struct xsc_core_device *xdev)
> +{
> +	union interrupt_stat {
> +		struct {
> +			u32	hw_read_req_err:1;
> +			u32	hw_write_req_err:1;
> +			u32	req_pid_err:1;
> +			u32	rsp_cid_err:1;
> +		};
> +		u32	raw;
> +	} stat;
> +
> +	int err = 0;
> +	int retry = 0;
> +
> +	xsc_read32(xdev->bar, xdev->hal_regs->interrupt_stat_addr, &stat.raw,
> +		   sizeof(stat.raw));
> +	while (stat.raw != 0) {
> +		err++;
> +		if (stat.hw_read_req_err) {
[ ... ]
> +		} else {
> +			stat.raw = 0;
> +			pci_err(xdev->pdev, "ignore unknown interrupt!\n");
> +		}
> +	}
> +
> +	if (retry) {
> +		if (xdev->cmd.retry_cnt >= XSC_CMD_MAX_RETRY_CNT) {
> +			pci_warn(xdev->pdev, "err handler have retried for %d times, stop retry\n",
> +				 xdev->cmd.retry_cnt);
> +			goto clear_err_int;
> +		}
> +		xsc_write32(xdev->bar, xdev->hal_regs->req_pid_addr,
> +			    &xdev->cmd.cmd_pid);
> +		xdev->cmd.retry_cnt++;
> +	}
> +
> +clear_err_int:
> +	if (err) {
> +		stat.raw = 0xf;
> +		xsc_write32(xdev->bar, xdev->hal_regs->interrupt_stat_addr, &stat.raw);

[Severity: High]
Will writing a hardcoded 0xf back to the hardware W1C register cause an
interrupt storm if a higher-order bit (e.g., bit 4) was asserted, since the
unknown bit would never be cleared?

Also, could unconditionally writing 1s to bits 0-3 clear new legitimate
interrupts that arrived after the initial read?

[ ... ]
> +u64 xsc_get_max_mr_size(struct xsc_core_device *xdev)
> +{
> +	u64 max_mpt_mr_size = xdev->caps.max_mr_size;
> +	u64 max_mtt_mr_size = xdev->caps.max_mtt * PAGE_SIZE;

[Severity: Medium]
Can this calculation overflow on 32-bit architectures where PAGE_SIZE is an
unsigned long? Both operands to the multiplication are 32-bit integers before
the assignment to the 64-bit result, which could truncate the capacity before
the type conversion happens.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=2