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 <address@hidden> wrote:
>> 
>> On 06.07.2011, at 00:05, Blue Swirl wrote:
>> 
>>> On Wed, Jul 6, 2011 at 12:55 AM, Alexander Graf <address@hidden> wrote:
>>>> 
>>>> On 05.07.2011, at 23:48, Blue Swirl wrote:
>>>> 
>>>>> On Tue, Jul 5, 2011 at 7:28 PM, Alexander Graf <address@hidden> 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 <address@hidden>
>>>>>> ---
>>>>>>  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