[Qemu-devel] [PATCH 0/9] Don't use ld./st._phys in hw/

[Qemu-devel] [PATCH 0/9] Don't use ld./st._phys in hw/

[Alexander Graf]

We have quite some code in hw/ that uses ld./st._phys functions
in device emulation code. This is just pure wrong, as devices
don't know about the CPU endianness (except for virtio), so they
should instead use something endian specific.

Unfortunately, there is no endian specific call to easily receive
an integer from memory, so this patch set introduces some and then
converts all the obvious users to them.

I tested the targets I could, but please double-check if your
architecture behaves differently or I accidently put in a _be
instead of _le :)


Alex

Alexander Graf (9):
  exec: add endian specific phys ld/st functions
  hpet: use specific endian ld/st_phys
  intel-hda: use specific endian ld/st_phys
  msi: use specific endian ld/st_phys
  msix: use specific endian ld/st_phys
  pl080: use specific endian ld/st_phys
  ppc405_uc: use specific endian ld/st_phys
  s390-virtio: use specific endian ld/st_phys
  spapr: use specific endian ld/st_phys

 cpu-common.h         |   12 ++++++
 exec.c               |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/hpet.c            |    2 +-
 hw/intel-hda.c       |   21 ++--------
 hw/msi.c             |    2 +-
 hw/msix.c            |    2 +-
 hw/pl080.c           |    8 ++--
 hw/ppc405_uc.c       |   43 +++++++++++----------
 hw/s390-virtio-bus.c |   10 ++--
 hw/s390-virtio.c     |    6 +-
 hw/spapr.h           |    4 +-
 hw/spapr_hcall.c     |   12 +++---
 12 files changed, 164 insertions(+), 60 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Alexander Graf]

Device code some times needs to access physical memory and does that
through the ld./st._phys functions. However, these are the exact same
functions that the CPU uses to access memory, which means they will
be endianness swapped depending on the target CPU.

However, devices don't know about the CPU's endianness, but instead
access memory directly using their own interface to the memory bus,
so they need some way to read data with their native endianness.

This patch adds _le and _be functions to ld./st._phys.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 cpu-common.h |   12 +++++++
 exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+), 0 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index b027e43..c6a2b5f 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
 
 uint32_t ldub_phys(target_phys_addr_t addr);
 uint32_t lduw_phys(target_phys_addr_t addr);
+uint32_t lduw_le_phys(target_phys_addr_t addr);
+uint32_t lduw_be_phys(target_phys_addr_t addr);
 uint32_t ldl_phys(target_phys_addr_t addr);
+uint32_t ldl_le_phys(target_phys_addr_t addr);
+uint32_t ldl_be_phys(target_phys_addr_t addr);
 uint64_t ldq_phys(target_phys_addr_t addr);
+uint64_t ldq_le_phys(target_phys_addr_t addr);
+uint64_t ldq_be_phys(target_phys_addr_t addr);
 void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
 void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
 void stb_phys(target_phys_addr_t addr, uint32_t val);
 void stw_phys(target_phys_addr_t addr, uint32_t val);
+void stw_le_phys(target_phys_addr_t addr, uint32_t val);
+void stw_be_phys(target_phys_addr_t addr, uint32_t val);
 void stl_phys(target_phys_addr_t addr, uint32_t val);
+void stl_le_phys(target_phys_addr_t addr, uint32_t val);
+void stl_be_phys(target_phys_addr_t addr, uint32_t val);
 void stq_phys(target_phys_addr_t addr, uint64_t val);
+void stq_le_phys(target_phys_addr_t addr, uint64_t val);
+void stq_be_phys(target_phys_addr_t addr, uint64_t val);
 
 void cpu_physical_memory_write_rom(target_phys_addr_t addr,
                                    const uint8_t *buf, int len);
diff --git a/exec.c b/exec.c
index 4c45299..5f2f87e 100644
--- a/exec.c
+++ b/exec.c
@@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
     return val;
 }
 
+uint32_t ldl_le_phys(target_phys_addr_t addr)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return bswap32(ldl_phys(addr));
+#else
+    return ldl_phys(addr);
+#endif
+}
+
+uint32_t ldl_be_phys(target_phys_addr_t addr)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return ldl_phys(addr);
+#else
+    return bswap32(ldl_phys(addr));
+#endif
+}
+
 /* warning: addr must be aligned */
 uint64_t ldq_phys(target_phys_addr_t addr)
 {
@@ -4196,6 +4214,24 @@ uint64_t ldq_phys(target_phys_addr_t addr)
     return val;
 }
 
+uint64_t ldq_le_phys(target_phys_addr_t addr)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return bswap64(ldq_phys(addr));
+#else
+    return ldq_phys(addr);
+#endif
+}
+
+uint64_t ldq_be_phys(target_phys_addr_t addr)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return ldq_phys(addr);
+#else
+    return bswap64(ldq_phys(addr));
+#endif
+}
+
 /* XXX: optimize */
 uint32_t ldub_phys(target_phys_addr_t addr)
 {
@@ -4236,6 +4272,24 @@ uint32_t lduw_phys(target_phys_addr_t addr)
     return val;
 }
 
+uint32_t lduw_le_phys(target_phys_addr_t addr)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return bswap16(lduw_phys(addr));
+#else
+    return lduw_phys(addr);
+#endif
+}
+
+uint32_t lduw_be_phys(target_phys_addr_t addr)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return lduw_phys(addr);
+#else
+    return bswap16(lduw_phys(addr));
+#endif
+}
+
 /* warning: addr must be aligned. The ram page is not masked as dirty
    and the code inside is not invalidated. It is useful if the dirty
    bits are used to track modified PTEs */
@@ -4343,6 +4397,24 @@ void stl_phys(target_phys_addr_t addr, uint32_t val)
     }
 }
 
+void stl_le_phys(target_phys_addr_t addr, uint32_t val)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return stl_phys(addr, bswap32(val));
+#else
+    return stl_phys(addr, val);
+#endif
+}
+
+void stl_be_phys(target_phys_addr_t addr, uint32_t val)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return stl_phys(addr, val);
+#else
+    return stl_phys(addr, bswap32(val));
+#endif
+}
+
 /* XXX: optimize */
 void stb_phys(target_phys_addr_t addr, uint32_t val)
 {
@@ -4386,6 +4458,24 @@ void stw_phys(target_phys_addr_t addr, uint32_t val)
     }
 }
 
+void stw_le_phys(target_phys_addr_t addr, uint32_t val)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return stw_phys(addr, bswap16(val));
+#else
+    return stw_phys(addr, val);
+#endif
+}
+
+void stw_be_phys(target_phys_addr_t addr, uint32_t val)
+{
+#if defined(TARGET_WORDS_BIGENDIAN)
+    return stw_phys(addr, val);
+#else
+    return stw_phys(addr, bswap16(val));
+#endif
+}
+
 /* XXX: optimize */
 void stq_phys(target_phys_addr_t addr, uint64_t val)
 {
@@ -4393,6 +4483,18 @@ void stq_phys(target_phys_addr_t addr, uint64_t val)
     cpu_physical_memory_write(addr, &val, 8);
 }
 
+void stq_le_phys(target_phys_addr_t addr, uint64_t val)
+{
+    val = cpu_to_le64(val);
+    cpu_physical_memory_write(addr, &val, 8);
+}
+
+void stq_be_phys(target_phys_addr_t addr, uint64_t val)
+{
+    val = cpu_to_be64(val);
+    cpu_physical_memory_write(addr, &val, 8);
+}
+
 /* virtual memory access for debug (includes writing to ROM) */
 int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
                         uint8_t *buf, int len, int is_write)
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Blue Swirl]

