hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

PATCH Superseded Carlo Marcelo Arenas Belon 4 participants 10 messages Nov 11, 2007 – Nov 30, 2007

lore

Share & tools

lore

Carlo Marcelo Arenas Belon

author

2007-11-11 15:44 UTC #0 lore raw

PATCH 1/1 hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

The following patch changes the formatting string from %08x to TARGET_FMT_plx to accommodate for 64bit hosts.

Carlo

---
Index: hw/sh7750.c
===================================================================
RCS file: /sources/qemu/qemu/hw/sh7750.c,v
retrieving revision 1.9

sh7750.c +2 -2

diff -u -r1.9 sh7750.c
@@ -180,13 +180,13 @@
 
 static void error_access(const char *kind, target_phys_addr_t addr)
 {
-    fprintf(stderr, "%s to %s (0x%08x) not supported\n",
+    fprintf(stderr, "%s to %s (0x" TARGET_FMT_plx ") not supported\n",
 	    kind, regname(addr), addr);
 }
 
 static void ignore_access(const char *kind, target_phys_addr_t addr)
 {
-    fprintf(stderr, "%s to %s (0x%08x) ignored\n",
+    fprintf(stderr, "%s to %s (0x" TARGET_FMT_plx ") ignored\n",
 	    kind, regname(addr), addr);
 }

Carlo Marcelo Arenas Belon

author

2007-11-18 21:13 UTC #1 lore raw

PATCH RESEND 1/1 hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

The following patch changes the formatting string from %08x to TARGET_FMT_plx to accommodate for compilation in 64bit hosts and that manifests with the following warning :

qemu/hw/sh7750.c: In function `error_access':
qemu/hw/sh7750.c:186: warning: unsigned int format, different type arg (arg 5) qemu/hw/sh7750.c: In function `ignore_access':
qemu/hw/sh7750.c:192: warning: unsigned int format, different type arg (arg 5)

Carlo

---
Index: sh7750.c
===================================================================
RCS file: /sources/qemu/qemu/hw/sh7750.c,v
retrieving revision 1.11

sh7750.c +2 -2

diff -u -r1.11 sh7750.c
@@ -182,13 +182,13 @@
 
 static void error_access(const char *kind, target_phys_addr_t addr)
 {
-    fprintf(stderr, "%s to %s (0x%08x) not supported\n",
+    fprintf(stderr, "%s to %s (0x" TARGET_FMT_plx ") not supported\n",
 	    kind, regname(addr), addr);
 }
 
 static void ignore_access(const char *kind, target_phys_addr_t addr)
 {
-    fprintf(stderr, "%s to %s (0x%08x) ignored\n",
+    fprintf(stderr, "%s to %s (0x" TARGET_FMT_plx ") ignored\n",
 	    kind, regname(addr), addr);
 }

Magnus Damm

2007-11-30 05:36 UTC #2 lore raw

Quoted 8 lines · Carlo Marcelo Arenas Belon

The following patch changes the formatting string from %08x to TARGET_FMT_plx to accommodate for compilation in 64bit hosts and that manifests with the following warning :
qemu/hw/sh7750.c: In function `error_access':
qemu/hw/sh7750.c:186: warning: unsigned int format, different type arg (arg 5) qemu/hw/sh7750.c: In function `ignore_access':
qemu/hw/sh7750.c:192: warning: unsigned int format, different type arg (arg 5)

This patch works fine on 32 bit x86 hosts. Please apply.

sh7750.c 18 Nov 2007 21:08:37 -0000

Quoted 7 lines

Index: sh7750.c
===================================================================
RCS file: /sources/qemu/qemu/hw/sh7750.c,v
retrieving revision 1.11
diff -u -r1.11 sh7750.c
sh7750.c 18 Nov 2007 21:08:37 -0000 renamed from sh7750.c 17 Nov 2007 17:14:48 -0000 1.11 +0 -0

Could you please create the diff from the top level directory next time? That way it can be applied with patch -p0 or -p1 directly in the top level directory which makes patch handling much easier. Thanks!

/ magnus

Carlo Marcelo Arenas Belon

author

2007-11-30 15:15 UTC #3 lore raw

Quoted 6 lines · Magnus Damm

Quoted 4 lines · Carlo Marcelo Arenas Belon
The following patch changes the formatting string from %08x to TARGET_FMT_plx to accommodate for compilation in 64bit hosts and that manifests with the following warning :

Quoted 6 lines

Quoted 4 lines
qemu/hw/sh7750.c: In function `error_access':
qemu/hw/sh7750.c:186: warning: unsigned int format, different type arg (arg 5) qemu/hw/sh7750.c: In function `ignore_access':
qemu/hw/sh7750.c:192: warning: unsigned int format, different type arg (arg 5)
This patch works fine on 32 bit x86 hosts. Please apply.

Thanks, forgot to mention that I tested it of course as well in 32 bit x86 where the code is equivalent as cpu-defs.h defines for 32 bit targets :

#define TARGET_FMT_plx "%08x"

For 64 bit targets, it will use a 64 bit type for physical addresses and therefore a 64 bit wide format as defined by :

#define TARGET_FMT_plx "%016" PRIx64

which might not be what was intended originally and might be uncovering a bug somewhere else and based on the fact that apparently (and this gets confusing as it seems to be inconsistently used everywhere in qemu) :

target_phys_addr_t = physical address of the host ram_addr_t = physical address of the guest

and so all this function should had been using ram_addr_t instead, and that would need to be redefined to be 64 bit safe and have as well a new formatting string to match that.

Quoted 11 lines

sh7750.c 18 Nov 2007 21:08:37 -0000
Quoted 7 lines
Index: sh7750.c
===================================================================
RCS file: /sources/qemu/qemu/hw/sh7750.c,v
retrieving revision 1.11
diff -u -r1.11 sh7750.c
sh7750.c 18 Nov 2007 21:08:37 -0000 renamed from sh7750.c 17 Nov 2007 17:14:48 -0000 1.11 +0 -0
Could you please create the diff from the top level directory next time? That way it can be applied with patch -p0 or -p1 directly in the top level directory which makes patch handling much easier. Thanks!

sure, sorry about that, made the mistake when rebasing the patch for this RESEND after a week has past without any feedback.

Carlo

Blue Swirl

2007-11-30 15:37 UTC #4 lore raw

Quoted 22 lines · Carlo Marcelo Arenas Belon

Quoted 6 lines · Magnus Damm
Quoted 4 lines · Carlo Marcelo Arenas Belon
The following patch changes the formatting string from %08x to TARGET_FMT_plx to accommodate for compilation in 64bit hosts and that manifests with the following warning :
Quoted 6 lines
Quoted 4 lines
qemu/hw/sh7750.c: In function `error_access':
qemu/hw/sh7750.c:186: warning: unsigned int format, different type arg (arg 5) qemu/hw/sh7750.c: In function `ignore_access':
qemu/hw/sh7750.c:192: warning: unsigned int format, different type arg (arg 5)
This patch works fine on 32 bit x86 hosts. Please apply.
Thanks, forgot to mention that I tested it of course as well in 32 bit x86 where the code is equivalent as cpu-defs.h defines for 32 bit targets :
#define TARGET_FMT_plx "%08x"
For 64 bit targets, it will use a 64 bit type for physical addresses and therefore a 64 bit wide format as defined by :

Quoted 8 lines

#define TARGET_FMT_plx "%016" PRIx64
which might not be what was intended originally and might be uncovering a bug somewhere else and based on the fact that apparently (and this gets confusing as it seems to be inconsistently used everywhere in qemu) :
target_phys_addr_t = physical address of the host ram_addr_t = physical address of the guest

No, target_phys_addr_t is the physical address of the emulated target system. For host addresses ram_addr_t, unsigned long or even int is used. Host addresses are of course virtual, Qemu is a user space application until someone makes it run in bare metal without OS.

Quoted 3 lines

and so all this function should had been using ram_addr_t instead, and that would need to be redefined to be 64 bit safe and have as well a new formatting string to match that.

No.

Carlo Marcelo Arenas Belon

author

2007-11-30 16:05 UTC #5 lore raw

Quoted 5 lines · Blue Swirl

Quoted 4 lines · Carlo Marcelo Arenas Belon
which might not be what was intended originally and might be uncovering a bug somewhere else and based on the fact that apparently (and this gets confusing as it seems to be inconsistently used everywhere in qemu) :

Quoted 7 lines

Quoted 2 lines
target_phys_addr_t = physical address of the host ram_addr_t = physical address of the guest
No, target_phys_addr_t is the physical address of the emulated target system. For host addresses ram_addr_t, unsigned long or even int is used. Host addresses are of course virtual, Qemu is a user space application until someone makes it run in bare metal without OS.

thanks, that makes much more sense that the convoluted idea I got from the code, and now that I recovered my sanity can see finally where the bug is.

Carlo

Paul Brook

2007-11-30 16:51 UTC #6 lore raw

In reply to Blue Swirl

Quoted 7 lines

Quoted 2 lines
target_phys_addr_t = physical address of the host ram_addr_t = physical address of the guest
No, target_phys_addr_t is the physical address of the emulated target system. For host addresses ram_addr_t, unsigned long or even int is used. Host addresses are of course virtual, Qemu is a user space application until someone makes it run in bare metal without OS.

Int should never be used to hold an address of any kind, and long probably shouldn't either. The only time you should use these is where you've got a known small offset, e.g after you've subtracted a base (physical) address to get an offset within an MMIO region.

Some of the arm devices use uint32_t for addresses, which is really a bug. We get away with it because these are only ever used by 32-bit targets.

target_ulong = target virtual address.

target_phys_addr_t = target physical address. Because of the way TLB handling works these occasionally need to hold a host address. However these uses are local to the internals of the TLB code, and should never occur anywhere else.

In general all access to target memory should be via cpu_physcial_memory_{rw,read,write}

For performance reasons we currently make an exception for framebuffer devices and allow them to access ram directly. ram_addr_t holds an offset from phys_ram_base.

If you do for some reason have a host address you should use real host pointers.

Usermode emulation complicates things a bit, but this isn't relevant for any of the code in hw/.

Paul

Blue Swirl

2007-11-30 17:17 UTC #7 lore raw

Quoted 8 lines · Paul Brook

Quoted 7 lines
Quoted 2 lines
target_phys_addr_t = physical address of the host ram_addr_t = physical address of the guest
No, target_phys_addr_t is the physical address of the emulated target system. For host addresses ram_addr_t, unsigned long or even int is used. Host addresses are of course virtual, Qemu is a user space application until someone makes it run in bare metal without OS.

Quoted 4 lines

Int should never be used to hold an address of any kind, and long probably shouldn't either. The only time you should use these is where you've got a known small offset, e.g after you've subtracted a base (physical) address to get an offset within an MMIO region.

Well, there is the physical memory size defined as int and various other places using unsigned long and even int. We discussed earlier replacing this with something better.

Quoted 2 lines

Some of the arm devices use uint32_t for addresses, which is really a bug. We get away with it because these are only ever used by 32-bit targets.

It's currently a bug, yes. On the other hand, hard coding the usable device address space would allow compiling the device code just once for all targets. Many devices only need a few lowest address bits and a chip select (which for Qemu is the CPURead/WriteMemoryFunc). Of course the address registration mechanism would need some changes to support different bus widths.

Quoted 5 lines

target_ulong = target virtual address.
target_phys_addr_t = target physical address. Because of the way TLB handling works these occasionally need to hold a host address. However these uses are local to the internals of the TLB code, and should never occur anywhere else.

I think T2 may need to store host addresses as well. To be frank, I don't understand that part but there is a compiler warning on a 64 bit host.

Quoted 6 lines

In general all access to target memory should be via cpu_physcial_memory_{rw,read,write}
For performance reasons we currently make an exception for framebuffer devices and allow them to access ram directly. ram_addr_t holds an offset from phys_ram_base.

Even better would be to make separate device memory access functions and hide this exception.

Paul Brook

2007-11-30 17:42 UTC #8 lore raw

Quoted 3 lines

I think T2 may need to store host addresses as well. To be frank, I don't understand that part but there is a compiler warning on a 64 bit host.

If you're thinking of the warnings in op_goto_tb0, these are actually due to tb->tb_next having the wrong type.

Quoted 9 lines

Quoted 6 lines
In general all access to target memory should be via cpu_physcial_memory_{rw,read,write}
For performance reasons we currently make an exception for framebuffer devices and allow them to access ram directly. ram_addr_t holds an offset from phys_ram_base.
Even better would be to make separate device memory access functions and hide this exception.

cpu_physical_memory_* are the device memory access functions.

Also, on second viewing what I said isn't entirely true. There are two different cases:

Some devices "own" their framebuffer, and can legitimately access it directly. ram_addr_t is a side-effect of the way qemu does ram allocation. With a proper dynamic allocation scheme this would be an opaque handle and/or a host pointer.

Other devices use system ram (which technically may not even be ram) for the framebuffer, so should be using the normal bus/device memory access routines.

Paul

Blue Swirl

2007-11-30 18:45 UTC #9 lore raw

Quoted 6 lines · Paul Brook

Quoted 3 lines
I think T2 may need to store host addresses as well. To be frank, I don't understand that part but there is a compiler warning on a 64 bit host.
If you're thinking of the warnings in op_goto_tb0, these are actually due to tb->tb_next having the wrong type.

I meant this one (target-sparc/op_helper.c):

            if (tb) {
                /* the PC is inside the translated code. It means that we have
                   a virtual CPU fault */
                cpu_restore_state(tb, env, pc, (void *)T2);
            }

Quoted 11 lines

Quoted 9 lines
Quoted 6 lines
In general all access to target memory should be via cpu_physcial_memory_{rw,read,write}
For performance reasons we currently make an exception for framebuffer devices and allow them to access ram directly. ram_addr_t holds an offset from phys_ram_base.
Even better would be to make separate device memory access functions and hide this exception.
cpu_physical_memory_* are the device memory access functions.

I meant a function to access memory from the device side, the effect is of course identical on IOMMU-less system.