[Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[Yongji Xie]

At the moment ram device's memory regions are DEVICE_NATIVE_ENDIAN. It's
incorrect. This memory region is backed by a MMIO area in host, so the
uint64_t data that MemoryRegionOps read from/write to this area should be
host-endian rather than target-endian. Hence, current code does not work
when target and host endianness are different which is the most common case
on PPC64. To fix it, this introduces DEVICE_HOST_ENDIAN for the ram device.

This has been tested on PPC64 BE/LE host/guest in all possible combinations
including TCG.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 include/exec/cpu-common.h |    6 ++++++
 memory.c                  |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index bd15853..eef74df 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -36,6 +36,12 @@ enum device_endian {
     DEVICE_LITTLE_ENDIAN,
 };
 
+#if defined(HOST_WORDS_BIGENDIAN)
+#define DEVICE_HOST_ENDIAN DEVICE_BIG_ENDIAN
+#else
+#define DEVICE_HOST_ENDIAN DEVICE_LITTLE_ENDIAN
+#endif
+
 /* address in the RAM (different from a physical address) */
 #if defined(CONFIG_XEN_BACKEND)
 typedef uint64_t ram_addr_t;
diff --git a/memory.c b/memory.c
index ed8b5aa..17cfada 100644
--- a/memory.c
+++ b/memory.c
@@ -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_HOST_ENDIAN,
     .valid = {
         .min_access_size = 1,
         .max_access_size = 8,
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

On Mon, Feb 27, 2017 at 12:52:44PM +0800, Yongji Xie wrote:
> At the moment ram device's memory regions are DEVICE_NATIVE_ENDIAN. It's
> incorrect. This memory region is backed by a MMIO area in host, so the
> uint64_t data that MemoryRegionOps read from/write to this area should be
> host-endian rather than target-endian. Hence, current code does not work
> when target and host endianness are different which is the most common case
> on PPC64. To fix it, this introduces DEVICE_HOST_ENDIAN for the ram device.
> 
> This has been tested on PPC64 BE/LE host/guest in all possible combinations
> including TCG.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

The effect of the patch is certainly correct.  I remain a little
concerned that the name "host endian" might cause more confusion than
it resolves, but a better term isn't immediately obvious to me.

> ---
>  include/exec/cpu-common.h |    6 ++++++
>  memory.c                  |    2 +-
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index bd15853..eef74df 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -36,6 +36,12 @@ enum device_endian {
>      DEVICE_LITTLE_ENDIAN,
>  };
>  
> +#if defined(HOST_WORDS_BIGENDIAN)
> +#define DEVICE_HOST_ENDIAN DEVICE_BIG_ENDIAN
> +#else
> +#define DEVICE_HOST_ENDIAN DEVICE_LITTLE_ENDIAN
> +#endif
> +
>  /* address in the RAM (different from a physical address) */
>  #if defined(CONFIG_XEN_BACKEND)
>  typedef uint64_t ram_addr_t;
> diff --git a/memory.c b/memory.c
> index ed8b5aa..17cfada 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -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_HOST_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
>          .max_access_size = 8,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[Alexey Kardashevskiy]

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

On 28/02/17 11:41, David Gibson wrote:
> On Mon, Feb 27, 2017 at 12:52:44PM +0800, Yongji Xie wrote:
>> At the moment ram device's memory regions are DEVICE_NATIVE_ENDIAN. It's
>> incorrect. This memory region is backed by a MMIO area in host, so the
>> uint64_t data that MemoryRegionOps read from/write to this area should be
>> host-endian rather than target-endian. Hence, current code does not work
>> when target and host endianness are different which is the most common case
>> on PPC64. To fix it, this introduces DEVICE_HOST_ENDIAN for the ram device.
>>
>> This has been tested on PPC64 BE/LE host/guest in all possible combinations
>> including TCG.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> The effect of the patch is certainly correct.  I remain a little
> concerned that the name "host endian" might cause more confusion than
> it resolves, but a better term isn't immediately obvious to me.


In order to match memory_region_wrong_endianness(), it could be
DEVICE_CORRECT_ENDIAN :)

Just joking :)


