On Mon, 29 Jun 2026 at 13:01, Peter Maydell <[email protected]> wrote:
>
> From: Matyáš Bobek <[email protected]>
>
> Added the FlexCAN2 emulator implementation core, with
> CAN_FLEXCAN Kconfig flag and MAINTAINERS entry.
>
> FlexCAN2 version can be found in i.MX6 SoCs and others.
>
> More information about the implementation can be found in [1].
>
> Some macro and struct defintions were borrowed from the Linux kernel.
> The original authors agreed with relicensing them to GPL-2.0-or-later on
> the qemu-devel mailing list.
>
> [1]
> http://dspace.cvut.cz/bitstream/handle/10467/122654/F3-BP-2025-Bobek-Matyas-BP_Bobek_FlexCAN_final_4.pdf
>
Hi; Coverity points out some issues with this change; details below.
> +/**
> + * flexcan_fifo_pop() - Pop message from FIFO and update IRQs
> + * @s: FlexCAN device pointer
> + *
> + * Does not require the queue to be non-empty.
> + */
> +static void flexcan_fifo_pop(FlexcanState *s)
> +{
> + if (s->regs.fifo.mb_back.can_ctrl != 0) {
> + /* move queue elements forward */
> + memmove(&s->regs.fifo.mb_back, &s->regs.fifo.mbs_queue[0],
> + sizeof(s->regs.fifo.mbs_queue));
CID 1662971: Coverity doesn't like this because mb_back is
only 1 element long and you're copying more than that.
We seem to use these fields only in this function -- did we
really need to specify them as two separate fields rather
than one array so that it's a bit clearer that we're just
copying within what is effectively a single 6 element array?
> +
> + /* clear the first-in slot */
> + memset(&s->regs.mbs[FLEXCAN_FIFO_DEPTH - 1], 0,
> + sizeof(FlexcanRegsMessageBuffer));
> +
> + trace_flexcan_fifo_pop(DEVICE(s)->canonical_path, 1,
> + s->regs.fifo.mb_back.can_ctrl != 0);
> + } else {
> + trace_flexcan_fifo_pop(DEVICE(s)->canonical_path, 0, 0);
> + }
> +
> + if (s->regs.fifo.mb_back.can_ctrl != 0) {
> + flexcan_irq_iflag_set(s, I_FIFO_AVAILABLE);
> + } else {
> + flexcan_irq_iflag_clear(s, I_FIFO_AVAILABLE);
> + }
> +}
> +
> +static enum FlexcanRx flexcan_mb_rx(FlexcanState *s, const qemu_can_frame
> *buf)
> +{
> + int last_not_free_to_receive_mbid = -1;
CID 1662790:
Initially last_not_free_to_receive_mbid is -1.
> + bool last_not_free_to_receive_locked = false;
> +
> + FlexcanRegsMessageBuffer *first_mb = flexcan_get_first_message_buffer(s);
> + FlexcanRegsMessageBuffer *last_mb = flexcan_get_last_enabled_mailbox(s);
> +
> + for (FlexcanRegsMessageBuffer *mb = first_mb;
> + mb <= last_mb; mb++) {
> + int mbid = mb - s->regs.mbs;
In this loop mbid must always be positive, because the bounds on mb
mean it's always somewhere inside the s->regs.mbs array.
> + enum FlexcanCheck r = flexcan_mb_rx_check_mb(s, buf, mbid);
> + if (r == FLEXCAN_CHECK_MB_MATCH) {
> + flexcan_mb_move_in(s, buf, mb);
> + flexcan_irq_iflag_set(s, mbid);
> + return FLEXCAN_RX_SEARCH_ACCEPT;
> + }
> +
> + if (r == FLEXCAN_CHECK_MB_MATCH_NON_FREE) {
> + last_not_free_to_receive_mbid = mbid;
> + last_not_free_to_receive_locked = false;
> + } else if (r == FLEXCAN_CHECK_MB_MATCH_LOCKED) {
> + /*
> + * message buffer is locked,
> + * we can move in the message after it's unlocked
> + */
> + last_not_free_to_receive_mbid = mbid;
> + last_not_free_to_receive_locked = true;
> + }
In the loop body the only places where we assign to
last_not_free_to_receive_mbid are where we set it to mbid.
> + }
> +
> + if (last_not_free_to_receive_mbid >= -1) {
So when can this condition be false ?
> + if (last_not_free_to_receive_locked) {
> + /*
> + * copy to temporary mailbox (SMB)
> + * it will be moved in when the mailbox is unlocked
> + */
> + s->regs.rx_smb0.can_ctrl =
> + s->regs.mbs[last_not_free_to_receive_mbid].can_id;
> + flexcan_mb_move_in(s, buf, &s->regs.rx_smb0);
> + s->smb_target_mbidx = last_not_free_to_receive_mbid;
> + return FLEXCAN_RX_SEARCH_ACCEPT;
> + }
> +
> + if (s->regs.mcr & FLEXCAN_MCR_IRMQ) {
> + flexcan_mb_move_in(s, buf,
> + &s->regs.mbs[last_not_free_to_receive_mbid]);
Here we use last_not_free_to_receive_mbid as an index into mbs,
but it could be -1, which will underrun the array.
Should the condition above have been ">= 0" ?
> + flexcan_irq_iflag_set(s, last_not_free_to_receive_mbid);
> + return FLEXCAN_RX_SEARCH_ACCEPT;
> + }
> + }
> +
> + return FLEXCAN_RX_SEARCH_RETRY;
> +}
> +/* ========== I/O handling ========== */
> +static void flexcan_mem_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> + FlexcanState *s = opaque;
> + uint32_t write_mask = ((const uint32_t *)
> + &flexcan_regs_write_mask)[addr / 4];
> + uint32_t old_value = s->regs_raw[addr / 4];
> +
> + /*
> + * 0 for bits that can "only be written in Freeze mode as it is blocked
> + * by hardware in other modes"
> + */
> + const uint32_t freeze_mask_mcr = 0xDF54CC80;
> + const uint32_t freeze_mask_ctrl1 = 0x0000E740;
> +
> + flexcan_trace_mem_op(s, addr, val, size, true);
> + switch (addr) {
> + case offsetof(FlexcanRegs, mcr):
> + if (!(s->regs.mcr & FLEXCAN_MCR_FRZ_ACK)) {
> + write_mask &= freeze_mask_mcr;
> + }
> + s->regs.mcr = (val & write_mask) | (old_value & ~write_mask);
> + flexcan_set_mcr(s, old_value);
> + break;
> + case offsetof(FlexcanRegs, ctrl):
> + if (!(s->regs.mcr & FLEXCAN_MCR_FRZ_ACK)) {
> + write_mask &= freeze_mask_ctrl1;
> + }
> + s->regs.ctrl = (val & write_mask) | (old_value & ~write_mask);
> + break;
> + case offsetof(FlexcanRegs, iflag1):
> + s->regs.iflag1 &= ~val;
> + if ((s->regs.mcr & FLEXCAN_MCR_FEN) &&
> + (val & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE)) {
> + flexcan_fifo_pop(s);
> + }
> + break;
> + case offsetof(FlexcanRegs, iflag2):
> + s->regs.iflag2 &= ~val;
> + break;
> + case offsetof(FlexcanRegs, ctrl2):
> + QEMU_FALLTHROUGH;
> + case offsetof(FlexcanRegs, ecr):
> + QEMU_FALLTHROUGH;
> + case offsetof(FlexcanRegs, rxmgmask):
> + QEMU_FALLTHROUGH;
> + case offsetof(FlexcanRegs, rx14mask):
> + QEMU_FALLTHROUGH;
> + case offsetof(FlexcanRegs, rx15mask):
> + QEMU_FALLTHROUGH;
> + case offsetof(FlexcanRegs, rxfgmask):
> + QEMU_FALLTHROUGH;
> + case offsetof(FlexcanRegs, rximr[0]) ... offsetof(FlexcanRegs,
> rximr[63]):
> + /* these registers can only be written in freeze mode */
> + if (!(s->regs.mcr & FLEXCAN_MCR_FRZ_ACK)) {
> + break;
> + }
> + QEMU_FALLTHROUGH;
> + default:
> + s->regs_raw[addr / 4] = (val & write_mask) | (old_value &
> ~write_mask);
> +
> + if (addr >= offsetof(FlexcanRegs, mb) &&
> + addr < offsetof(FlexcanRegs, _reserved4)) {
> + /* access to mailbox */
> + int mbid = (addr - offsetof(FlexcanRegs, mb)) /
> + sizeof(FlexcanRegsMessageBuffer);
CID 1662974: This bounds check isn't sufficient to ensure that
mbid is within the bounds of s->regs.mbs[], which flexcan_mb_write()
will access. This is the same as 1662975 below; see that for detail.
> +
> + if (s->locked_mbidx == mbid) {
> + flexcan_mb_unlock(s);
> + }
> +
> + /* check for invalid writes into FIFO region */
> + if (s->regs.mcr & FLEXCAN_MCR_FEN && mbid < FLEXCAN_FIFO_DEPTH) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid write to Rx-FIFO structure",
> + DEVICE(s)->canonical_path);
> + return;
> + }
> +
> + /* run mailbox processing function on write to control word */
> + if ((addr & 0xF) == 0) {
> + flexcan_mb_write(s, mbid);
> + }
> + }
> + break;
> + }
> +
> + flexcan_irq_update(s);
> +}
> +
> +static uint64_t flexcan_mem_read(void *opqaue, hwaddr addr, unsigned size)
> +{
> + FlexcanState *s = opqaue;
> + uint32_t rv = s->regs_raw[addr >> 2];
> +
> + if (addr >= offsetof(FlexcanRegs, mb) &&
> + addr < offsetof(FlexcanRegs, _reserved4)) {
> + /* reading from mailbox */
> + hwaddr offset = addr - offsetof(FlexcanRegs, mb);
> + int mbid = offset / sizeof(FlexcanRegsMessageBuffer);
> +
> + if (addr % 16 == 0 && s->locked_mbidx != mbid) {
> + /* reading control word locks the mailbox */
> + flexcan_mb_unlock(s);
> + flexcan_mb_lock(s, mbid);
> + flexcan_irq_update(s);
> + rv = s->regs.mbs[mbid].can_ctrl & ~FLEXCAN_MB_CNT_NOT_SRV;
CID 1662972, 1662975:
Here we guard the bounds on addr based on the struct layout,
where the relevant part is:
union { /* 0x80 - not affected by soft reset */
uint32_t mb[sizeof(FlexcanRegsMessageBuffer) * FLEXCAN_MAILBOX_COUNT];
FlexcanRegsMessageBuffer mbs[FLEXCAN_MAILBOX_COUNT];
FlexcanRegsRXFifo fifo;
};
uint32_t _reserved4[256]; /* 0x480 */
So we know that 'addr' is inside the union. However, 'mbs' is
not the smallest member of the union, because 'mbs' is 64 * 16 == 1024
bytes, but mb is 64 * 16 * 4 == 14436 bytes, and fifo is
(6 * 16 + 128 * 4) == 608 bytes. So because we only bounded
'mbid' based on the total size of the union, it can be larger
than the size of the mb[] array.
What is the intention here?
As an aside, using unions like this is rather error-prone, because
it is an easy way to introduce endianness bugs. This one happens to
work because all the fields in all the sub-structs end up being
uint32_t, but for instance if there was a uint16_t field in there
somewhere that would be an endianness bug.
> +
> + } else if (addr == offsetof(FlexcanRegs, timer)) {
> + flexcan_mb_unlock(s);
> + flexcan_irq_update(s);
> + rv = flexcan_get_timestamp(s, false);
> + }
> +
> + flexcan_trace_mem_op(s, addr, rv, size, false);
> + return rv;
> +}
> +#define FLEXCAN_GENMASK(h, l) (((~(uint32_t)0) >> (31 - (h) + (l))) << (l))
Please don't roll your own mask creation macros. We have
MAKE_64BIT_MASK() in bitops.h.
> +/* FLEXCAN CRC Register (CRCR) bits */
> +#define FLEXCAN_CRCR_MBCRC_MASK FLEXCAN_GENMASK(22, 16)
> +#define FLEXCAN_CRCR_MBCRC(x) (((x) & FLEXCAN_CRCR_MBCRC_MASK) <<
> 16)
CID 1662973: FLEXCAN_CRCR_MBCRC_MASK is 0x7F0000. That means
that (((x) & FLEXCAN_CRCR_MBCRC_MASK) << 16) will always be 0,
because the field gets shifted off the top of the 32-bit type.
I suspect you meant here
(((x) << 16) & FLEXCAN_CRCR_MBCRC_MASK)
The standard QEMU FIELD macros let you use FIELD_DP32() which
avoids this kind of bug entirely.
thanks
-- PMM
