Re: [Qemu-devel] [PATCH] memory: make ram device read/write endian sensitive

[Paolo Bonzini]

On 21/02/2017 17:21, Alex Williamson wrote:
> On Tue, 21 Feb 2017 14:46:55 +0800
> Yongji Xie <address@hidden> wrote:
> 
>> At the moment ram device's memory regions are NATIVE_ENDIAN. This does
>> not work on PPC64 because VFIO PCI device is little endian but PPC64
>> always defines static macro TARGET_WORDS_BIGENDIAN.
>>
>> This fixes endianness for ram device the same way as it is done
>> for VFIO region in commit 6758008e2c4e79fb6bd04fe8e7a41665fa583965.
> 
> The referenced commit was to vfio code where the endianness is fixed,
> here you're modifying shared generic code to assume the same
> endianness as vfio.  That seems wrong.

Is the goal to have the same endianness as VFIO?  Or is it just a trick
to ensure the number of swaps is always 0 or 2, so that they cancel away?

In other words, would Yongji's patch just work if it used
DEVICE_BIG_ENDIAN and beNN_to_cpu/cpu_to_beNN?  If so, then I think the
patch is okay.

Paolo

> 
> Alex
>  
>> Signed-off-by: Yongji Xie <address@hidden>
>> ---
>>  memory.c |   14 +++++++-------
>>  1 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index 6c58373..1ccb99f 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1139,13 +1139,13 @@ static uint64_t memory_region_ram_device_read(void 
>> *opaque,
>>          data = *(uint8_t *)(mr->ram_block->host + addr);
>>          break;
>>      case 2:
>> -        data = *(uint16_t *)(mr->ram_block->host + addr);
>> +        data = le16_to_cpu(*(uint16_t *)(mr->ram_block->host + addr));
>>          break;
>>      case 4:
>> -        data = *(uint32_t *)(mr->ram_block->host + addr);
>> +        data = le32_to_cpu(*(uint32_t *)(mr->ram_block->host + addr));
>>          break;
>>      case 8:
>> -        data = *(uint64_t *)(mr->ram_block->host + addr);
>> +        data = le64_to_cpu(*(uint64_t *)(mr->ram_block->host + addr));
>>          break;
>>      }
>>  
>> @@ -1166,13 +1166,13 @@ static void memory_region_ram_device_write(void 
>> *opaque, hwaddr addr,
>>          *(uint8_t *)(mr->ram_block->host + addr) = (uint8_t)data;
>>          break;
>>      case 2:
>> -        *(uint16_t *)(mr->ram_block->host + addr) = (uint16_t)data;
>> +        *(uint16_t *)(mr->ram_block->host + addr) = 
>> cpu_to_le16((uint16_t)data);
>>          break;
>>      case 4:
>> -        *(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;
>> +        *(uint32_t *)(mr->ram_block->host + addr) = 
>> cpu_to_le32((uint32_t)data);
>>          break;
>>      case 8:
>> -        *(uint64_t *)(mr->ram_block->host + addr) = data;
>> +        *(uint64_t *)(mr->ram_block->host + addr) = cpu_to_le64(data);
>>          break;
>>      }
>>  }
>> @@ -1180,7 +1180,7 @@ static void memory_region_ram_device_write(void 
>> *opaque, hwaddr addr,
>>  static const MemoryRegionOps ram_device_mem_ops = {
>>      .read = memory_region_ram_device_read,
>>      .write = memory_region_ram_device_write,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>      .valid = {
>>          .min_access_size = 1,
>>          .max_access_size = 8,
>