> 
>> ---
>>  include/exec/cpu-common.h |    6 ++++++
>>  memory.c                  |    2 +-
>>  2 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
>> index bd15853..eef74df 100644
>> --- a/include/exec/cpu-common.h
>> +++ b/include/exec/cpu-common.h
>> @@ -36,6 +36,12 @@ enum device_endian {
>>      DEVICE_LITTLE_ENDIAN,
>>  };
>>  
>> +#if defined(HOST_WORDS_BIGENDIAN)
>> +#define DEVICE_HOST_ENDIAN DEVICE_BIG_ENDIAN
>> +#else
>> +#define DEVICE_HOST_ENDIAN DEVICE_LITTLE_ENDIAN
>> +#endif
>> +
>>  /* address in the RAM (different from a physical address) */
>>  #if defined(CONFIG_XEN_BACKEND)
>>  typedef uint64_t ram_addr_t;
>> diff --git a/memory.c b/memory.c
>> index ed8b5aa..17cfada 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -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_HOST_ENDIAN,
>>      .valid = {
>>          .min_access_size = 1,
>>          .max_access_size = 8,
> 


-- 
Alexey


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]

Re: [Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[Yongji Xie]

on 2017/2/28 8:41, David Gibson wrote:

> On Mon, Feb 27, 2017 at 12:52:44PM +0800, Yongji Xie wrote:
>> At the moment ram device's memory regions are DEVICE_NATIVE_ENDIAN. It's
>> incorrect. This memory region is backed by a MMIO area in host, so the
>> uint64_t data that MemoryRegionOps read from/write to this area should be
>> host-endian rather than target-endian. Hence, current code does not work
>> when target and host endianness are different which is the most common case
>> on PPC64. To fix it, this introduces DEVICE_HOST_ENDIAN for the ram device.
>>
>> This has been tested on PPC64 BE/LE host/guest in all possible combinations
>> including TCG.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> The effect of the patch is certainly correct.  I remain a little
> concerned that the name "host endian" might cause more confusion than
> it resolves, but a better term isn't immediately obvious to me.

If the memory region's endianness indicates the endianness of multi-byte 
value that
MemoryRegionOps read from/write to this memory region, should "host endian"
be reasonable?

For a mmio store, QEMU just get a bunch of bytes in the memory at the 
beginning.
Then we use ldX_p to load a target-endian multi-byte value from the 
memory.  Then
adjust_endianness() change the endianness of the multi-byte value from 
target-endian
to memory region's endianness.

For the mmap MMIO area, we should use host-endian multi-byte value to 
access it.

*(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;

Here it is the same as stl_he_p().

The "host-endian" means we load a bunch of bytes as a host-endian value, 
and write the
value to the mmap MMIO area. That's my understanding. Not sure if it's 
correct.

Thanks,
Yongji

Re: [Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

On Tue, Feb 28, 2017 at 06:12:56PM +0800, Yongji Xie wrote:
> on 2017/2/28 8:41, David Gibson wrote:
> 
> > On Mon, Feb 27, 2017 at 12:52:44PM +0800, Yongji Xie wrote:
> > > At the moment ram device's memory regions are DEVICE_NATIVE_ENDIAN. It's
> > > incorrect. This memory region is backed by a MMIO area in host, so the
> > > uint64_t data that MemoryRegionOps read from/write to this area should be
> > > host-endian rather than target-endian. Hence, current code does not work
> > > when target and host endianness are different which is the most common case
> > > on PPC64. To fix it, this introduces DEVICE_HOST_ENDIAN for the ram device.
> > > 
> > > This has been tested on PPC64 BE/LE host/guest in all possible combinations
> > > including TCG.
> > > 
> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > The effect of the patch is certainly correct.  I remain a little
> > concerned that the name "host endian" might cause more confusion than
> > it resolves, but a better term isn't immediately obvious to me.
> 
> If the memory region's endianness indicates the endianness of multi-byte
> value that
> MemoryRegionOps read from/write to this memory region, should "host endian"
> be reasonable?
> 
> For a mmio store, QEMU just get a bunch of bytes in the memory at the
> beginning.
> Then we use ldX_p to load a target-endian multi-byte value from the memory.
> Then
> adjust_endianness() change the endianness of the multi-byte value from
> target-endian
> to memory region's endianness.
> 
> For the mmap MMIO area, we should use host-endian multi-byte value to access
> it.
> 
> *(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;
> 
> Here it is the same as stl_he_p().
> 
> The "host-endian" means we load a bunch of bytes as a host-endian value, and
> write the
> value to the mmap MMIO area. That's my understanding. Not sure if it's
> correct.

That's correct.  The difficulty is that generally the endian flag
describes the device's endianness as it appears to the guest.  The
guest doesn't (and shouldn't) know the host's endianness, so
describing something as "host endian" is pretty weird from that point
of view.  Basically the only way this can work is if the qemu device
is treating all data from the guest as pieces of a bytestream and
never interpreting things as multibyte values.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] memory: Introduce DEVICE_HOST_ENDIAN for ram device

[Yongji Xie]

on 2017/3/1 8:35, David Gibson wrote:

> On Tue, Feb 28, 2017 at 06:12:56PM +0800, Yongji Xie wrote:
>> on 2017/2/28 8:41, David Gibson wrote:
>>
>>> On Mon, Feb 27, 2017 at 12:52:44PM +0800, Yongji Xie wrote:
>>>> At the moment ram device's memory regions are DEVICE_NATIVE_ENDIAN. It's
>>>> incorrect. This memory region is backed by a MMIO area in host, so the
>>>> uint64_t data that MemoryRegionOps read from/write to this area should be
>>>> host-endian rather than target-endian. Hence, current code does not work
>>>> when target and host endianness are different which is the most common case
>>>> on PPC64. To fix it, this introduces DEVICE_HOST_ENDIAN for the ram device.
>>>>
>>>> This has been tested on PPC64 BE/LE host/guest in all possible combinations
>>>> including TCG.
>>>>
>>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>>
>>> The effect of the patch is certainly correct.  I remain a little
>>> concerned that the name "host endian" might cause more confusion than
>>> it resolves, but a better term isn't immediately obvious to me.
>> If the memory region's endianness indicates the endianness of multi-byte
>> value that
>> MemoryRegionOps read from/write to this memory region, should "host endian"
>> be reasonable?
>>
>> For a mmio store, QEMU just get a bunch of bytes in the memory at the
>> beginning.
>> Then we use ldX_p to load a target-endian multi-byte value from the memory.
>> Then
>> adjust_endianness() change the endianness of the multi-byte value from
>> target-endian
>> to memory region's endianness.
>>
>> For the mmap MMIO area, we should use host-endian multi-byte value to access
>> it.
>>
>> *(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;
>>
>> Here it is the same as stl_he_p().
>>
>> The "host-endian" means we load a bunch of bytes as a host-endian value, and
>> write the
>> value to the mmap MMIO area. That's my understanding. Not sure if it's
>> correct.
> That's correct.  The difficulty is that generally the endian flag
> describes the device's endianness as it appears to the guest.  The
> guest doesn't (and shouldn't) know the host's endianness, so
> describing something as "host endian" is pretty weird from that point
> of view.  Basically the only way this can work is if the qemu device
> is treating all data from the guest as pieces of a bytestream and
> never interpreting things as multibyte values.
>

OK, I think I know what you mean. Indeed, it's hard to describe the ram 
device's
endianness from this point of view.  Just transfer the bytestream without
considering any endianness seems to be good.

Thanks,
Yongji