Hi Daniel,
On Fri, Aug 28, 2026 at 12:40:38PM +0800, Daniel Henrique Barboza wrote:
>
>
> On 8/28/2026 1:13 AM, Chao Liu wrote:
> > On Thu, Aug 27, 2026 at 07:26:01PM +0800, Daniel Henrique Barboza wrote:
> > > Yet another FDT that we want to move to a helper to avoid copy/pasting
> > > code to other boards that will use a mtimer. In particular the future
> > > 'riscv-server-ref' board.
> > >
> > > No FDT changes made.
> > >
> > > Signed-off-by: Daniel Henrique Barboza <[email protected]>
> > > Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
> > > ---
> > > hw/riscv/fdt-common.c | 96 ++++++++++++++++++++++++++++++++
> > > hw/riscv/virt.c | 101 ++++------------------------------
> > > include/hw/riscv/fdt-common.h | 11 ++++
> > > 3 files changed, 119 insertions(+), 89 deletions(-)
> > >
> > > diff --git a/hw/riscv/fdt-common.c b/hw/riscv/fdt-common.c
> > > index d815c5adff..76a783709d 100644
> > > --- a/hw/riscv/fdt-common.c
> > > +++ b/hw/riscv/fdt-common.c
> > > @@ -16,6 +16,7 @@
> > > #include "target/riscv/cpu_bits.h"
> > > #include "hw/riscv/riscv-iommu-bits.h"
> > > #include "hw/riscv/iommu.h"
> > > +#include "hw/intc/riscv_aclint.h"
> > > #include "hw/intc/riscv_imsic.h"
> > > #include "hw/pci/pci.h"
> > > #include "hw/pci/pcie_host.h"
> > > @@ -702,3 +703,98 @@ void riscv_create_fdt_socket_aplic(void *fdt,
> > > APLICFdtProps *props,
> > > aplic_phandles[props->socket] = aplic_s_phandle;
> > > }
> > > +
> > > +void riscv_create_fdt_socket_aclint(void *fdt, ACLINTFdtProps *props,
> > > + uint32_t *intc_phandles)
> > > +{
> > > + uint32_t aclint_cells_size = props->num_harts * sizeof(uint32_t) * 2;
> > > + g_autofree uint32_t *aclint_mswi_cells = NULL;
> > > + g_autofree uint32_t *aclint_sswi_cells = NULL;
> > > + g_autofree uint32_t *aclint_mtimer_cells = NULL;
> > > + hwaddr addr, size;
> > > + char *name;
> > > + int cpu;
> > > +
> > > + aclint_mswi_cells = g_new0(uint32_t, props->num_harts * 2);
> > > + aclint_mtimer_cells = g_new0(uint32_t, props->num_harts * 2);
> > > + aclint_sswi_cells = g_new0(uint32_t, props->num_harts * 2);
> > > +
> > > + for (cpu = 0; cpu < props->num_harts; cpu++) {
> > > + aclint_mswi_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > > + aclint_mswi_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_SOFT);
> > > + aclint_mtimer_cells[cpu * 2 + 0] =
> > > cpu_to_be32(intc_phandles[cpu]);
> > > + aclint_mtimer_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_TIMER);
> > > + aclint_sswi_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > > + aclint_sswi_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_S_SOFT);
> > > + }
> > > +
> > > + if (props->aia_type != AIA_TYPE_APLIC_IMSIC) {
> > > + addr = props->clint->base + (props->clint->size * props->socket);
> > > + name = g_strdup_printf("/soc/mswi@%"HWADDR_PRIx, addr);
> > > +
> > > + qemu_fdt_add_subnode(fdt, name);
> > > + qemu_fdt_setprop_string(fdt, name, "compatible",
> > > "riscv,aclint-mswi");
> > > + qemu_fdt_setprop_sized_cells(fdt, name, "reg",
> > > + 2, addr, 2, RISCV_ACLINT_SWI_SIZE);
> > > + qemu_fdt_setprop(fdt, name, "interrupts-extended",
> > > + aclint_mswi_cells, aclint_cells_size);
> > > + qemu_fdt_setprop(fdt, name, "interrupt-controller", NULL, 0);
> > > + qemu_fdt_setprop_cell(fdt, name, "#interrupt-cells", 0);
> > > +
> > > + if (props->numa_enabled) {
> > > + qemu_fdt_setprop_cell(fdt, name, "numa-node-id",
> > > props->socket);
> > > + }
> > > +
> > > + g_free(name);
> > > + }
> > > +
> > > + if (props->aia_type == AIA_TYPE_APLIC_IMSIC) {
> > > + addr = props->clint->base +
> > > + (RISCV_ACLINT_DEFAULT_MTIMER_SIZE * props->socket);
> > > + size = RISCV_ACLINT_DEFAULT_MTIMER_SIZE;
> > > + } else {
> > > + addr = props->clint->base + RISCV_ACLINT_SWI_SIZE +
> > > + (props->clint->size * props->socket);
> > > + size = props->clint->size - RISCV_ACLINT_SWI_SIZE;
> > > + }
> > > +
> > > + name = g_strdup_printf("/soc/mtimer@%"HWADDR_PRIx, addr);
> > > + qemu_fdt_add_subnode(fdt, name);
> > > + qemu_fdt_setprop_string(fdt, name, "compatible",
> > > + "riscv,aclint-mtimer");
> > > + qemu_fdt_setprop_sized_cells(fdt, name, "reg",
> > > + 2, addr,
> > Should be:
> > 2, addr + RISCV_ACLINT_DEFAULT_MTIME,
> >
> > I found an MTIMER FDT issue in the virt machine FDT.
> >
> > Command:
> >
> > ```
> > timeout 30s ./build/qemu-system-riscv64 \
> > -machine
> > virt,aia=aplic-imsic,aia-guests=5,aclint=on,dumpdtb=/tmp/virt.dtb \
> > -smp 2 -display none -nodefaults
> >
> > dtc -I dtb -O dts /tmp/virt.dtb | grep -A4 mtimer@
> > ```
> >
> > The generated FDT contains:
> >
> > mtimer@2000000 {
> > interrupts-extended = <0x04 0x07 0x02 0x07>;
> > reg = <0x00 0x2000000 0x00 0x08 0x00 0x2000000
> > 0x00 0x7ff8>;
> > compatible = "riscv,aclint-mtimer";
> > };
> >
> > without this patches:
> >
> > mtimer@2000000 {
> > interrupts-extended = <0x04 0x07 0x02 0x07>;
> > reg = <0x00 0x2007ff8 0x00 0x08 0x00 0x2000000
> > 0x00 0x7ff8>;
> > compatible = "riscv,aclint-mtimer";
> > };
> >
> > The first address should be 0x20007ff8.
>
> That was intentional and I forgot to document it in the commit msg. The
> "No FDT changes made" I said doesn't apply here. My bad!
Thanks for clarifying. I also noticed this waring during my previous
review and testing.
>
> If you use 'dtc' in the current 'virt' DT you'll get warnings:
>
> dtc -I dtb -O dts virt.dtb > virt.dts
>
> <stdout>: Warning (simple_bus_reg): /soc/mtimer@2000000: simple-bus unit
> address format error, expected "2007ff8"
> <stdout>: Warning (simple_bus_reg): /soc/mtimer@2008000: simple-bus unit
> address format error, expected "200fff8"
Yes, the warning is valid, and I can reproduce it as well.
>
>
> This happens because the unit address in the node name doesn't match the
> first address in 'reg'. For mtimer@2000000, "reg" must be reg = <0x00
> 0x2000000 (...)>.
> For mtimer@2008000, reg = <0x00 2008000 (...)> and so on.
I agree that the address in the node name should match the first address
in `reg`, as described by the Devicetree Spec [1].
However, I think the value that needs to change is the node name, not
the first address in `reg`.
See ACLINT spec, an MITER device has two separate base addr: one for MTIME
and another for the MTIMECMP registers [2].
In the current QEMU implement, `addr` is the MTIMECMP base address, while
the MTIME register is located at:
addr + RISCV_ACLINT_DEFAULT_METIME
For this example:
MTIMECMP base = 0x2000000
MITIME base = 0x2007ff8
So the existing `reg` property correctly describes the two register
regions:
reg = <0x00 0x2007ff8 0x00 0x08
0x00 0x2000000 0x00 0x7ff8>
>
>
> Now, does this change break aclint in 'virt'? Because in that case we'll
> just keep the "dtc" warning and live with it. Aside from the existing tests
> we have in QEMU I didn't do a deep dive in aclint.
Yes, I think using `addr` here would make the ACLINT description
incorrect. `addr` is the MTIMECMP base address, while MTIME is located
at `addr + RISCV_ACLINT_DEFAULT_MTIME`.
Please keep the first `reg` address as it was:
qemu_fdt_setprop_sized_cells(fdt, name, "reg",
- 2, addr,
+ 2, addr + RISCV_ACLINT_DEFAULT_MTIME,
2, size - RISCV_ACLINT_DEFAULT_MTIME,
2, addr + RISCV_ACLINT_DEFAULT_MTIMECMP,
2, RISCV_ACLINT_DEFAULT_MTIME);
Since this patch is mainly a code-duplication cleanup, I suggest
leaving the generated FDT unchanged here. The unit-address fix can be
sent as a separate follow-up patch:
- name = g_strdup_printf("/soc/mtimer@%" HWADDR_PRIx, addr);
+ name = g_strdup_printf("/soc/mtimer@%" HWADDR_PRIx,
+ addr + RISCV_ACLINT_DEFAULT_MTIME);
--
LINK:
[1] Devicetree Specification, Section 2.2.1, "Node Names":
https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#node-names
[2] RISC-V ACLINT Specification, "Machine-level Timer Device (MTIMER)",
Section "Register Map":
https://github.com/riscvarchive/riscv-aclint/blob/main/riscv-aclint.adoc
Thanks,
Chao
>
>
> Thanks,
> Daniel
>
> >
> > > + 2, size - RISCV_ACLINT_DEFAULT_MTIME,
> > > + 2, addr + RISCV_ACLINT_DEFAULT_MTIMECMP,
> > > + 2, RISCV_ACLINT_DEFAULT_MTIME);
> >
> > [...]
> >
> > > + qemu_fdt_setprop(fdt, name, "interrupts-extended",
> > > - qemu_fdt_setprop_sized_cells(ms->fdt, name, "reg",
> > > - 2, addr + RISCV_ACLINT_DEFAULT_MTIME,
> > I think this is the original code that was moved here.
> >
> > Thanks,
> > Chao
> > > - 2, size - RISCV_ACLINT_DEFAULT_MTIME,
> > > - 2, addr + RISCV_ACLINT_DEFAULT_MTIMECMP,
> > > - 2, RISCV_ACLINT_DEFAULT_MTIME);
> >
>
