Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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