On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
> Device code some times needs to access physical memory and does that
> through the ld./st._phys functions. However, these are the exact same
> functions that the CPU uses to access memory, which means they will
> be endianness swapped depending on the target CPU.
>
> However, devices don't know about the CPU's endianness, but instead
> access memory directly using their own interface to the memory bus,
> so they need some way to read data with their native endianness.
>
> This patch adds _le and _be functions to ld./st._phys.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  cpu-common.h |   12 +++++++
>  exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+), 0 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b027e43..c6a2b5f 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
>
>  uint32_t ldub_phys(target_phys_addr_t addr);
>  uint32_t lduw_phys(target_phys_addr_t addr);
> +uint32_t lduw_le_phys(target_phys_addr_t addr);
> +uint32_t lduw_be_phys(target_phys_addr_t addr);
>  uint32_t ldl_phys(target_phys_addr_t addr);
> +uint32_t ldl_le_phys(target_phys_addr_t addr);
> +uint32_t ldl_be_phys(target_phys_addr_t addr);
>  uint64_t ldq_phys(target_phys_addr_t addr);
> +uint64_t ldq_le_phys(target_phys_addr_t addr);
> +uint64_t ldq_be_phys(target_phys_addr_t addr);
>  void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
>  void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
>  void stb_phys(target_phys_addr_t addr, uint32_t val);
>  void stw_phys(target_phys_addr_t addr, uint32_t val);
> +void stw_le_phys(target_phys_addr_t addr, uint32_t val);
> +void stw_be_phys(target_phys_addr_t addr, uint32_t val);
>  void stl_phys(target_phys_addr_t addr, uint32_t val);
> +void stl_le_phys(target_phys_addr_t addr, uint32_t val);
> +void stl_be_phys(target_phys_addr_t addr, uint32_t val);
>  void stq_phys(target_phys_addr_t addr, uint64_t val);
> +void stq_le_phys(target_phys_addr_t addr, uint64_t val);
> +void stq_be_phys(target_phys_addr_t addr, uint64_t val);
>
>  void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>                                    const uint8_t *buf, int len);
> diff --git a/exec.c b/exec.c
> index 4c45299..5f2f87e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
>     return val;
>  }
>
> +uint32_t ldl_le_phys(target_phys_addr_t addr)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return bswap32(ldl_phys(addr));

This would introduce a second bswap in some cases. Please make instead
two versions of ldl_phys which use ldl_le/be_p instead of ldl_p. Then
ldl_phys could be #defined to the suitable function.

BTW, these functions would need a serious cleanup, there's a lot code
duplication and comments about XXX: optimize.

> +#else
> +    return ldl_phys(addr);
> +#endif
> +}
> +
> +uint32_t ldl_be_phys(target_phys_addr_t addr)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return ldl_phys(addr);
> +#else
> +    return bswap32(ldl_phys(addr));
> +#endif
> +}
> +
>  /* warning: addr must be aligned */
>  uint64_t ldq_phys(target_phys_addr_t addr)
>  {
> @@ -4196,6 +4214,24 @@ uint64_t ldq_phys(target_phys_addr_t addr)
>     return val;
>  }
>
> +uint64_t ldq_le_phys(target_phys_addr_t addr)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return bswap64(ldq_phys(addr));
> +#else
> +    return ldq_phys(addr);
> +#endif
> +}
> +
> +uint64_t ldq_be_phys(target_phys_addr_t addr)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return ldq_phys(addr);
> +#else
> +    return bswap64(ldq_phys(addr));
> +#endif
> +}
> +
>  /* XXX: optimize */
>  uint32_t ldub_phys(target_phys_addr_t addr)
>  {
> @@ -4236,6 +4272,24 @@ uint32_t lduw_phys(target_phys_addr_t addr)
>     return val;
>  }
>
> +uint32_t lduw_le_phys(target_phys_addr_t addr)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return bswap16(lduw_phys(addr));
> +#else
> +    return lduw_phys(addr);
> +#endif
> +}
> +
> +uint32_t lduw_be_phys(target_phys_addr_t addr)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return lduw_phys(addr);
> +#else
> +    return bswap16(lduw_phys(addr));
> +#endif
> +}
> +
>  /* warning: addr must be aligned. The ram page is not masked as dirty
>    and the code inside is not invalidated. It is useful if the dirty
>    bits are used to track modified PTEs */
> @@ -4343,6 +4397,24 @@ void stl_phys(target_phys_addr_t addr, uint32_t val)
>     }
>  }
>
> +void stl_le_phys(target_phys_addr_t addr, uint32_t val)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return stl_phys(addr, bswap32(val));
> +#else
> +    return stl_phys(addr, val);
> +#endif
> +}
> +
> +void stl_be_phys(target_phys_addr_t addr, uint32_t val)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return stl_phys(addr, val);
> +#else
> +    return stl_phys(addr, bswap32(val));
> +#endif
> +}
> +
>  /* XXX: optimize */
>  void stb_phys(target_phys_addr_t addr, uint32_t val)
>  {
> @@ -4386,6 +4458,24 @@ void stw_phys(target_phys_addr_t addr, uint32_t val)
>     }
>  }
>
> +void stw_le_phys(target_phys_addr_t addr, uint32_t val)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return stw_phys(addr, bswap16(val));
> +#else
> +    return stw_phys(addr, val);
> +#endif
> +}
> +
> +void stw_be_phys(target_phys_addr_t addr, uint32_t val)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return stw_phys(addr, val);
> +#else
> +    return stw_phys(addr, bswap16(val));
> +#endif
> +}
> +
>  /* XXX: optimize */
>  void stq_phys(target_phys_addr_t addr, uint64_t val)
>  {
> @@ -4393,6 +4483,18 @@ void stq_phys(target_phys_addr_t addr, uint64_t val)
>     cpu_physical_memory_write(addr, &val, 8);
>  }
>
> +void stq_le_phys(target_phys_addr_t addr, uint64_t val)
> +{
> +    val = cpu_to_le64(val);
> +    cpu_physical_memory_write(addr, &val, 8);
> +}
> +
> +void stq_be_phys(target_phys_addr_t addr, uint64_t val)
> +{
> +    val = cpu_to_be64(val);
> +    cpu_physical_memory_write(addr, &val, 8);
> +}
> +
>  /* virtual memory access for debug (includes writing to ROM) */
>  int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
>                         uint8_t *buf, int len, int is_write)
> --
> 1.7.3.4
>
>
>

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Alexander Graf]

On 05.07.2011, at 23:48, Blue Swirl wrote:

