On Wed, Sep 17, 2025 at 01:44:47PM +0200, Luc Michel wrote: > The CANFD device has several groups of registers: > - the main control registers from 0x0 to 0xec > - several banks of multiple registers. The number of banks is either > hardcoded, or configurable using QOM properties: > - Tx registers > - Filter registers > - Tx events registers > - Rx0 registers > - Rx1 registers > > As of now, all the registers are handled using the register API. The > banked register logic results in a convoluted code to correctly allocate > the register descriptors for the register API. This code bypasses the > standard register API creation function (register_init_block). The > resulting code leaks memory when the device is finalized. > > This commit introduces decoding logic for the banked registers. Those > registers are quite simple in practice. Accessing them triggers no > side-effect (only the filter registers need a check to catch guest > invalid behaviour). Starting from the Tx events registers, they are all > read-only. > > The main device memory region is changed to an I/O one, calling the > new decoding logic when accessed. The register API memory region still > overlaps all of it so for now the introduced code has no effect. The > next commit will remove the register API usage for banked registers. > > Signed-off-by: Luc Michel <[email protected]>
Reviewed-by: Francisco Iglesias <[email protected]> > --- > hw/net/can/xlnx-versal-canfd.c | 155 ++++++++++++++++++++++++++++++++- > 1 file changed, 153 insertions(+), 2 deletions(-) > > diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c > index 343348660b5..81615bc52a6 100644 > --- a/hw/net/can/xlnx-versal-canfd.c > +++ b/hw/net/can/xlnx-versal-canfd.c > @@ -1408,10 +1408,27 @@ static uint64_t canfd_srr_pre_write(RegisterInfo > *reg, uint64_t val64) > } > > return s->regs[R_SOFTWARE_RESET_REGISTER]; > } > > +static void filter_reg_write(XlnxVersalCANFDState *s, hwaddr addr, > + size_t bank_idx, uint32_t val) > +{ > + size_t reg_idx = addr / sizeof(uint32_t); > + > + if (!(s->regs[R_ACCEPTANCE_FILTER_CONTROL_REGISTER] & > + (1 << bank_idx))) { > + s->regs[reg_idx] = val; > + } else { > + g_autofree char *path = object_get_canonical_path(OBJECT(s)); > + > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Acceptance filter register 0x%" > + HWADDR_PRIx " changed while filter %zu enabled\n", > + path, addr, bank_idx + 1); > + } > +} > + > static uint64_t filter_mask(RegisterInfo *reg, uint64_t val64) > { > XlnxVersalCANFDState *s = XILINX_CANFD(reg->opaque); > uint32_t reg_idx = (reg->access->addr) / 4; > uint32_t val = val64; > @@ -1761,11 +1778,145 @@ static const RegisterAccessInfo canfd_regs_info[] = { > static void xlnx_versal_canfd_ptimer_cb(void *opaque) > { > /* No action required on the timer rollover. */ > } > > +static bool canfd_decode_reg_bank(XlnxVersalCANFDState *s, hwaddr addr, > + hwaddr first_reg, hwaddr last_reg, > + size_t num_banks, size_t *idx, size_t > *offset) > +{ > + hwaddr base = addr - first_reg; > + hwaddr span = last_reg - first_reg + sizeof(uint32_t); > + > + *idx = base / span; > + > + if (*idx >= num_banks) { > + return false; > + } > + > + *offset = base % span; > + *offset += first_reg; > + > + return true; > +} > + > +/* > + * Decode the given addr into a (idx, offset) pair: > + * - idx is the bank index of the register at addr, > + * - offset is the register offset relative to bank 0 > + * > + * @return true is the decoding succeded, false otherwise > + */ > +static bool canfd_decode_addr(XlnxVersalCANFDState *s, hwaddr addr, > + size_t *idx, size_t *offset) > +{ > + if (addr <= A_RX_FIFO_WATERMARK_REGISTER) { > + /* from 0x0 to 0xec. Handled by the register API */ > + g_assert_not_reached(); > + } else if (addr < A_TB_ID_REGISTER) { > + /* no register in this gap */ > + return false; > + } else if (addr < A_AFMR_REGISTER) { > + /* TX registers */ > + return canfd_decode_reg_bank(s, addr, > + A_TB_ID_REGISTER, A_TB_DW15_REGISTER, > + s->cfg.tx_fifo, idx, offset); > + } else if (addr < A_TXE_FIFO_TB_ID_REGISTER) { > + /* Filter registers */ > + return canfd_decode_reg_bank(s, addr, > + A_AFMR_REGISTER, A_AFIR_REGISTER, > + 32, idx, offset); > + } else if (addr < A_RB_ID_REGISTER) { > + /* TX event registers */ > + return canfd_decode_reg_bank(s, addr, > + A_TXE_FIFO_TB_ID_REGISTER, > + A_TXE_FIFO_TB_DLC_REGISTER, > + 32, idx, offset); > + } else if (addr < A_RB_ID_REGISTER_1) { > + /* RX0 registers */ > + return canfd_decode_reg_bank(s, addr, > + A_RB_ID_REGISTER, > + A_RB_DW15_REGISTER, > + s->cfg.rx0_fifo, idx, offset); > + } else if (addr <= A_RB_DW15_REGISTER_1) { > + /* RX1 registers */ > + return canfd_decode_reg_bank(s, addr, > + A_RB_ID_REGISTER_1, > + A_RB_DW15_REGISTER_1, > + s->cfg.rx1_fifo, idx, offset); > + } > + > + /* decode error */ > + return false; > +} > + > +static uint64_t canfd_read(void *opaque, hwaddr addr, unsigned size) > +{ > + XlnxVersalCANFDState *s = XILINX_CANFD(opaque); > + size_t bank_idx; > + hwaddr reg_offset; > + uint64_t ret; > + > + if (!canfd_decode_addr(s, addr, &bank_idx, ®_offset)) { > + qemu_log_mask(LOG_GUEST_ERROR, TYPE_XILINX_CANFD > + ": read to unknown register at address 0x%" > + HWADDR_PRIx "\n", addr); > + return 0; > + } > + > + switch (reg_offset) { > + default: > + ret = s->regs[addr / sizeof(uint32_t)]; > + } > + > + return ret; > +} > + > +static void canfd_write(void *opaque, hwaddr addr, uint64_t value, > + unsigned size) > +{ > + XlnxVersalCANFDState *s = XILINX_CANFD(opaque); > + size_t bank_idx; > + hwaddr reg_offset; > + > + if (!canfd_decode_addr(s, addr, &bank_idx, ®_offset)) { > + qemu_log_mask(LOG_GUEST_ERROR, TYPE_XILINX_CANFD > + ": write to unknown register at address 0x%" > + HWADDR_PRIx "\n", addr); > + return; > + } > + > + if (addr >= A_TXE_FIFO_TB_ID_REGISTER) { > + /* All registers from TX event regs to the end are read-only */ > + qemu_log_mask(LOG_GUEST_ERROR, TYPE_XILINX_CANFD > + ": write to read-only register at 0x%" HWADDR_PRIx > "\n", > + addr); > + return; > + } > + > + switch (reg_offset) { > + case A_AFMR_REGISTER: > + case A_AFIR_REGISTER: > + filter_reg_write(s, addr, bank_idx, value); > + break; > + > + default: > + s->regs[addr / sizeof(uint32_t)] = value; > + } > +} > + > static const MemoryRegionOps canfd_ops = { > + .read = canfd_read, > + .write = canfd_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > +}; > + > +static const MemoryRegionOps canfd_regs_ops = { > .read = register_read_memory, > .write = register_write_memory, > .endianness = DEVICE_LITTLE_ENDIAN, > .valid = { > .min_access_size = 4, > @@ -2018,12 +2169,12 @@ static void canfd_realize(DeviceState *dev, Error > **errp) > > static void canfd_init(Object *obj) > { > XlnxVersalCANFDState *s = XILINX_CANFD(obj); > > - memory_region_init(&s->iomem, obj, TYPE_XILINX_CANFD, > - XLNX_VERSAL_CANFD_R_MAX * 4); > + memory_region_init_io(&s->iomem, obj, &canfd_ops, s, TYPE_XILINX_CANFD, > + XLNX_VERSAL_CANFD_R_MAX * 4); > } > > static const VMStateDescription vmstate_canfd = { > .name = TYPE_XILINX_CANFD, > .version_id = 1, > -- > 2.50.1 >
