[Qemu-devel] [PATCH] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Qemu-devel] [PATCH] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Carlo Marcelo Arenas Belon]

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
diff -u -r1.9 sh7750.c
--- hw/sh7750.c	4 Oct 2007 21:53:54 -0000	1.9
+++ hw/sh7750.c	11 Nov 2007 15:27:31 -0000
@@ -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);
 }
 

[Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[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)
 
Carlo

---
Index: sh7750.c
===================================================================
RCS file: /sources/qemu/qemu/hw/sh7750.c,v
retrieving revision 1.11
diff -u -r1.11 sh7750.c
--- sh7750.c	17 Nov 2007 17:14:48 -0000	1.11
+++ sh7750.c	18 Nov 2007 21:08:37 -0000
@@ -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);
 }
 

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Magnus Damm]

On Nov 19, 2007 6:18 AM, Carlo Marcelo Arenas Belon
<carenas@sajinet.com.pe> wrote:
> 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.

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

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

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Carlo Marcelo Arenas Belon]

On Fri, Nov 30, 2007 at 02:36:32PM +0900, Magnus Damm wrote:
> On Nov 19, 2007 6:18 AM, Carlo Marcelo Arenas Belon
> <carenas@sajinet.com.pe> wrote:
> > 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.

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.

> > Index: sh7750.c
> > ===================================================================
> > RCS file: /sources/qemu/qemu/hw/sh7750.c,v
> > retrieving revision 1.11
> > diff -u -r1.11 sh7750.c
> > --- sh7750.c    17 Nov 2007 17:14:48 -0000      1.11
> > +++ sh7750.c    18 Nov 2007 21:08:37 -0000
> 
> 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

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Blue Swirl]

On 11/30/07, Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe> wrote:
> On Fri, Nov 30, 2007 at 02:36:32PM +0900, Magnus Damm wrote:
> > On Nov 19, 2007 6:18 AM, Carlo Marcelo Arenas Belon
> > <carenas@sajinet.com.pe> wrote:
> > > 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.
>
> 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

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.

> 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.

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Carlo Marcelo Arenas Belon]

On Fri, Nov 30, 2007 at 05:37:35PM +0200, Blue Swirl wrote:
> On 11/30/07, Carlo Marcelo Arenas Belon <carenas@sajinet.com.pe> wrote:
> > 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.

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

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Paul Brook]

> >   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

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Blue Swirl]

On 11/30/07, Paul Brook <paul@codesourcery.com> wrote:
> > >   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.

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.

> 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.

> 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.

> 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.

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Paul Brook]

> 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.

> > 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

Re: [Qemu-devel] [PATCH] [RESEND] hw/sh7750.c: use TARGET_FMT_plx to printf target_phys_addr_t

[Blue Swirl]

On 11/30/07, Paul Brook <paul@codesourcery.com> wrote:
> > 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);
            }

> > > 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.