Re: [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

[Richard Henderson]

On 8/7/19 11:00 AM, Paolo Bonzini wrote:
> On 07/08/19 19:49, Richard Henderson wrote:
>> On 8/7/19 1:33 AM, address@hidden wrote:
>>> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>>> hwaddr addr,
>>>          /* As length is under guest control, handle illegal values. */
>>>          return;
>>>      }
>>> +    /* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>>      memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>>                                   MEMTXATTRS_UNSPECIFIED);
>>>  }
>>
>> Here is an example of where Paolo is quite right -- you cannot simply add 
>> MO_TE
>> via size_memop().  In patch 22 we see
>>
>>> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy 
>>> *proxy, hwaddr addr,
>>>          val = pci_get_byte(buf);
>>>          break;
>>>      case 2:
>>> -        val = cpu_to_le16(pci_get_word(buf));
>>> +        val = pci_get_word(buf);
>>>          break;
>>>      case 4:
>>> -        val = cpu_to_le32(pci_get_long(buf));
>>> +        val = pci_get_long(buf);
>>>          break;
>>>      default:
>>>          /* As length is under guest control, handle illegal values. */
>>>          return;
>>>      }
>>> -    /* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>>      memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>>                                   MEMTXATTRS_UNSPECIFIED);
>>
>> This is a little-endian store -- MO_LE not MO_TE.
> 
> Or leave the switch statement aside and request host endianness.  Either
> is fine.

The goal is to minimize the number of places and the number of times that
bswaps happen.  That's why I think pushing the cpu_to_le16 into
memory_region_dispatch_write is a good thing.

However, leaving a host-endian might be the easiest short-term solution for the
more complicated cases.  It also seems like a way to split the patch further if
we think that's desirable.


r~