[RFC] memory: fix access_with_adjusted_size() on big-endian

remove unnecessary memory_region_big_endian()

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
This is part of a branch with many cross-endianness tests for 2.11

Frederic does it helps your issues on armeb?

 memory.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

On 11/08/2017 16:06, Philippe Mathieu-Daudé wrote:
> remove unnecessary memory_region_big_endian()

Can you explain why it's unnecessary?...

Paolo

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> This is part of a branch with many cross-endianness tests for 2.11
> 
> Frederic does it helps your issues on armeb?
> 
>  memory.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index c0adc35410..600f5d5b1a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -338,15 +338,6 @@ static void flatview_simplify(FlatView *view)
>      }
>  }
>  
> -static bool memory_region_big_endian(MemoryRegion *mr)
> -{
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
> -#else
> -    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
> -#endif
> -}
> -
>  static bool memory_region_wrong_endianness(MemoryRegion *mr)
>  {
>  #ifdef TARGET_WORDS_BIGENDIAN
> @@ -572,7 +563,8 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>  {
>      uint64_t access_mask;
>      unsigned access_size;
> -    unsigned i;
> +    hwaddr access_addr;
> +    unsigned offset;
>      MemTxResult r = MEMTX_OK;
>  
>      if (!access_size_min) {
> @@ -585,17 +577,17 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>      /* FIXME: support unaligned access? */
>      access_size = MAX(MIN(size, access_size_max), access_size_min);
>      access_mask = -1ULL >> (64 - access_size * 8);
> -    if (memory_region_big_endian(mr)) {
> -        for (i = 0; i < size; i += access_size) {
> -            r |= access(mr, addr + i, value, access_size,
> -                        (size - access_size - i) * 8, access_mask, attrs);
> +    access_addr = addr & ~(access_size - 1);
> +    if (access_size < size) {
> +        for (offset = 0; offset < size; offset += access_size) {
> +            r |= access(mr, access_addr + offset, value, access_size,
> +                        offset * 8, access_mask, attrs);
>          }
>      } else {
> -        for (i = 0; i < size; i += access_size) {
> -            r |= access(mr, addr + i, value, access_size, i * 8,
> -                        access_mask, attrs);
> -        }
> +        r = access(mr, access_addr, value, access_size,
> +                    0, access_mask, attrs);
>      }
> +
>      return r;
>  }
>  
>

On 08/11/2017 11:25 AM, Paolo Bonzini wrote:
> On 11/08/2017 16:06, Philippe Mathieu-Daudé wrote:
>> remove unnecessary memory_region_big_endian()
> 
> Can you explain why it's unnecessary?...

I might have missed a lot here, I'm focusing on something else and 
thought this might help Frederic so I only cherry-picked this patch from 
a bigger series, since I memory_region_big_endian() is static/unused I 
just dropped it, which is surely wrong.

I see in this series I then call "adjust_endianness(mr, value, size)" 
before returning. And then lot of unit-tests to understand failures I'm 
having accessing PCI bus on armeb.

This branch is few months old and I'm out of context, I shouldn't have 
rushed to send this patch, I'll try to get some time during the week-end 
to remember.

Cheers,

Phil.

> 
> Paolo
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> This is part of a branch with many cross-endianness tests for 2.11
>>
>> Frederic does it helps your issues on armeb?
>>
>>   memory.c | 28 ++++++++++------------------
>>   1 file changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index c0adc35410..600f5d5b1a 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -338,15 +338,6 @@ static void flatview_simplify(FlatView *view)
>>       }
>>   }
>>   
>> -static bool memory_region_big_endian(MemoryRegion *mr)
>> -{
>> -#ifdef TARGET_WORDS_BIGENDIAN
>> -    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
>> -#else
>> -    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
>> -#endif
>> -}
>> -
>>   static bool memory_region_wrong_endianness(MemoryRegion *mr)
>>   {
>>   #ifdef TARGET_WORDS_BIGENDIAN
>> @@ -572,7 +563,8 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>>   {
>>       uint64_t access_mask;
>>       unsigned access_size;
>> -    unsigned i;
>> +    hwaddr access_addr;
>> +    unsigned offset;
>>       MemTxResult r = MEMTX_OK;
>>   
>>       if (!access_size_min) {
>> @@ -585,17 +577,17 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>>       /* FIXME: support unaligned access? */
>>       access_size = MAX(MIN(size, access_size_max), access_size_min);
>>       access_mask = -1ULL >> (64 - access_size * 8);
>> -    if (memory_region_big_endian(mr)) {
>> -        for (i = 0; i < size; i += access_size) {
>> -            r |= access(mr, addr + i, value, access_size,
>> -                        (size - access_size - i) * 8, access_mask, attrs);
>> +    access_addr = addr & ~(access_size - 1);
>> +    if (access_size < size) {
>> +        for (offset = 0; offset < size; offset += access_size) {
>> +            r |= access(mr, access_addr + offset, value, access_size,
>> +                        offset * 8, access_mask, attrs);
>>           }
>>       } else {
>> -        for (i = 0; i < size; i += access_size) {
>> -            r |= access(mr, addr + i, value, access_size, i * 8,
>> -                        access_mask, attrs);
>> -        }
>> +        r = access(mr, access_addr, value, access_size,
>> +                    0, access_mask, attrs);
>>       }
>> +
>>       return r;
>>   }
>>   
>>
>

Hi Philippe,

Thanks for that, I'll try this out when I'm back. The 15th is
blank holiday here.

Thanks,
Fred

On 08/11/2017 04:49 PM, Philippe Mathieu-Daudé wrote:
> On 08/11/2017 11:25 AM, Paolo Bonzini wrote:
>> On 11/08/2017 16:06, Philippe Mathieu-Daudé wrote:
>>> remove unnecessary memory_region_big_endian()
>>
>> Can you explain why it's unnecessary?...
> 
> I might have missed a lot here, I'm focusing on something else 
> and thought this might help Frederic so I only cherry-picked this 
> patch from a bigger series, since I memory_region_big_endian() is 
> static/unused I just dropped it, which is surely wrong.
> 
> I see in this series I then call "adjust_endianness(mr, value, 
> size)" before returning. And then lot of unit-tests to understand 
> failures I'm having accessing PCI bus on armeb.
> 
> This branch is few months old and I'm out of context, I shouldn't 
> have rushed to send this patch, I'll try to get some time during 
> the week-end to remember.
> 
> Cheers,
> 
> Phil.
> 
>>
>> Paolo
>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> This is part of a branch with many cross-endianness tests for 
>>> 2.11
>>>
>>> Frederic does it helps your issues on armeb?
>>>
>>>   memory.c | 28 ++++++++++------------------
>>>   1 file changed, 10 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index c0adc35410..600f5d5b1a 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -338,15 +338,6 @@ static void flatview_simplify(FlatView 
>>> *view)
>>>       }
>>>   }
>>> -static bool memory_region_big_endian(MemoryRegion *mr)
>>> -{
>>> -#ifdef TARGET_WORDS_BIGENDIAN
>>> -    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
>>> -#else
>>> -    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
>>> -#endif
>>> -}
>>> -
>>>   static bool memory_region_wrong_endianness(MemoryRegion *mr)
>>>   {
>>>   #ifdef TARGET_WORDS_BIGENDIAN
>>> @@ -572,7 +563,8 @@ static MemTxResult 
>>> access_with_adjusted_size(hwaddr addr,
>>>   {
>>>       uint64_t access_mask;
>>>       unsigned access_size;
>>> -    unsigned i;
>>> +    hwaddr access_addr;
>>> +    unsigned offset;
>>>       MemTxResult r = MEMTX_OK;
>>>       if (!access_size_min) {
>>> @@ -585,17 +577,17 @@ static MemTxResult 
>>> access_with_adjusted_size(hwaddr addr,
>>>       /* FIXME: support unaligned access? */
>>>       access_size = MAX(MIN(size, access_size_max), 
>>> access_size_min);
>>>       access_mask = -1ULL >> (64 - access_size * 8);
>>> -    if (memory_region_big_endian(mr)) {
>>> -        for (i = 0; i < size; i += access_size) {
>>> -            r |= access(mr, addr + i, value, access_size,
>>> -                        (size - access_size - i) * 8, 
>>> access_mask, attrs);
>>> +    access_addr = addr & ~(access_size - 1);
>>> +    if (access_size < size) {
>>> +        for (offset = 0; offset < size; offset += access_size) {
>>> +            r |= access(mr, access_addr + offset, value, 
>>> access_size,
>>> +                        offset * 8, access_mask, attrs);
>>>           }
>>>       } else {
>>> -        for (i = 0; i < size; i += access_size) {
>>> -            r |= access(mr, addr + i, value, access_size, i * 8,
>>> -                        access_mask, attrs);
>>> -        }
>>> +        r = access(mr, access_addr, value, access_size,
>>> +                    0, access_mask, attrs);
>>>       }
>>> +
>>>       return r;
>>>   }
>>>
>>