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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Philippe Mathieu-Daudé
remove unnecessary memory_region_big_endian()

Signed-off-by: Philippe Mathieu-Daudé <[hidden email]>
---
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;
 }
 
--
2.13.3


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Paolo Bonzini-5
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é <[hidden email]>
> ---
> 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;
>  }
>  
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Philippe Mathieu-Daudé
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é <[hidden email]>
>> ---
>> 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;
>>   }
>>  
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

KONRAD Frederic-2
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é <[hidden email]>
>>> ---
>>> 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;
>>>   }
>>>
>>

Loading...