> On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
>> Device code some times needs to access physical memory and does that
>> through the ld./st._phys functions. However, these are the exact same
>> functions that the CPU uses to access memory, which means they will
>> be endianness swapped depending on the target CPU.
>> 
>> However, devices don't know about the CPU's endianness, but instead
>> access memory directly using their own interface to the memory bus,
>> so they need some way to read data with their native endianness.
>> 
>> This patch adds _le and _be functions to ld./st._phys.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  cpu-common.h |   12 +++++++
>>  exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 114 insertions(+), 0 deletions(-)
>> 
>> diff --git a/cpu-common.h b/cpu-common.h
>> index b027e43..c6a2b5f 100644
>> --- a/cpu-common.h
>> +++ b/cpu-common.h
>> @@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
>> 
>>  uint32_t ldub_phys(target_phys_addr_t addr);
>>  uint32_t lduw_phys(target_phys_addr_t addr);
>> +uint32_t lduw_le_phys(target_phys_addr_t addr);
>> +uint32_t lduw_be_phys(target_phys_addr_t addr);
>>  uint32_t ldl_phys(target_phys_addr_t addr);
>> +uint32_t ldl_le_phys(target_phys_addr_t addr);
>> +uint32_t ldl_be_phys(target_phys_addr_t addr);
>>  uint64_t ldq_phys(target_phys_addr_t addr);
>> +uint64_t ldq_le_phys(target_phys_addr_t addr);
>> +uint64_t ldq_be_phys(target_phys_addr_t addr);
>>  void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
>>  void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
>>  void stb_phys(target_phys_addr_t addr, uint32_t val);
>>  void stw_phys(target_phys_addr_t addr, uint32_t val);
>> +void stw_le_phys(target_phys_addr_t addr, uint32_t val);
>> +void stw_be_phys(target_phys_addr_t addr, uint32_t val);
>>  void stl_phys(target_phys_addr_t addr, uint32_t val);
>> +void stl_le_phys(target_phys_addr_t addr, uint32_t val);
>> +void stl_be_phys(target_phys_addr_t addr, uint32_t val);
>>  void stq_phys(target_phys_addr_t addr, uint64_t val);
>> +void stq_le_phys(target_phys_addr_t addr, uint64_t val);
>> +void stq_be_phys(target_phys_addr_t addr, uint64_t val);
>> 
>>  void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>                                    const uint8_t *buf, int len);
>> diff --git a/exec.c b/exec.c
>> index 4c45299..5f2f87e 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
>>     return val;
>>  }
>> 
>> +uint32_t ldl_le_phys(target_phys_addr_t addr)
>> +{
>> +#if defined(TARGET_WORDS_BIGENDIAN)
>> +    return bswap32(ldl_phys(addr));
> 
> This would introduce a second bswap in some cases. Please make instead
> two versions of ldl_phys which use ldl_le/be_p instead of ldl_p. Then
> ldl_phys could be #defined to the suitable function.

Yeah, I was thinking to do that one at first and then realized how MMIO gets tricky, since we also need to potentially swap it again, as by now it returns values in target CPU endianness. But if you prefer, I can dig into that.

> 
> BTW, these functions would need a serious cleanup, there's a lot code
> duplication and comments about XXX: optimize.

Yes :). One thing at a time :).


Alex

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Blue Swirl]

On Wed, Jul 6, 2011 at 12:55 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 05.07.2011, at 23:48, Blue Swirl wrote:
>
>> On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>> Device code some times needs to access physical memory and does that
>>> through the ld./st._phys functions. However, these are the exact same
>>> functions that the CPU uses to access memory, which means they will
>>> be endianness swapped depending on the target CPU.
>>>
>>> However, devices don't know about the CPU's endianness, but instead
>>> access memory directly using their own interface to the memory bus,
>>> so they need some way to read data with their native endianness.
>>>
>>> This patch adds _le and _be functions to ld./st._phys.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  cpu-common.h |   12 +++++++
>>>  exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 114 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cpu-common.h b/cpu-common.h
>>> index b027e43..c6a2b5f 100644
>>> --- a/cpu-common.h
>>> +++ b/cpu-common.h
>>> @@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
>>>
>>>  uint32_t ldub_phys(target_phys_addr_t addr);
>>>  uint32_t lduw_phys(target_phys_addr_t addr);
>>> +uint32_t lduw_le_phys(target_phys_addr_t addr);
>>> +uint32_t lduw_be_phys(target_phys_addr_t addr);
>>>  uint32_t ldl_phys(target_phys_addr_t addr);
>>> +uint32_t ldl_le_phys(target_phys_addr_t addr);
>>> +uint32_t ldl_be_phys(target_phys_addr_t addr);
>>>  uint64_t ldq_phys(target_phys_addr_t addr);
>>> +uint64_t ldq_le_phys(target_phys_addr_t addr);
>>> +uint64_t ldq_be_phys(target_phys_addr_t addr);
>>>  void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
>>>  void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
>>>  void stb_phys(target_phys_addr_t addr, uint32_t val);
>>>  void stw_phys(target_phys_addr_t addr, uint32_t val);
>>> +void stw_le_phys(target_phys_addr_t addr, uint32_t val);
>>> +void stw_be_phys(target_phys_addr_t addr, uint32_t val);
>>>  void stl_phys(target_phys_addr_t addr, uint32_t val);
>>> +void stl_le_phys(target_phys_addr_t addr, uint32_t val);
>>> +void stl_be_phys(target_phys_addr_t addr, uint32_t val);
>>>  void stq_phys(target_phys_addr_t addr, uint64_t val);
>>> +void stq_le_phys(target_phys_addr_t addr, uint64_t val);
>>> +void stq_be_phys(target_phys_addr_t addr, uint64_t val);
>>>
>>>  void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>>                                    const uint8_t *buf, int len);
>>> diff --git a/exec.c b/exec.c
>>> index 4c45299..5f2f87e 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
>>>     return val;
>>>  }
>>>
>>> +uint32_t ldl_le_phys(target_phys_addr_t addr)
>>> +{
>>> +#if defined(TARGET_WORDS_BIGENDIAN)
>>> +    return bswap32(ldl_phys(addr));
>>
>> This would introduce a second bswap in some cases. Please make instead
>> two versions of ldl_phys which use ldl_le/be_p instead of ldl_p. Then
>> ldl_phys could be #defined to the suitable function.
>
> Yeah, I was thinking to do that one at first and then realized how MMIO gets tricky, since we also need to potentially swap it again, as by now it returns values in target CPU endianness. But if you prefer, I can dig into that.

Maybe the swapendian thing could be adjusted so that it's possible to
access the device in either LE or BE way directly? For example, there
could be two io_mem_read/write tables, the current one for CPU and
another one for device MMIO with known endianness. Or even three
tables: CPU, BE, LE.

>>
>> BTW, these functions would need a serious cleanup, there's a lot code
>> duplication and comments about XXX: optimize.
>
> Yes :). One thing at a time :).
>
>
> Alex
>
>

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Alexander Graf]

On 06.07.2011, at 00:05, Blue Swirl wrote:

