On Thu, 15 Oct 2026 at 19:11, Marcelo Manzo <[email protected]> wrote:
>
> From: Sergey Kambalin <[email protected]>
>
> Replace the stub MMIO read/write handlers with real dispatch over
> the register offsets from the previous patches: SYS revision
> control, INTRL0/1 status/mask registers, UMAC command/MAC-address/
> MDIO-command registers, and TDMA/RDMA ring configuration, wired up
> to the NIC's realize()/reset() and qdev NIC properties
> (DEFINE_NIC_PROPERTIES) so the device gets a MAC address and peer
> netdev from the command line.
>
> Part of Sergey Kambalin's original Raspberry Pi 4B PCIe/GENET
> networking series (patchwork series 829638, posted to qemu-devel in
> 2024, never merged). Carried forward and completed by Marcelo Manzo:
> adapted to the current Property/class_init/Resettable APIs, with no
> functional change to the register dispatch logic itself.
>
> Signed-off-by: Sergey Kambalin <[email protected]>
> Signed-off-by: Marcelo Manzo <[email protected]>
> static uint64_t bcm2838_genet_read(void *opaque, hwaddr offset, unsigned
> size)
> {
> uint64_t value = ~0;
> + BCM2838GenetState *s = opaque;
>
> - qemu_log_mask(
> - LOG_GUEST_ERROR,
> - "%s: out-of-range access, %u bytes @ offset 0x%04" PRIx64 "\n",
> - __func__, size, offset);
> + if (offset + size < sizeof(s->regs)) {
> + memcpy(&value, (uint8_t *)&s->regs + offset, size);
This has endianness problems, because not everything in s->regs
is a host uint32_t. It works for the uint32_t fields because we
force that only 4-byte accesses are valid and 'value' here is
in host order, but in e.g. BCM2838GenetRegsHfb the flt[] array
is uint8_t. (Plus it looks on the face of it like it will handle
an arbitrary offset/size but it won't work on big-endian hosts
for anything except 4/4.) Similarly for the write memcpy.
This kind of thing is why devices tend to either
(a) have a uint32_t regs[] array, and use "s->regs[offset >> 2] = value"
as their default "set the register", or
(b) have structs where the fields are named after the registers, and
have switch(offset) statements which handle each register case
separately with "s->myfield = value", and don't try to have a default
case that handles most registers.
> + } else {
> + qemu_log_mask(
> + LOG_GUEST_ERROR,
> + "%s: out-of-range access, %u bytes @ offset 0x%04" PRIx64 "\n",
> + __func__, size, offset);
> + }
>
> trace_bcm2838_genet_read(size, offset, value);
> return value;
> }
>
> static void bcm2838_genet_write(void *opaque, hwaddr offset, uint64_t value,
> - unsigned size) {
> - qemu_log_mask(
> - LOG_GUEST_ERROR,
> - "%s: out-of-range access, %u bytes @ offset 0x%04" PRIx64 "\n",
> - __func__, size, offset);
> + unsigned size)
> +{
> + BCM2838GenetState *s = opaque;
> + MACAddr *mac = &s->nic_conf.macaddr;
> + NetClientState *ncs = qemu_get_queue(s->nic);
> +
> + trace_bcm2838_genet_write(size, offset, value);
> +
> + if (offset + size < sizeof(s->regs)) {
> + switch (offset) {
> + case BCM2838_GENET_INTRL0_SET:
> + s->regs.intrl0.stat |= value;
> + break;
> + case BCM2838_GENET_INTRL0_CLEAR:
> + s->regs.intrl0.stat &= ~value;
> + break;
> + case BCM2838_GENET_INTRL0_MASK_SET:
> + s->regs.intrl0.mask_status |= value;
> + break;
> + case BCM2838_GENET_INTRL0_MASK_CLEAR:
> + s->regs.intrl0.mask_status &= ~value;
> + break;
> + case BCM2838_GENET_INTRL1_SET:
> + s->regs.intrl1.stat |= value;
> + break;
> + case BCM2838_GENET_INTRL1_CLEAR:
> + s->regs.intrl1.stat &= ~value;
> + break;
> + case BCM2838_GENET_INTRL1_MASK_SET:
> + s->regs.intrl1.mask_status |= value;
> + break;
> + case BCM2838_GENET_INTRL1_MASK_CLEAR:
> + s->regs.intrl1.mask_status &= ~value;
> + break;
> + case BCM2838_GENET_UMAC_CMD:
> + /* Complete SW reset as soon as it has been requested */
> + if (FIELD_EX32(value, GENET_UMAC_CMD, SW_RESET) == 1) {
> + device_cold_reset(DEVICE(s));
> + value = FIELD_DP32(value, GENET_UMAC_CMD, SW_RESET, 0);
> + }
> + break;
> + /*
> + * TODO: before changing MAC address we'd better inform QEMU
> + * network subsystem about freeing previously used one, but
> + * qemu_macaddr_set_free function isn't accessible for us (marked
> + * as static in net/net.c), see also https://lists.nongnu.org/
> + * archive/html/qemu-devel/2022-07/msg02123.html
> + */
> + case BCM2838_GENET_UMAC_MAC0:
> + mac->a[0] = FIELD_EX32(value, GENET_UMAC_MAC_0, ADDR_0);
> + mac->a[1] = FIELD_EX32(value, GENET_UMAC_MAC_0, ADDR_1);
> + mac->a[2] = FIELD_EX32(value, GENET_UMAC_MAC_0, ADDR_2);
> + mac->a[3] = FIELD_EX32(value, GENET_UMAC_MAC_0, ADDR_3);
> + qemu_macaddr_default_if_unset(mac);
No other network device uses this function like this. This is the
function for "in your realize method, if the user didn't specify
a MAC address via config, let the net code pick one for them".
Maybe (as per the linked email thread) the APIs here could be
improved, but a new device model isn't the place to start doing
something that no other device is doing. The standard thing seems
to be that we call qemu_format_nic_info_str() with the new MAC and
call that good enough.
If we want to fix bugs in the MAC address APIs we should do that
as a separate patchset that proposes its change and implements it
in the existing device models.
> + qemu_format_nic_info_str(ncs, mac->a);
> + trace_bcm2838_genet_mac_address(ncs->info_str);
> + break;
> + case BCM2838_GENET_UMAC_MAC1:
> + mac->a[4] = FIELD_EX32(value, GENET_UMAC_MAC_1, ADDR_4);
> + mac->a[5] = FIELD_EX32(value, GENET_UMAC_MAC_1, ADDR_5);
> + qemu_macaddr_default_if_unset(mac);
> + qemu_format_nic_info_str(ncs, mac->a);
> + trace_bcm2838_genet_mac_address(ncs->info_str);
> + break;
thanks
-- PMM
