On 4/24/25 6:41 AM, Joel Stanley wrote:
On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza <dbarb...@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 <dbarb...@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