> On Wed, Jul 6, 2011 at 12:55 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 05.07.2011, at 23:48, Blue Swirl wrote:
>> 
>>> On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>> Device code some times needs to access physical memory and does that
>>>> through the ld./st._phys functions. However, these are the exact same
>>>> functions that the CPU uses to access memory, which means they will
>>>> be endianness swapped depending on the target CPU.
>>>> 
>>>> However, devices don't know about the CPU's endianness, but instead
>>>> access memory directly using their own interface to the memory bus,
>>>> so they need some way to read data with their native endianness.
>>>> 
>>>> This patch adds _le and _be functions to ld./st._phys.
>>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>  cpu-common.h |   12 +++++++
>>>>  exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 114 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/cpu-common.h b/cpu-common.h
>>>> index b027e43..c6a2b5f 100644
>>>> --- a/cpu-common.h
>>>> +++ b/cpu-common.h
>>>> @@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
>>>> 
>>>>  uint32_t ldub_phys(target_phys_addr_t addr);
>>>>  uint32_t lduw_phys(target_phys_addr_t addr);
>>>> +uint32_t lduw_le_phys(target_phys_addr_t addr);
>>>> +uint32_t lduw_be_phys(target_phys_addr_t addr);
>>>>  uint32_t ldl_phys(target_phys_addr_t addr);
>>>> +uint32_t ldl_le_phys(target_phys_addr_t addr);
>>>> +uint32_t ldl_be_phys(target_phys_addr_t addr);
>>>>  uint64_t ldq_phys(target_phys_addr_t addr);
>>>> +uint64_t ldq_le_phys(target_phys_addr_t addr);
>>>> +uint64_t ldq_be_phys(target_phys_addr_t addr);
>>>>  void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
>>>>  void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
>>>>  void stb_phys(target_phys_addr_t addr, uint32_t val);
>>>>  void stw_phys(target_phys_addr_t addr, uint32_t val);
>>>> +void stw_le_phys(target_phys_addr_t addr, uint32_t val);
>>>> +void stw_be_phys(target_phys_addr_t addr, uint32_t val);
>>>>  void stl_phys(target_phys_addr_t addr, uint32_t val);
>>>> +void stl_le_phys(target_phys_addr_t addr, uint32_t val);
>>>> +void stl_be_phys(target_phys_addr_t addr, uint32_t val);
>>>>  void stq_phys(target_phys_addr_t addr, uint64_t val);
>>>> +void stq_le_phys(target_phys_addr_t addr, uint64_t val);
>>>> +void stq_be_phys(target_phys_addr_t addr, uint64_t val);
>>>> 
>>>>  void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>>>                                    const uint8_t *buf, int len);
>>>> diff --git a/exec.c b/exec.c
>>>> index 4c45299..5f2f87e 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
>>>>     return val;
>>>>  }
>>>> 
>>>> +uint32_t ldl_le_phys(target_phys_addr_t addr)
>>>> +{
>>>> +#if defined(TARGET_WORDS_BIGENDIAN)
>>>> +    return bswap32(ldl_phys(addr));
>>> 
>>> This would introduce a second bswap in some cases. Please make instead
>>> two versions of ldl_phys which use ldl_le/be_p instead of ldl_p. Then
>>> ldl_phys could be #defined to the suitable function.
>> 
>> Yeah, I was thinking to do that one at first and then realized how MMIO gets tricky, since we also need to potentially swap it again, as by now it returns values in target CPU endianness. But if you prefer, I can dig into that.
> 
> Maybe the swapendian thing could be adjusted so that it's possible to
> access the device in either LE or BE way directly? For example, there
> could be two io_mem_read/write tables, the current one for CPU and
> another one for device MMIO with known endianness. Or even three
> tables: CPU, BE, LE.

If you take a look at the patches following this one, you can easily see how few devices actually use these functions. I don't think the additional memory usage would count up for a couple of byte swaps here really.

We could however try to be clever and directly check if the device mmio is a swapendian function and just bypass it. I'm just not sure it's worth the additional overhead for the non-swapped case (which should be the normal one).


Alex

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Blue Swirl]

On Wed, Jul 6, 2011 at 1:13 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 06.07.2011, at 00:05, Blue Swirl wrote:
>
>> On Wed, Jul 6, 2011 at 12:55 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 05.07.2011, at 23:48, Blue Swirl wrote:
>>>
>>>> On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> Device code some times needs to access physical memory and does that
>>>>> through the ld./st._phys functions. However, these are the exact same
>>>>> functions that the CPU uses to access memory, which means they will
>>>>> be endianness swapped depending on the target CPU.
>>>>>
>>>>> However, devices don't know about the CPU's endianness, but instead
>>>>> access memory directly using their own interface to the memory bus,
>>>>> so they need some way to read data with their native endianness.
>>>>>
>>>>> This patch adds _le and _be functions to ld./st._phys.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>>  cpu-common.h |   12 +++++++
>>>>>  exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 114 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/cpu-common.h b/cpu-common.h
>>>>> index b027e43..c6a2b5f 100644
>>>>> --- a/cpu-common.h
>>>>> +++ b/cpu-common.h
>>>>> @@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
>>>>>
>>>>>  uint32_t ldub_phys(target_phys_addr_t addr);
>>>>>  uint32_t lduw_phys(target_phys_addr_t addr);
>>>>> +uint32_t lduw_le_phys(target_phys_addr_t addr);
>>>>> +uint32_t lduw_be_phys(target_phys_addr_t addr);
>>>>>  uint32_t ldl_phys(target_phys_addr_t addr);
>>>>> +uint32_t ldl_le_phys(target_phys_addr_t addr);
>>>>> +uint32_t ldl_be_phys(target_phys_addr_t addr);
>>>>>  uint64_t ldq_phys(target_phys_addr_t addr);
>>>>> +uint64_t ldq_le_phys(target_phys_addr_t addr);
>>>>> +uint64_t ldq_be_phys(target_phys_addr_t addr);
>>>>>  void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
>>>>>  void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
>>>>>  void stb_phys(target_phys_addr_t addr, uint32_t val);
>>>>>  void stw_phys(target_phys_addr_t addr, uint32_t val);
>>>>> +void stw_le_phys(target_phys_addr_t addr, uint32_t val);
>>>>> +void stw_be_phys(target_phys_addr_t addr, uint32_t val);
>>>>>  void stl_phys(target_phys_addr_t addr, uint32_t val);
>>>>> +void stl_le_phys(target_phys_addr_t addr, uint32_t val);
>>>>> +void stl_be_phys(target_phys_addr_t addr, uint32_t val);
>>>>>  void stq_phys(target_phys_addr_t addr, uint64_t val);
>>>>> +void stq_le_phys(target_phys_addr_t addr, uint64_t val);
>>>>> +void stq_be_phys(target_phys_addr_t addr, uint64_t val);
>>>>>
>>>>>  void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>>>>                                    const uint8_t *buf, int len);
>>>>> diff --git a/exec.c b/exec.c
>>>>> index 4c45299..5f2f87e 100644
>>>>> --- a/exec.c
>>>>> +++ b/exec.c
>>>>> @@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
>>>>>     return val;
>>>>>  }
>>>>>
>>>>> +uint32_t ldl_le_phys(target_phys_addr_t addr)
>>>>> +{
>>>>> +#if defined(TARGET_WORDS_BIGENDIAN)
>>>>> +    return bswap32(ldl_phys(addr));
>>>>
>>>> This would introduce a second bswap in some cases. Please make instead
>>>> two versions of ldl_phys which use ldl_le/be_p instead of ldl_p. Then
>>>> ldl_phys could be #defined to the suitable function.
>>>
>>> Yeah, I was thinking to do that one at first and then realized how MMIO gets tricky, since we also need to potentially swap it again, as by now it returns values in target CPU endianness. But if you prefer, I can dig into that.
>>
>> Maybe the swapendian thing could be adjusted so that it's possible to
>> access the device in either LE or BE way directly? For example, there
>> could be two io_mem_read/write tables, the current one for CPU and
>> another one for device MMIO with known endianness. Or even three
>> tables: CPU, BE, LE.
>
> If you take a look at the patches following this one, you can easily see how few devices actually use these functions. I don't think the additional memory usage would count up for a couple of byte swaps here really.
>
> We could however try to be clever and directly check if the device mmio is a swapendian function and just bypass it. I'm just not sure it's worth the additional overhead for the non-swapped case (which should be the normal one).

Neither seems to be very attractive option. It may be enough to
optimize just RAM accesses and forget about extra bswap for MMIO for
now.

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Alexander Graf]

On 06.07.2011, at 00:22, Blue Swirl wrote:

