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


Alex