[Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Paolo Bonzini]

This is a first baby step towards removing widespread inclusion of
cpu.h and compiling more devices once (so that arm, aarch64 and
in the future target-multi can share the object files).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/dma/soc_dma.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
index c06aabb..ac395c5 100644
--- a/hw/dma/soc_dma.c
+++ b/hw/dma/soc_dma.c
@@ -269,11 +269,10 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, hwaddr virt_base,
         if (entry->type == soc_dma_port_mem) {
             if (entry->addr <= virt_base &&
                             entry->addr + entry->u.mem.size > virt_base) {
-                fprintf(stderr, "%s: FIFO at " TARGET_FMT_lx
-                                " collides with RAM region at " TARGET_FMT_lx
-                                "-" TARGET_FMT_lx "\n", __FUNCTION__,
-                                (target_ulong) virt_base,
-                                (target_ulong) entry->addr, (target_ulong)
+                fprintf(stderr, "%s: FIFO at %"PRIx64
+                                " collides with RAM region at %"PRIx64
+                                "-%"PRIx64 "\n", __FUNCTION__,
+                                virt_base, entry->addr,
                                 (entry->addr + entry->u.mem.size));
                 exit(-1);
             }
@@ -284,10 +283,9 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, hwaddr virt_base,
             while (entry < dma->memmap + dma->memmap_size &&
                             entry->addr <= virt_base) {
                 if (entry->addr == virt_base && entry->u.fifo.out == out) {
-                    fprintf(stderr, "%s: FIFO at " TARGET_FMT_lx
-                                    " collides FIFO at " TARGET_FMT_lx "\n",
-                                    __FUNCTION__, (target_ulong) virt_base,
-                                    (target_ulong) entry->addr);
+                    fprintf(stderr, "%s: FIFO at %"PRIx64
+                                    " collides FIFO at %"PRIx64 "\n",
+                                    __FUNCTION__, virt_base, entry->addr);
                     exit(-1);
                 }
 
@@ -322,13 +320,11 @@ void soc_dma_port_add_mem(struct soc_dma_s *soc, uint8_t *phys_base,
             if ((entry->addr >= virt_base && entry->addr < virt_base + size) ||
                             (entry->addr <= virt_base &&
                              entry->addr + entry->u.mem.size > virt_base)) {
-                fprintf(stderr, "%s: RAM at " TARGET_FMT_lx "-" TARGET_FMT_lx
-                                " collides with RAM region at " TARGET_FMT_lx
-                                "-" TARGET_FMT_lx "\n", __FUNCTION__,
-                                (target_ulong) virt_base,
-                                (target_ulong) (virt_base + size),
-                                (target_ulong) entry->addr, (target_ulong)
-                                (entry->addr + entry->u.mem.size));
+                fprintf(stderr, "%s: RAM at %"PRIx64 "-%"PRIx64
+                                " collides with RAM region at %"PRIx64
+                                "-%"PRIx64 "\n", __FUNCTION__,
+                                virt_base, virt_base + size,
+                                entry->addr, entry->addr + entry->u.mem.size);
                 exit(-1);
             }
 
@@ -337,12 +333,11 @@ void soc_dma_port_add_mem(struct soc_dma_s *soc, uint8_t *phys_base,
         } else {
             if (entry->addr >= virt_base &&
                             entry->addr < virt_base + size) {
-                fprintf(stderr, "%s: RAM at " TARGET_FMT_lx "-" TARGET_FMT_lx
-                                " collides with FIFO at " TARGET_FMT_lx
+                fprintf(stderr, "%s: RAM at %"PRIx64 "-%"PRIx64
+                                " collides with FIFO at %"PRIx64
                                 "\n", __FUNCTION__,
-                                (target_ulong) virt_base,
-                                (target_ulong) (virt_base + size),
-                                (target_ulong) entry->addr);
+                                virt_base, virt_base + size,
+                                entry->addr);
                 exit(-1);
             }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH] arm: explicitly mark loads as little-endian

[Paolo Bonzini]

ARM softmmu is always compiled as little endian; BE8/BE32 can be
done as part of CPU emulation.  Thus, devices need not use the
endian-dependent loads and swaps.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/omap_lcd_template.h | 4 ++--
 hw/display/pxa2xx_lcd.c        | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/display/omap_lcd_template.h b/hw/display/omap_lcd_template.h
index e5dd447..f0ce71f 100644
--- a/hw/display/omap_lcd_template.h
+++ b/hw/display/omap_lcd_template.h
@@ -136,7 +136,7 @@ static void glue(draw_line12_, DEPTH)(void *opaque,
     uint8_t r, g, b;
 
     do {
-        v = lduw_p((void *) s);
+        v = lduw_le_p((void *) s);
         r = (v >> 4) & 0xf0;
         g = v & 0xf0;
         b = (v << 4) & 0xf0;
@@ -159,7 +159,7 @@ static void glue(draw_line16_, DEPTH)(void *opaque,
     uint8_t r, g, b;
 
     do {
-        v = lduw_p((void *) s);
+        v = lduw_le_p((void *) s);
         r = (v >> 8) & 0xf8;
         g = (v >> 3) & 0xfc;
         b = (v << 3) & 0xf8;
diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index 494700d..4d36c94 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -309,10 +309,10 @@ static void pxa2xx_descriptor_load(PXA2xxLCDState *s)
         }
 
         cpu_physical_memory_read(descptr, &desc, sizeof(desc));
-        s->dma_ch[i].descriptor = tswap32(desc.fdaddr);
-        s->dma_ch[i].source = tswap32(desc.fsaddr);
-        s->dma_ch[i].id = tswap32(desc.fidr);
-        s->dma_ch[i].command = tswap32(desc.ldcmd);
+        s->dma_ch[i].descriptor = le32_to_cpu(desc.fdaddr);
+        s->dma_ch[i].source = le32_to_cpu(desc.fsaddr);
+        s->dma_ch[i].id = le32_to_cpu(desc.fidr);
+        s->dma_ch[i].command = le32_to_cpu(desc.ldcmd);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] arm: explicitly mark loads as little-endian

[Peter Maydell]

On 4 December 2015 at 12:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> ARM softmmu is always compiled as little endian; BE8/BE32 can be
> done as part of CPU emulation.  Thus, devices need not use the
> endian-dependent loads and swaps.

The patch code is right, but I think the more significant point
here is that the behaviour of the PXA2xx and OMAP devices being
emulated here doesn't (shouldn't) depend on the CPU endianness
anyway, and these devices are little endian when they do DMA
accesses.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/display/omap_lcd_template.h | 4 ++--
>  hw/display/pxa2xx_lcd.c        | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/omap_lcd_template.h b/hw/display/omap_lcd_template.h
> index e5dd447..f0ce71f 100644
> --- a/hw/display/omap_lcd_template.h
> +++ b/hw/display/omap_lcd_template.h
> @@ -136,7 +136,7 @@ static void glue(draw_line12_, DEPTH)(void *opaque,
>      uint8_t r, g, b;
>
>      do {
> -        v = lduw_p((void *) s);
> +        v = lduw_le_p((void *) s);
>          r = (v >> 4) & 0xf0;
>          g = v & 0xf0;
>          b = (v << 4) & 0xf0;
> @@ -159,7 +159,7 @@ static void glue(draw_line16_, DEPTH)(void *opaque,
>      uint8_t r, g, b;
>
>      do {
> -        v = lduw_p((void *) s);
> +        v = lduw_le_p((void *) s);
>          r = (v >> 8) & 0xf8;
>          g = (v >> 3) & 0xfc;
>          b = (v << 3) & 0xf8;
> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
> index 494700d..4d36c94 100644
> --- a/hw/display/pxa2xx_lcd.c
> +++ b/hw/display/pxa2xx_lcd.c
> @@ -309,10 +309,10 @@ static void pxa2xx_descriptor_load(PXA2xxLCDState *s)
>          }
>
>          cpu_physical_memory_read(descptr, &desc, sizeof(desc));
> -        s->dma_ch[i].descriptor = tswap32(desc.fdaddr);
> -        s->dma_ch[i].source = tswap32(desc.fsaddr);
> -        s->dma_ch[i].id = tswap32(desc.fidr);
> -        s->dma_ch[i].command = tswap32(desc.ldcmd);
> +        s->dma_ch[i].descriptor = le32_to_cpu(desc.fdaddr);
> +        s->dma_ch[i].source = le32_to_cpu(desc.fsaddr);
> +        s->dma_ch[i].id = le32_to_cpu(desc.fidr);
> +        s->dma_ch[i].command = le32_to_cpu(desc.ldcmd);
>      }
>  }
>
> --
> 1.8.3.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: explicitly mark loads as little-endian

[Peter Maydell]

On 4 December 2015 at 12:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> ARM softmmu is always compiled as little endian; BE8/BE32 can be
> done as part of CPU emulation.  Thus, devices need not use the
> endian-dependent loads and swaps.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I have applied this to target-arm.next with a tweaked
commit message:

arm: explicitly mark device loads as little-endian

Behaviour of emulated devices should not depend on the endianness
of the CPU, so avoid using the endian-dependent load and store
functions in the PXA2xx and OMAP display devices. These devices
are little endian when they do DMA access.

(Since ARM softmmu is always compiled as little endian, this means
that the endian-dependent load and store functions are always little
endian, so this commit makes no functionally visible change.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Peter Maydell]

On 4 December 2015 at 12:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This is a first baby step towards removing widespread inclusion of
> cpu.h and compiling more devices once (so that arm, aarch64 and
> in the future target-multi can share the object files).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/dma/soc_dma.c | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
>
> diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
> index c06aabb..ac395c5 100644
> --- a/hw/dma/soc_dma.c
> +++ b/hw/dma/soc_dma.c
> @@ -269,11 +269,10 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, hwaddr virt_base,
>          if (entry->type == soc_dma_port_mem) {
>              if (entry->addr <= virt_base &&
>                              entry->addr + entry->u.mem.size > virt_base) {
> -                fprintf(stderr, "%s: FIFO at " TARGET_FMT_lx
> -                                " collides with RAM region at " TARGET_FMT_lx
> -                                "-" TARGET_FMT_lx "\n", __FUNCTION__,
> -                                (target_ulong) virt_base,
> -                                (target_ulong) entry->addr, (target_ulong)
> +                fprintf(stderr, "%s: FIFO at %"PRIx64
> +                                " collides with RAM region at %"PRIx64
> +                                "-%"PRIx64 "\n", __FUNCTION__,
> +                                virt_base, entry->addr,
>                                  (entry->addr + entry->u.mem.size));

Is using the HWADDR_PRI* macros for printing hwaddrs deprecated now?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Paolo Bonzini]

On 04/12/2015 13:33, Peter Maydell wrote:
> On 4 December 2015 at 12:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> This is a first baby step towards removing widespread inclusion of
>> cpu.h and compiling more devices once (so that arm, aarch64 and
>> in the future target-multi can share the object files).
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/dma/soc_dma.c | 37 ++++++++++++++++---------------------
>>  1 file changed, 16 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
>> index c06aabb..ac395c5 100644
>> --- a/hw/dma/soc_dma.c
>> +++ b/hw/dma/soc_dma.c
>> @@ -269,11 +269,10 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, hwaddr virt_base,
>>          if (entry->type == soc_dma_port_mem) {
>>              if (entry->addr <= virt_base &&
>>                              entry->addr + entry->u.mem.size > virt_base) {
>> -                fprintf(stderr, "%s: FIFO at " TARGET_FMT_lx
>> -                                " collides with RAM region at " TARGET_FMT_lx
>> -                                "-" TARGET_FMT_lx "\n", __FUNCTION__,
>> -                                (target_ulong) virt_base,
>> -                                (target_ulong) entry->addr, (target_ulong)
>> +                fprintf(stderr, "%s: FIFO at %"PRIx64
>> +                                " collides with RAM region at %"PRIx64
>> +                                "-%"PRIx64 "\n", __FUNCTION__,
>> +                                virt_base, entry->addr,
>>                                  (entry->addr + entry->u.mem.size));
> 
> Is using the HWADDR_PRI* macros for printing hwaddrs deprecated now?

It's the first time I hear about them. :)  There are still 130-odd
usages of HWADDR_PRIx and friends, so at least it's an incomplete
transition.  So I can use them if the maintainer tells me to.

Paolo

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Peter Maydell]

On 4 December 2015 at 12:36, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 04/12/2015 13:33, Peter Maydell wrote:
>> On 4 December 2015 at 12:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> This is a first baby step towards removing widespread inclusion of
>>> cpu.h and compiling more devices once (so that arm, aarch64 and
>>> in the future target-multi can share the object files).
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  hw/dma/soc_dma.c | 37 ++++++++++++++++---------------------
>>>  1 file changed, 16 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
>>> index c06aabb..ac395c5 100644
>>> --- a/hw/dma/soc_dma.c
>>> +++ b/hw/dma/soc_dma.c
>>> @@ -269,11 +269,10 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, hwaddr virt_base,
>>>          if (entry->type == soc_dma_port_mem) {
>>>              if (entry->addr <= virt_base &&
>>>                              entry->addr + entry->u.mem.size > virt_base) {
>>> -                fprintf(stderr, "%s: FIFO at " TARGET_FMT_lx
>>> -                                " collides with RAM region at " TARGET_FMT_lx
>>> -                                "-" TARGET_FMT_lx "\n", __FUNCTION__,
>>> -                                (target_ulong) virt_base,
>>> -                                (target_ulong) entry->addr, (target_ulong)
>>> +                fprintf(stderr, "%s: FIFO at %"PRIx64
>>> +                                " collides with RAM region at %"PRIx64
>>> +                                "-%"PRIx64 "\n", __FUNCTION__,
>>> +                                virt_base, entry->addr,
>>>                                  (entry->addr + entry->u.mem.size));
>>
>> Is using the HWADDR_PRI* macros for printing hwaddrs deprecated now?
>
> It's the first time I hear about them. :)  There are still 130-odd
> usages of HWADDR_PRIx and friends, so at least it's an incomplete
> transition.  So I can use them if the maintainer tells me to.

Well, we needed HWADDR_PRI* when hwaddr wasn't unconditionally 64-bits
(that changed back in 2012 with commit 4be403c8158e1b6, back when it
was still called target_phys_addr_t and the macros were TARGET_PRI*).
So back in 2012 we'd have noticed uses of PRIx64 to print hwaddrs
because it would have meant a compile failure (at least in some bits
of code). But these days we probably wouldn't catch uses in code
review and they wouldn't be compile time failures.

I don't think we've ever said "we should transition away from HWADDR_*",
but whether we should is an interesting question, which is why I asked.
Does retaining the format macros to go with the typedef give us
useful flexibility, or is it just confusing?

(Also TARGET_FMT_plx, which is even more heavily used and now
rather out of step with the type name.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Paolo Bonzini]

> I don't think we've ever said "we should transition away from HWADDR_*",
> but whether we should is an interesting question, which is why I asked.
> Does retaining the format macros to go with the typedef give us
> useful flexibility, or is it just confusing?

I think it's confusing, but not enough to warrant a tree-wide search
and replace.

Paolo

> (Also TARGET_FMT_plx, which is even more heavily used and now
> rather out of step with the type name.)
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Peter Maydell]

On 4 December 2015 at 16:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> I don't think we've ever said "we should transition away from HWADDR_*",
>> but whether we should is an interesting question, which is why I asked.
>> Does retaining the format macros to go with the typedef give us
>> useful flexibility, or is it just confusing?
>
> I think it's confusing, but not enough to warrant a tree-wide search
> and replace.

I think I agree :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

and I'll queue this in target-arm.next for 2.6.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Peter Maydell]

On 4 December 2015 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 December 2015 at 16:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> I don't think we've ever said "we should transition away from HWADDR_*",
>>> but whether we should is an interesting question, which is why I asked.
>>> Does retaining the format macros to go with the typedef give us
>>> useful flexibility, or is it just confusing?
>>
>> I think it's confusing, but not enough to warrant a tree-wide search
>> and replace.
>
> I think I agree :-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> and I'll queue this in target-arm.next for 2.6.

Now in target-arm.next. I also did s/__FUNCTION__/__func__/
since checkpatch complains about it and we're touching these lines
of code anyway.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Paolo Bonzini]

On 15/12/2015 12:28, Peter Maydell wrote:
> Now in target-arm.next. I also did s/__FUNCTION__/__func__/
> since checkpatch complains about it and we're touching these lines
> of code anyway.

Thanks!

Paolo

Re: [Qemu-devel] [PATCH] arm: soc-dma: use hwaddr instead of target_ulong in printf

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This is a first baby step towards removing widespread inclusion of
> cpu.h and compiling more devices once (so that arm, aarch64 and
> in the future target-multi can share the object files).

Sounds like something that should be covered in
http://qemu-project.org/CodeTransitions