> On Wed, Jul 6, 2011 at 1:13 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 06.07.2011, at 00:05, Blue Swirl wrote:
>> 
>>> On Wed, Jul 6, 2011 at 12:55 AM, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> On 05.07.2011, at 23:48, Blue Swirl wrote:
>>>> 
>>>>> On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>>> Device code some times needs to access physical memory and does that
>>>>>> through the ld./st._phys functions. However, these are the exact same
>>>>>> functions that the CPU uses to access memory, which means they will
>>>>>> be endianness swapped depending on the target CPU.
>>>>>> 
>>>>>> However, devices don't know about the CPU's endianness, but instead
>>>>>> access memory directly using their own interface to the memory bus,
>>>>>> so they need some way to read data with their native endianness.
>>>>>> 
>>>>>> This patch adds _le and _be functions to ld./st._phys.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>> ---
>>>>>>  cpu-common.h |   12 +++++++
>>>>>>  exec.c       |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 114 insertions(+), 0 deletions(-)
>>>>>> 
>>>>>> diff --git a/cpu-common.h b/cpu-common.h
>>>>>> index b027e43..c6a2b5f 100644
>>>>>> --- a/cpu-common.h
>>>>>> +++ b/cpu-common.h
>>>>>> @@ -135,14 +135,26 @@ void qemu_flush_coalesced_mmio_buffer(void);
>>>>>> 
>>>>>>  uint32_t ldub_phys(target_phys_addr_t addr);
>>>>>>  uint32_t lduw_phys(target_phys_addr_t addr);
>>>>>> +uint32_t lduw_le_phys(target_phys_addr_t addr);
>>>>>> +uint32_t lduw_be_phys(target_phys_addr_t addr);
>>>>>>  uint32_t ldl_phys(target_phys_addr_t addr);
>>>>>> +uint32_t ldl_le_phys(target_phys_addr_t addr);
>>>>>> +uint32_t ldl_be_phys(target_phys_addr_t addr);
>>>>>>  uint64_t ldq_phys(target_phys_addr_t addr);
>>>>>> +uint64_t ldq_le_phys(target_phys_addr_t addr);
>>>>>> +uint64_t ldq_be_phys(target_phys_addr_t addr);
>>>>>>  void stl_phys_notdirty(target_phys_addr_t addr, uint32_t val);
>>>>>>  void stq_phys_notdirty(target_phys_addr_t addr, uint64_t val);
>>>>>>  void stb_phys(target_phys_addr_t addr, uint32_t val);
>>>>>>  void stw_phys(target_phys_addr_t addr, uint32_t val);
>>>>>> +void stw_le_phys(target_phys_addr_t addr, uint32_t val);
>>>>>> +void stw_be_phys(target_phys_addr_t addr, uint32_t val);
>>>>>>  void stl_phys(target_phys_addr_t addr, uint32_t val);
>>>>>> +void stl_le_phys(target_phys_addr_t addr, uint32_t val);
>>>>>> +void stl_be_phys(target_phys_addr_t addr, uint32_t val);
>>>>>>  void stq_phys(target_phys_addr_t addr, uint64_t val);
>>>>>> +void stq_le_phys(target_phys_addr_t addr, uint64_t val);
>>>>>> +void stq_be_phys(target_phys_addr_t addr, uint64_t val);
>>>>>> 
>>>>>>  void cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>>>>>                                    const uint8_t *buf, int len);
>>>>>> diff --git a/exec.c b/exec.c
>>>>>> index 4c45299..5f2f87e 100644
>>>>>> --- a/exec.c
>>>>>> +++ b/exec.c
>>>>>> @@ -4158,6 +4158,24 @@ uint32_t ldl_phys(target_phys_addr_t addr)
>>>>>>     return val;
>>>>>>  }
>>>>>> 
>>>>>> +uint32_t ldl_le_phys(target_phys_addr_t addr)
>>>>>> +{
>>>>>> +#if defined(TARGET_WORDS_BIGENDIAN)
>>>>>> +    return bswap32(ldl_phys(addr));
>>>>> 
>>>>> This would introduce a second bswap in some cases. Please make instead
>>>>> two versions of ldl_phys which use ldl_le/be_p instead of ldl_p. Then
>>>>> ldl_phys could be #defined to the suitable function.
>>>> 
>>>> Yeah, I was thinking to do that one at first and then realized how MMIO gets tricky, since we also need to potentially swap it again, as by now it returns values in target CPU endianness. But if you prefer, I can dig into that.
>>> 
>>> Maybe the swapendian thing could be adjusted so that it's possible to
>>> access the device in either LE or BE way directly? For example, there
>>> could be two io_mem_read/write tables, the current one for CPU and
>>> another one for device MMIO with known endianness. Or even three
>>> tables: CPU, BE, LE.
>> 
>> If you take a look at the patches following this one, you can easily see how few devices actually use these functions. I don't think the additional memory usage would count up for a couple of byte swaps here really.
>> 
>> We could however try to be clever and directly check if the device mmio is a swapendian function and just bypass it. I'm just not sure it's worth the additional overhead for the non-swapped case (which should be the normal one).
> 
> Neither seems to be very attractive option. It may be enough to
> optimize just RAM accesses and forget about extra bswap for MMIO for
> now.

How about something like this on top? Plus the q, uw and st version of course. The inline is there on purpose, moving the be/le specific code (which is rarely used) out of the way from the often(?) used native ones. Eventually, TCG generated code comes into these, no?


diff --git a/exec.c b/exec.c
index 5f2f87e..f281ba4 100644
--- a/exec.c
+++ b/exec.c
@@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
 }
 
 /* warning: addr must be aligned */
-uint32_t ldl_phys(target_phys_addr_t addr)
+static inline uint32_t ldl_phys_internal(target_phys_addr_t addr,
+                                         enum device_endian endian)
 {
     int io_index;
     uint8_t *ptr;
@@ -4149,31 +4150,47 @@ uint32_t ldl_phys(target_phys_addr_t addr)
         if (p)
             addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset;
         val = io_mem_read[io_index][2](io_mem_opaque[io_index], addr);
+#if defined(TARGET_WORDS_BIGENDIAN)
+        if (endian == DEVICE_LITTLE_ENDIAN) {
+            val = bswap32(val);
+        }
+#else
+        if (endian == DEVICE_BIG_ENDIAN) {
+            val = bswap32(val);
+        }
+#endif
     } else {
         /* RAM case */
         ptr = qemu_get_ram_ptr(pd & TARGET_PAGE_MASK) +
             (addr & ~TARGET_PAGE_MASK);
-        val = ldl_p(ptr);
+        switch (endian) {
+        case DEVICE_LITTLE_ENDIAN:
+            val = ldl_le_p(ptr);
+            break;
+        case DEVICE_BIG_ENDIAN:
+            val = ldl_be_p(ptr);
+            break;
+        default:
+            val = ldl_p(ptr);
+            break;
+        }
     }
     return val;
 }
 
+uint32_t ldl_phys(target_phys_addr_t addr)
+{
+    return ldl_phys_internal(addr, DEVICE_NATIVE_ENDIAN);
+}
+
 uint32_t ldl_le_phys(target_phys_addr_t addr)
 {
-#if defined(TARGET_WORDS_BIGENDIAN)
-    return bswap32(ldl_phys(addr));
-#else
-    return ldl_phys(addr);
-#endif
+    return ldl_phys_internal(addr, DEVICE_LITTLE_ENDIAN);
 }
 
 uint32_t ldl_be_phys(target_phys_addr_t addr)
 {
-#if defined(TARGET_WORDS_BIGENDIAN)
-    return ldl_phys(addr);
-#else
-    return bswap32(ldl_phys(addr));
-#endif
+    return ldl_phys_internal(addr, DEVICE_BIG_ENDIAN);
 }
 
 /* warning: addr must be aligned */


Alex

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Paolo Bonzini]

> diff --git a/exec.c b/exec.c
> index 5f2f87e..f281ba4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
>   }
>
>   /* warning: addr must be aligned */
> -uint32_t ldl_phys(target_phys_addr_t addr)
> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr,
> +                                         enum device_endian endian)

You probably need the __always_inline__ attribute to really convince GCC 
to inline this.

Paolo

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Alexander Graf]

