We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
strings for uint64_t and hwaddr types.
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
hw/riscv/virt.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 036a0a9bfb..075c035f25 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
size = riscv_socket_mem_size(ms, socket);
- mem_name = g_strdup_printf("/memory@%lx", (long)addr);
+ mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
qemu_fdt_add_subnode(ms->fdt, mem_name);
qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
addr >> 32, addr, size >> 32, size);
@@ -880,8 +880,8 @@ static void create_fdt_pcie(RISCVVirtState *s,
g_autofree char *name = NULL;
MachineState *ms = MACHINE(s);
- name = g_strdup_printf("/soc/pci@%lx",
- (long) s->memmap[VIRT_PCIE_ECAM].base);
+ name = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+ s->memmap[VIRT_PCIE_ECAM].base);
qemu_fdt_setprop_cell(ms->fdt, name, "#address-cells",
FDT_PCI_ADDR_CELLS);
qemu_fdt_setprop_cell(ms->fdt, name, "#interrupt-cells",
@@ -925,8 +925,8 @@ static void create_fdt_reset(RISCVVirtState *s, uint32_t *phandle)
MachineState *ms = MACHINE(s);
test_phandle = (*phandle)++;
- name = g_strdup_printf("/soc/test@%lx",
- (long)s->memmap[VIRT_TEST].base);
+ name = g_strdup_printf("/soc/test@%"HWADDR_PRIx,
+ s->memmap[VIRT_TEST].base);
qemu_fdt_add_subnode(ms->fdt, name);
{
static const char * const compat[3] = {
@@ -964,8 +964,8 @@ static void create_fdt_uart(RISCVVirtState *s,
g_autofree char *name = NULL;
MachineState *ms = MACHINE(s);
- name = g_strdup_printf("/soc/serial@%lx",
- (long)s->memmap[VIRT_UART0].base);
+ name = g_strdup_printf("/soc/serial@%"HWADDR_PRIx,
+ s->memmap[VIRT_UART0].base);
qemu_fdt_add_subnode(ms->fdt, name);
qemu_fdt_setprop_string(ms->fdt, name, "compatible", "ns16550a");
qemu_fdt_setprop_cells(ms->fdt, name, "reg",
@@ -989,7 +989,8 @@ static void create_fdt_rtc(RISCVVirtState *s,
g_autofree char *name = NULL;
MachineState *ms = MACHINE(s);
- name = g_strdup_printf("/soc/rtc@%lx", (long)s->memmap[VIRT_RTC].base);
+ name = g_strdup_printf("/soc/rtc@%"HWADDR_PRIx,
+ s->memmap[VIRT_RTC].base);
qemu_fdt_add_subnode(ms->fdt, name);
qemu_fdt_setprop_string(ms->fdt, name, "compatible",
"google,goldfish-rtc");
@@ -1042,8 +1043,8 @@ static void create_fdt_virtio_iommu(RISCVVirtState *s, uint16_t bdf)
g_autofree char *iommu_node = NULL;
g_autofree char *pci_node = NULL;
- pci_node = g_strdup_printf("/soc/pci@%lx",
- (long) s->memmap[VIRT_PCIE_ECAM].base);
+ pci_node = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+ s->memmap[VIRT_PCIE_ECAM].base);
iommu_node = g_strdup_printf("%s/virtio_iommu@%x,%x", pci_node,
PCI_SLOT(bdf), PCI_FUNC(bdf));
iommu_phandle = qemu_fdt_alloc_phandle(fdt);
@@ -1111,8 +1112,8 @@ static void create_fdt_iommu(RISCVVirtState *s, uint16_t bdf)
g_autofree char *iommu_node = NULL;
g_autofree char *pci_node = NULL;
- pci_node = g_strdup_printf("/soc/pci@%lx",
- (long) s->memmap[VIRT_PCIE_ECAM].base);
+ pci_node = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+ s->memmap[VIRT_PCIE_ECAM].base);
iommu_node = g_strdup_printf("%s/iommu@%x", pci_node, bdf);
iommu_phandle = qemu_fdt_alloc_phandle(fdt);
qemu_fdt_add_subnode(fdt, iommu_node);
@@ -1181,8 +1182,8 @@ static void create_fdt(RISCVVirtState *s)
* The "/soc/pci@..." node is needed for PCIE hotplugs
* that might happen before finalize_fdt().
*/
- name = g_strdup_printf("/soc/pci@%lx",
- (long) s->memmap[VIRT_PCIE_ECAM].base);
+ name = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+ s->memmap[VIRT_PCIE_ECAM].base);
qemu_fdt_add_subnode(ms->fdt, name);
qemu_fdt_add_subnode(ms->fdt, "/chosen");
--
2.49.0On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
> strings for uint64_t and hwaddr types.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> hw/riscv/virt.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 036a0a9bfb..075c035f25 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>
> addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
> size = riscv_socket_mem_size(ms, socket);
> - mem_name = g_strdup_printf("/memory@%lx", (long)addr);
> + mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
I wondered why this wasn't a HWADDR_PRIx.
addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
everything more consistent.
Cheers,
Joel
Joel,
I'll make these changes in this patch to be consistent with what we've
been discussing:
- change addr to hwaddr
- use HWADDR_PRIx instead of PRIx64
i.e. this diff:
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 1eae84db15..0020d8f404 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -303,12 +303,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
{
g_autofree char *mem_name = NULL;
- uint64_t addr, size;
+ hwaddr addr;
+ uint64_t size;
MachineState *ms = MACHINE(s);
addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
size = riscv_socket_mem_size(ms, socket);
- mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
+ mem_name = g_strdup_printf("/memory@%"HWADDR_PRIx, addr);
qemu_fdt_add_subnode(ms->fdt, mem_name);
qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
addr >> 32, addr, size >> 32, size);
I did this change and applied your "hw/riscv/virt: device tree reg cleanups" series on top
of it, and there are no conflicts. No change needed in your side.
Thanks,
Daniel
On 4/24/25 6:41 AM, Joel Stanley wrote:
> On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
>> strings for uint64_t and hwaddr types.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>> hw/riscv/virt.c | 29 +++++++++++++++--------------
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 036a0a9bfb..075c035f25 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>>
>> addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>> size = riscv_socket_mem_size(ms, socket);
>> - mem_name = g_strdup_printf("/memory@%lx", (long)addr);
>> + mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
>
> I wondered why this wasn't a HWADDR_PRIx.
>
> addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
> everything more consistent.
>
> Cheers,
>
> Joel
On 4/29/25 9:40 AM, Daniel Henrique Barboza wrote:
> Joel,
>
> I'll make these changes in this patch to be consistent with what we've
> been discussing:
>
> - change addr to hwaddr
> - use HWADDR_PRIx instead of PRIx64
>
> i.e. this diff:
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 1eae84db15..0020d8f404 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -303,12 +303,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
> {
> g_autofree char *mem_name = NULL;
> - uint64_t addr, size;
> + hwaddr addr;
> + uint64_t size;
> MachineState *ms = MACHINE(s);
>
> addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
> size = riscv_socket_mem_size(ms, socket);
> - mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> + mem_name = g_strdup_printf("/memory@%"HWADDR_PRIx, addr);
> qemu_fdt_add_subnode(ms->fdt, mem_name);
> qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
> addr >> 32, addr, size >> 32, size);
>
>
> I did this change and applied your "hw/riscv/virt: device tree reg cleanups" series on top
> of it, and there are no conflicts. No change needed in your side.
It seems I was wrong. The v2 will conflict with your patch 03. I think a rebase from
your series can't be avoided ...
Daniel
>
>
> Thanks,
>
> Daniel
>
>
>
> On 4/24/25 6:41 AM, Joel Stanley wrote:
>> On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
>>> strings for uint64_t and hwaddr types.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>> hw/riscv/virt.c | 29 +++++++++++++++--------------
>>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> index 036a0a9bfb..075c035f25 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>>>
>>> addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>>> size = riscv_socket_mem_size(ms, socket);
>>> - mem_name = g_strdup_printf("/memory@%lx", (long)addr);
>>> + mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
>>
>> I wondered why this wasn't a HWADDR_PRIx.
>>
>> addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
>> everything more consistent.
>>
>> Cheers,
>>
>> Joel
>
On Wed, 30 Apr 2025 at 02:41, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 4/29/25 9:40 AM, Daniel Henrique Barboza wrote:
> > Joel,
> >
> > I'll make these changes in this patch to be consistent with what we've
> > been discussing:
> >
> > - change addr to hwaddr
> > - use HWADDR_PRIx instead of PRIx64
> >
> > i.e. this diff:
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 1eae84db15..0020d8f404 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -303,12 +303,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> > static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
> > {
> > g_autofree char *mem_name = NULL;
> > - uint64_t addr, size;
> > + hwaddr addr;
> > + uint64_t size;
Size should be a hwaddr too. This would be consistent with how
MemMapEntry describes the base/size pairs.
> > MachineState *ms = MACHINE(s);
> >
> > addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
> > size = riscv_socket_mem_size(ms, socket);
> > - mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> > + mem_name = g_strdup_printf("/memory@%"HWADDR_PRIx, addr);
> > qemu_fdt_add_subnode(ms->fdt, mem_name);
> > qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
> > addr >> 32, addr, size >> 32, size);
> >
> >
> > I did this change and applied your "hw/riscv/virt: device tree reg cleanups" series on top
> > of it, and there are no conflicts. No change needed in your side.
>
> It seems I was wrong. The v2 will conflict with your patch 03. I think a rebase from
> your series can't be avoided ...
If you want to pick them up as part of your series, and send them as a
big patch set then that's fine with me.
Otherwise I'll wait until we've got yours staged and send a new version out.
Thanks!
Joel
On 4/24/25 6:41 AM, Joel Stanley wrote:
> On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
>> strings for uint64_t and hwaddr types.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>> hw/riscv/virt.c | 29 +++++++++++++++--------------
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 036a0a9bfb..075c035f25 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>>
>> addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>> size = riscv_socket_mem_size(ms, socket);
>> - mem_name = g_strdup_printf("/memory@%lx", (long)addr);
>> + mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
>
> I wondered why this wasn't a HWADDR_PRIx.
>
> addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
> everything more consistent.
I didn't put too much thought about it in this patch. I saw that 'addr' was an
uint64_t and just matched the format string.
As for whether this 'addr' var and NodeInfo::node_mem could be a hwaddr, at first
glance I don't see why not. There are lots of 'hwaddr' references in the memory API
(memory_region_* functions, address_space* functions, etc) so using hwaddr in the
memory context is valid.
If we want to go further, the patch that introduced hwaddr in QEMU (commit a8170e5e)
states:
---------
Rename target_phys_addr_t to hwaddr
target_phys_addr_t is unwieldly, violates the C standard (_t suffixes are
reserved) and its purpose doesn't match the name (most target_phys_addr_t
addresses are not target specific). Replace it with a finger-friendly,
standards conformant hwaddr.
---------
So it seems to me that the idea is to have an abstraction of target independent
physical addresses, and memory is a good example of that. I believe we're not
making full use of hwaddr and overusing uint64_t. Thanks,
Daniel
>
> Cheers,
>
> Joel
On Fri, 25 Apr 2025 at 22:03, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 4/24/25 6:41 AM, Joel Stanley wrote:
> > On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
> >> strings for uint64_t and hwaddr types.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >> hw/riscv/virt.c | 29 +++++++++++++++--------------
> >> 1 file changed, 15 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> index 036a0a9bfb..075c035f25 100644
> >> --- a/hw/riscv/virt.c
> >> +++ b/hw/riscv/virt.c
> >> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
> >>
> >> addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
> >> size = riscv_socket_mem_size(ms, socket);
> >> - mem_name = g_strdup_printf("/memory@%lx", (long)addr);
> >> + mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> >
> > I wondered why this wasn't a HWADDR_PRIx.
> >
> > addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
> > everything more consistent.
>
> I didn't put too much thought about it in this patch. I saw that 'addr' was an
> uint64_t and just matched the format string.
>
> As for whether this 'addr' var and NodeInfo::node_mem could be a hwaddr, at first
> glance I don't see why not. There are lots of 'hwaddr' references in the memory API
> (memory_region_* functions, address_space* functions, etc) so using hwaddr in the
> memory context is valid.
>
> If we want to go further, the patch that introduced hwaddr in QEMU (commit a8170e5e)
> states:
>
> ---------
> Rename target_phys_addr_t to hwaddr
>
> target_phys_addr_t is unwieldly, violates the C standard (_t suffixes are
> reserved) and its purpose doesn't match the name (most target_phys_addr_t
> addresses are not target specific). Replace it with a finger-friendly,
> standards conformant hwaddr.
> ---------
>
> So it seems to me that the idea is to have an abstraction of target independent
> physical addresses, and memory is a good example of that. I believe we're not
> making full use of hwaddr and overusing uint64_t. Thanks,
Cool. I've got a little series that I'll post. It cleans this one up,
and cleans up some other device tree things.
Cheers,
Joel
On 23/4/25 13:06, Daniel Henrique Barboza wrote: > We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt > strings for uint64_t and hwaddr types. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > hw/riscv/virt.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2025 Red Hat, Inc.