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

[Yongji Xie]

on 2017/2/24 0:15, Paolo Bonzini wrote:

> On 23/02/2017 17:08, Peter Maydell wrote:
> > On 23 February 2017 at 15:58, Paolo Bonzini <address@hidden> wrote:
> > > However, DEVICE_NATIVE_ENDIAN would have to be paired with tswap, which
> > > the current code does not do, hence the bug.  To have no swap at all,
> > > you'd need DEVICE_HOST_ENDIAN.
> > Yes, I agree that the current ramdevice code has this bug (and
> > that we can fix it by any of the various options).
> Good. :)
>
> > > > AIUI what we want for this VFIO case is "when the guest does
> > > > a 32-bit write of 0x12345678 then the bytes are 0x12 0x34 0x56 0x78
> > > > regardless of whether TARGET_BIG_ENDIAN or not".
> > > No, I don't think so.  This is not specific to VFIO.  You can do it with
> > > any device, albeit VFIO is currently the only one using ramd regions.
> > The commit message in the patch that started this thread off
> > says specifically that "VFIO PCI device is little endian".
> > Is that wrong?
> Yes, I think it's a red herring.  Hence my initial confusion, when I
> asked "would Yongji's patch just work if it used DEVICE_BIG_ENDIAN and
> beNN_to_cpu/cpu_to_beNN".

Thank you for the great discussion. I have a better understanding of the
endianness now.:-)

And for the commit message, I was wrong to assume the same endianness
as vfio. That's my fault. This bug should happen when target and host
endianness are different regardless of the device endianness.

To fix it, introducing DEVICE_HOST_ENDIAN for the ram device seems to be
more reasonable than other ways. I think I'll update the patch with this 
way.

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,

Thanks,
Yongji