On 06.07.2011, at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote:

>> diff --git a/exec.c b/exec.c
>> index 5f2f87e..f281ba4 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
>>  }
>> 
>>  /* warning: addr must be aligned */
>> -uint32_t ldl_phys(target_phys_addr_t addr)
>> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr,
>> +                                         enum device_endian endian)
> 
> You probably need the __always_inline__ attribute to really convince GCC to inline this.

There's a define in osdep.h that converts inline into always_inline :)

Alex

> 

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Hannes Reinecke]

On 07/06/2011 01:34 PM, Alexander Graf wrote:
>
>
>
>
> On 06.07.2011, at 12:24, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>
>>> diff --git a/exec.c b/exec.c
>>> index 5f2f87e..f281ba4 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
>>>   }
>>>
>>>   /* warning: addr must be aligned */
>>> -uint32_t ldl_phys(target_phys_addr_t addr)
>>> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr,
>>> +                                         enum device_endian endian)
>>
>> You probably need the __always_inline__ attribute to really convince GCC to inline this.
>
> There's a define in osdep.h that converts inline into always_inline :)
>
Btw, while you're at it:

uint32_t ldub_phys(target_phys_addr_t addr);
uint32_t lduw_phys(target_phys_addr_t addr);

Hmm? ldub is supposed to read an 'unsigned byte' (uint8_t),
and lduw is supposed to read an 'unsigned word' (uint16_t).

Why does it return an uint32_t?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Alexander Graf]

Am 06.07.2011 um 15:03 schrieb Hannes Reinecke <hare@suse.de>:

> On 07/06/2011 01:34 PM, Alexander Graf wrote:
>> 
>> 
>> 
>> 
>> On 06.07.2011, at 12:24, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>> 
>>>> diff --git a/exec.c b/exec.c
>>>> index 5f2f87e..f281ba4 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
>>>>  }
>>>> 
>>>>  /* warning: addr must be aligned */
>>>> -uint32_t ldl_phys(target_phys_addr_t addr)
>>>> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr,
>>>> +                                         enum device_endian endian)
>>> 
>>> You probably need the __always_inline__ attribute to really convince GCC to inline this.
>> 
>> There's a define in osdep.h that converts inline into always_inline :)
>> 
> Btw, while you're at it:
> 
> uint32_t ldub_phys(target_phys_addr_t addr);
> uint32_t lduw_phys(target_phys_addr_t addr);
> 
> Hmm? ldub is supposed to read an 'unsigned byte' (uint8_t),
> and lduw is supposed to read an 'unsigned word' (uint16_t).
> 
> Why does it return an uint32_t?

Because it ends up being a 32-bit register for the return value / parameter anyways :).

But this is a different thing. I'd prefer to  focus on the endian problem for now.


Alex

Re: [Qemu-devel] [PATCH 1/9] exec: add endian specific phys ld/st functions

[Paolo Bonzini]

On 07/06/2011 03:03 PM, Hannes Reinecke wrote:
>
> uint32_t ldub_phys(target_phys_addr_t addr);
> uint32_t lduw_phys(target_phys_addr_t addr);
>
> Hmm? ldub is supposed to read an 'unsigned byte' (uint8_t),
> and lduw is supposed to read an 'unsigned word' (uint16_t).
>
> Why does it return an uint32_t?

I don't know if this is the reason, but uint{8,16}_t are promoted to a 
signed int.  So when you do

   (uint64_t) (ldub_phys (addr) << 24)

you'd get a sign extension in bits 32-63.  Admittedly a bit contrived, 
but it can happen and QEMU is full of such bugs:

     case 4:
         lba = (uint64_t) buf[9] | ((uint64_t) buf[8] << 8) |
               ((uint64_t) buf[7] << 16) | ((uint64_t) buf[6] << 24) |
               ((uint64_t) buf[5] << 32) | ((uint64_t) buf[4] << 40) |
               ((uint64_t) buf[3] << 48) | ((uint64_t) buf[2] << 56);
         break;

(found by Coverity).

This was the reason for my series at 
http://permalink.gmane.org/gmane.comp.emulators.qemu/105336 (which you 
reminded me to ping---thanks!)

Paolo

[Qemu-devel] [PATCH 2/9] hpet: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/hpet.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index ef9a2a0..4eda33d 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -192,7 +192,7 @@ static void update_irq(struct HPETTimer *timer, int set)
             qemu_irq_lower(s->irqs[route]);
         }
     } else if (timer_fsb_route(timer)) {
-        stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+        stl_le_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
     } else if (timer->config & HPET_TN_TYPE_LEVEL) {
         s->isr |= mask;
         qemu_irq_raise(s->irqs[route]);
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/9] intel-hda: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/intel-hda.c |   21 ++++-----------------
 1 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 0ffffce..5a2bc3a 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -224,19 +224,6 @@ static target_phys_addr_t intel_hda_addr(uint32_t lbase, uint32_t ubase)
     return addr;
 }
 
-static void stl_phys_le(target_phys_addr_t addr, uint32_t value)
-{
-    uint32_t value_le = cpu_to_le32(value);
-    cpu_physical_memory_write(addr, (uint8_t*)(&value_le), sizeof(value_le));
-}
-
-static uint32_t ldl_phys_le(target_phys_addr_t addr)
-{
-    uint32_t value_le;
-    cpu_physical_memory_read(addr, (uint8_t*)(&value_le), sizeof(value_le));
-    return le32_to_cpu(value_le);
-}
-
 static void intel_hda_update_int_sts(IntelHDAState *d)
 {
     uint32_t sts = 0;
@@ -341,7 +328,7 @@ static void intel_hda_corb_run(IntelHDAState *d)
 
         rp = (d->corb_rp + 1) & 0xff;
         addr = intel_hda_addr(d->corb_lbase, d->corb_ubase);
-        verb = ldl_phys_le(addr + 4*rp);
+        verb = ldl_le_phys(addr + 4*rp);
         d->corb_rp = rp;
 
         dprint(d, 2, "%s: [rp 0x%x] verb 0x%08x\n", __FUNCTION__, rp, verb);
@@ -373,8 +360,8 @@ static void intel_hda_response(HDACodecDevice *dev, bool solicited, uint32_t res
     ex = (solicited ? 0 : (1 << 4)) | dev->cad;
     wp = (d->rirb_wp + 1) & 0xff;
     addr = intel_hda_addr(d->rirb_lbase, d->rirb_ubase);
-    stl_phys_le(addr + 8*wp, response);
-    stl_phys_le(addr + 8*wp + 4, ex);
+    stl_le_phys(addr + 8*wp, response);
+    stl_le_phys(addr + 8*wp + 4, ex);
     d->rirb_wp = wp;
 
     dprint(d, 2, "%s: [wp 0x%x] response 0x%x, extra 0x%x\n",
@@ -461,7 +448,7 @@ static bool intel_hda_xfer(HDACodecDevice *dev, uint32_t stnr, bool output,
     }
     if (d->dp_lbase & 0x01) {
         addr = intel_hda_addr(d->dp_lbase & ~0x01, d->dp_ubase);
-        stl_phys_le(addr + 8*s, st->lpib);
+        stl_le_phys(addr + 8*s, st->lpib);
     }
     dprint(d, 3, "dma: --\n");
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/9] msi: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/msi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index e8c5607..f214fcf 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -249,7 +249,7 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
                    "notify vector 0x%x"
                    " address: 0x%"PRIx64" data: 0x%"PRIx32"\n",
                    vector, address, data);
-    stl_phys(address, data);
+    stl_le_phys(address, data);
 }
 
 /* call this function after updating configs by pci_default_write_config(). */
