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