-- 
1.7.3.4

[Qemu-devel] [PATCH 5/9] msix: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/msix.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 03d7bec..e67e700 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -359,7 +359,7 @@ void msix_notify(PCIDevice *dev, unsigned vector)
 
     address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
     data = pci_get_long(table_entry + PCI_MSIX_ENTRY_DATA);
-    stl_phys(address, data);
+    stl_le_phys(address, data);
 }
 
 void msix_reset(PCIDevice *dev)
-- 
1.7.3.4

[Qemu-devel] [PATCH 6/9] pl080: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/pl080.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pl080.c b/hw/pl080.c
index 901f04a..dd8139b 100644
--- a/hw/pl080.c
+++ b/hw/pl080.c
@@ -199,10 +199,10 @@ again:
             if (size == 0) {
                 /* Transfer complete.  */
                 if (ch->lli) {
-                    ch->src = ldl_phys(ch->lli);
-                    ch->dest = ldl_phys(ch->lli + 4);
-                    ch->ctrl = ldl_phys(ch->lli + 12);
-                    ch->lli = ldl_phys(ch->lli + 8);
+                    ch->src = ldl_le_phys(ch->lli);
+                    ch->dest = ldl_le_phys(ch->lli + 4);
+                    ch->ctrl = ldl_le_phys(ch->lli + 12);
+                    ch->lli = ldl_le_phys(ch->lli + 8);
                 } else {
                     ch->conf &= ~PL080_CCONF_E;
                 }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 6/9] pl080: use specific endian ld/st_phys

[Peter Maydell]

On 5 July 2011 17:28, Alexander Graf <agraf@suse.de> wrote:
> --- a/hw/pl080.c
> +++ b/hw/pl080.c
> @@ -199,10 +199,10 @@ again:
>             if (size == 0) {
>                 /* Transfer complete.  */
>                 if (ch->lli) {
> -                    ch->src = ldl_phys(ch->lli);
> -                    ch->dest = ldl_phys(ch->lli + 4);
> -                    ch->ctrl = ldl_phys(ch->lli + 12);
> -                    ch->lli = ldl_phys(ch->lli + 8);
> +                    ch->src = ldl_le_phys(ch->lli);
> +                    ch->dest = ldl_le_phys(ch->lli + 4);
> +                    ch->ctrl = ldl_le_phys(ch->lli + 12);
> +                    ch->lli = ldl_le_phys(ch->lli + 8);
>                 } else {
>                     ch->conf &= ~PL080_CCONF_E;
>                 }

(Mostly this email is to get this info into the archive before I
forget; we talked about it on IRC. It's not intended to be a nak,
which is just as well given the patch is already committed :-))

This is no worse than the existing code, but it's not actually
the right behaviour. Bit 0 of ch->lli indicates which AHB master
we make the request on; you use that to identify which of bits
1 and 2 in the DMACConfiguration Register to test to determine
whether to be little endian or big endian for each master:

  if (ch->conf & (PL080_CONF_M1 << (ch->lli & 1))) {
     bigendian;
  } else {
     littleendian;
  }

However since that function pl080_run() has this early on:
 hw_error("DMA active\n");
we'll never reach this code, so it's all a bit moot :-)

(There are no doubt other bits of that code which would need
fixing to properly support bigendian DMA too, which is a bit
much effort for functionality that isn't used.)

-- PMM

[Qemu-devel] [PATCH 7/9] ppc405_uc: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc405_uc.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/hw/ppc405_uc.c b/hw/ppc405_uc.c
index 2ce79ee..06a053b 100644
--- a/hw/ppc405_uc.c
+++ b/hw/ppc405_uc.c
@@ -51,39 +51,42 @@ ram_addr_t ppc405_set_bootinfo (CPUState *env, ppc4xx_bd_info_t *bd,
         bdloc = 0x01000000UL - sizeof(struct ppc4xx_bd_info_t);
     else
         bdloc = bd->bi_memsize - sizeof(struct ppc4xx_bd_info_t);
-    stl_phys(bdloc + 0x00, bd->bi_memstart);
-    stl_phys(bdloc + 0x04, bd->bi_memsize);
-    stl_phys(bdloc + 0x08, bd->bi_flashstart);
-    stl_phys(bdloc + 0x0C, bd->bi_flashsize);
-    stl_phys(bdloc + 0x10, bd->bi_flashoffset);
-    stl_phys(bdloc + 0x14, bd->bi_sramstart);
-    stl_phys(bdloc + 0x18, bd->bi_sramsize);
-    stl_phys(bdloc + 0x1C, bd->bi_bootflags);
-    stl_phys(bdloc + 0x20, bd->bi_ipaddr);
-    for (i = 0; i < 6; i++)
+    stl_be_phys(bdloc + 0x00, bd->bi_memstart);
+    stl_be_phys(bdloc + 0x04, bd->bi_memsize);
+    stl_be_phys(bdloc + 0x08, bd->bi_flashstart);
+    stl_be_phys(bdloc + 0x0C, bd->bi_flashsize);
+    stl_be_phys(bdloc + 0x10, bd->bi_flashoffset);
+    stl_be_phys(bdloc + 0x14, bd->bi_sramstart);
+    stl_be_phys(bdloc + 0x18, bd->bi_sramsize);
+    stl_be_phys(bdloc + 0x1C, bd->bi_bootflags);
+    stl_be_phys(bdloc + 0x20, bd->bi_ipaddr);
+    for (i = 0; i < 6; i++) {
         stb_phys(bdloc + 0x24 + i, bd->bi_enetaddr[i]);
-    stw_phys(bdloc + 0x2A, bd->bi_ethspeed);
-    stl_phys(bdloc + 0x2C, bd->bi_intfreq);
-    stl_phys(bdloc + 0x30, bd->bi_busfreq);
-    stl_phys(bdloc + 0x34, bd->bi_baudrate);
-    for (i = 0; i < 4; i++)
+    }
+    stw_be_phys(bdloc + 0x2A, bd->bi_ethspeed);
+    stl_be_phys(bdloc + 0x2C, bd->bi_intfreq);
+    stl_be_phys(bdloc + 0x30, bd->bi_busfreq);
+    stl_be_phys(bdloc + 0x34, bd->bi_baudrate);
+    for (i = 0; i < 4; i++) {
         stb_phys(bdloc + 0x38 + i, bd->bi_s_version[i]);
+    }
     for (i = 0; i < 32; i++) {
         stb_phys(bdloc + 0x3C + i, bd->bi_r_version[i]);
     }
-    stl_phys(bdloc + 0x5C, bd->bi_plb_busfreq);
-    stl_phys(bdloc + 0x60, bd->bi_pci_busfreq);
-    for (i = 0; i < 6; i++)
+    stl_be_phys(bdloc + 0x5C, bd->bi_plb_busfreq);
+    stl_be_phys(bdloc + 0x60, bd->bi_pci_busfreq);
+    for (i = 0; i < 6; i++) {
         stb_phys(bdloc + 0x64 + i, bd->bi_pci_enetaddr[i]);
+    }
     n = 0x6A;
     if (flags & 0x00000001) {
         for (i = 0; i < 6; i++)
             stb_phys(bdloc + n++, bd->bi_pci_enetaddr2[i]);
     }
-    stl_phys(bdloc + n, bd->bi_opbfreq);
+    stl_be_phys(bdloc + n, bd->bi_opbfreq);
     n += 4;
     for (i = 0; i < 2; i++) {
-        stl_phys(bdloc + n, bd->bi_iic_fast[i]);
+        stl_be_phys(bdloc + n, bd->bi_iic_fast[i]);
         n += 4;
     }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 8/9] s390-virtio: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/s390-virtio-bus.c |   10 +++++-----
 hw/s390-virtio.c     |    6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index d4a12f7..dde6ba5 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -165,7 +165,7 @@ static uint64_t s390_virtio_device_vq_token(VirtIOS390Device *dev, int vq)
                 (vq * VIRTIO_VQCONFIG_LEN) +
                 VIRTIO_VQCONFIG_OFFS_TOKEN;
 
-    return ldq_phys(token_off);
+    return ldq_be_phys(token_off);
 }
 
 static ram_addr_t s390_virtio_device_num_vq(VirtIOS390Device *dev)
@@ -219,8 +219,8 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
         vring = s390_virtio_next_ring(bus);
         virtio_queue_set_addr(dev->vdev, i, vring);
         virtio_queue_set_vector(dev->vdev, i, i);
-        stq_phys(vq + VIRTIO_VQCONFIG_OFFS_ADDRESS, vring);
-        stw_phys(vq + VIRTIO_VQCONFIG_OFFS_NUM, virtio_queue_get_num(dev->vdev, i));
+        stq_be_phys(vq + VIRTIO_VQCONFIG_OFFS_ADDRESS, vring);
+        stw_be_phys(vq + VIRTIO_VQCONFIG_OFFS_NUM, virtio_queue_get_num(dev->vdev, i));
     }
 
     cur_offs = dev->dev_offs;
@@ -228,7 +228,7 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
     cur_offs += num_vq * VIRTIO_VQCONFIG_LEN;
 
     /* Sync feature bitmap */
-    stl_phys(cur_offs, bswap32(dev->host_features));
+    stl_le_phys(cur_offs, dev->host_features);
 
     dev->feat_offs = cur_offs + dev->feat_len;
     cur_offs += dev->feat_len * 2;
@@ -252,7 +252,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
 
     /* Update guest supported feature bitmap */
 
-    features = bswap32(ldl_phys(dev->feat_offs));
+    features = bswap32(ldl_be_phys(dev->feat_offs));
     if (vdev->set_features) {
         vdev->set_features(vdev, features);
     }
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 3eba7ea..abe954d 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -193,7 +193,7 @@ static void s390_init(ram_addr_t my_ram_size,
     if (kernel_filename) {
         kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
 
-        if (lduw_phys(KERN_IMAGE_START) != 0x0dd0) {
+        if (lduw_be_phys(KERN_IMAGE_START) != 0x0dd0) {
             fprintf(stderr, "Specified image is not an s390 boot image\n");
             exit(1);
         }
@@ -232,8 +232,8 @@ static void s390_init(ram_addr_t my_ram_size,
         }
         initrd_size = load_image(initrd_filename, qemu_get_ram_ptr(initrd_offset));
 
-        stq_phys(INITRD_PARM_START, initrd_offset);
-        stq_phys(INITRD_PARM_SIZE, initrd_size);
+        stq_be_phys(INITRD_PARM_START, initrd_offset);
+        stq_be_phys(INITRD_PARM_SIZE, initrd_size);
     }
 
     if (kernel_cmdline) {
-- 
1.7.3.4

[Qemu-devel] [PATCH 9/9] spapr: use specific endian ld/st_phys

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/spapr.h       |    4 ++--
 hw/spapr_hcall.c |   12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/spapr.h b/hw/spapr.h
index b52133a..263691b 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -280,12 +280,12 @@ target_ulong spapr_hypercall(CPUState *env, target_ulong opcode,
 
 static inline uint32_t rtas_ld(target_ulong phys, int n)
 {
-    return ldl_phys(phys + 4*n);
+    return ldl_be_phys(phys + 4*n);
 }
 
 static inline void rtas_st(target_ulong phys, int n, uint32_t val)
 {
-    stl_phys(phys + 4*n, val);
+    stl_be_phys(phys + 4*n, val);
 }
 
 typedef void (*spapr_rtas_fn)(sPAPREnvironment *spapr, uint32_t token,
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index 84da8fc..5cd8d8f 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -278,7 +278,7 @@ static target_ulong register_vpa(CPUState *env, target_ulong vpa)
     }
     /* FIXME: bounds check the address */
 
-    size = lduw_phys(vpa + 0x4);
+    size = lduw_be_phys(vpa + 0x4);
 
     if (size < VPA_MIN_SIZE) {
         return H_PARAMETER;
@@ -321,7 +321,7 @@ static target_ulong register_slb_shadow(CPUState *env, target_ulong addr)
         return H_HARDWARE;
     }
 
-    size = ldl_phys(addr + 0x4);
+    size = ldl_be_phys(addr + 0x4);
     if (size < 0x8) {
         return H_PARAMETER;
     }
@@ -354,7 +354,7 @@ static target_ulong register_dtl(CPUState *env, target_ulong addr)
         return H_HARDWARE;
     }
 
-    size = ldl_phys(addr + 0x4);
+    size = ldl_be_phys(addr + 0x4);
 
     if (size < 48) {
         return H_PARAMETER;
@@ -441,9 +441,9 @@ static target_ulong h_rtas(CPUState *env, sPAPREnvironment *spapr,
                            target_ulong opcode, target_ulong *args)
 {
     target_ulong rtas_r3 = args[0];
-    uint32_t token = ldl_phys(rtas_r3);
-    uint32_t nargs = ldl_phys(rtas_r3 + 4);
-    uint32_t nret = ldl_phys(rtas_r3 + 8);
+    uint32_t token = ldl_be_phys(rtas_r3);
+    uint32_t nargs = ldl_be_phys(rtas_r3 + 4);
+    uint32_t nret = ldl_be_phys(rtas_r3 + 8);
 
     return spapr_rtas_call(spapr, token, nargs, rtas_r3 + 12,
                            nret, rtas_r3 + 12 + 4*nargs);
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 0/9] Don't use ld./st._phys in hw/

[Blue Swirl]

On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <agraf@suse.de> wrote:
> We have quite some code in hw/ that uses ld./st._phys functions
> in device emulation code. This is just pure wrong, as devices
> don't know about the CPU endianness (except for virtio), so they
> should instead use something endian specific.
>
> Unfortunately, there is no endian specific call to easily receive
> an integer from memory, so this patch set introduces some and then
> converts all the obvious users to them.
>
> I tested the targets I could, but please double-check if your
> architecture behaves differently or I accidently put in a _be
> instead of _le :)
>
> Alex
>
> Alexander Graf (9):
>  exec: add endian specific phys ld/st functions

Here I'd prefer a more optimal approach. Other patches looked OK, nice cleanup.

>  hpet: use specific endian ld/st_phys
>  intel-hda: use specific endian ld/st_phys
>  msi: use specific endian ld/st_phys
>  msix: use specific endian ld/st_phys
>  pl080: use specific endian ld/st_phys
>  ppc405_uc: use specific endian ld/st_phys
>  s390-virtio: use specific endian ld/st_phys
>  spapr: use specific endian ld/st_phys
>
>  cpu-common.h         |   12 ++++++
>  exec.c               |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/hpet.c            |    2 +-
>  hw/intel-hda.c       |   21 ++--------
>  hw/msi.c             |    2 +-
>  hw/msix.c            |    2 +-
>  hw/pl080.c           |    8 ++--
>  hw/ppc405_uc.c       |   43 +++++++++++----------
>  hw/s390-virtio-bus.c |   10 ++--
>  hw/s390-virtio.c     |    6 +-
>  hw/spapr.h           |    4 +-
>  hw/spapr_hcall.c     |   12 +++---
>  12 files changed, 164 insertions(+), 60 deletions(-)
>
> --
> 1.7.3.4
>
>
>