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

[Paolo Bonzini]

On 21/02/2017 19:44, Alex Williamson wrote:
>>> 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.  
>
> The part where I get lost is that if PPC64 always sets the target
> to big endian, then adjust_endianness() we will always bswap after a
> read and before a write.  I don't follow how that bswap necessarily
> gets us to QEMU CPU endian such that the cpu_to_leXX/leXX_to_cpu in the
> access functions always do the right thing.  I know we do this
> elsewhere in vfio, perhaps I've deferred to someone convincing me it
> was the right thing at the time, but I'm not able to derive it myself
> currently.
> 
> To answer Paolo's question, if PPC64 sets TARGET_WORDS_BIGENDIAN, which
> is a compile time setting, then using DEVICE_BIG_ENDIAN would
> deterministically remove the bswap from adjust_endianness(), 

And beNN_to_cpu/cpu_to_beNN would add a swap in 
memory_region_ram_device_read/write, so it would work just as well.

KVM expects data in host endianness:

static inline bool kvmppc_need_byteswap(struct kvm_vcpu *vcpu)
{
        return (kvmppc_get_msr(vcpu) & MSR_LE) != (MSR_KERNEL & MSR_LE);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^^^^
                 endianness of the target              endianness of
                                                      struct kvm_run
}

while QEMU hardcodes bigendian for DEVICE_NATIVE_ENDIAN, even if 
running on PPC64LE host.

However, I'm confused as to what would happen with TCG. :(  The best 
thing to do, IMO, is to add a RAM device MR to pc-testdev (e.g. at 0xec)

diff --git a/hw/misc/pc-testdev.c b/hw/misc/pc-testdev.c
index b81d820..5ea444f 100644
--- a/hw/misc/pc-testdev.c
+++ b/hw/misc/pc-testdev.c
@@ -47,11 +47,13 @@ typedef struct PCTestdev {
 
     MemoryRegion ioport;
     MemoryRegion ioport_byte;
+    MemoryRegion ioport_ramd;
     MemoryRegion flush;
     MemoryRegion irq;
     MemoryRegion iomem;
     uint32_t ioport_data;
     char iomem_buf[IOMEM_LEN];
+    char ioport_ramd_bytes[4];
 } PCTestdev;
 
 #define TYPE_TESTDEV "pc-testdev"
@@ -167,6 +169,10 @@ static void testdev_realizefn(DeviceState *d, Error **errp)
     memory_region_init_io(&dev->ioport_byte, OBJECT(dev),
                           &test_ioport_byte_ops, dev,
                           "pc-testdev-ioport-byte", 4);
+    memory_region_init_ram_device_ptr(&dev->ioport_ramd, OBJECT(dev),
+                                      "pc-testdev-ioport-ramd",
+                                      ARRAY_SIZE(dev->ioport_ramd_bytes),
+                                      &dev->ioport_ramd_bytes);
     memory_region_init_io(&dev->flush, OBJECT(dev), &test_flush_ops, dev,
                           "pc-testdev-flush-page", 4);
     memory_region_init_io(&dev->irq, OBJECT(dev), &test_irq_ops, dev,
@@ -177,6 +183,7 @@ static void testdev_realizefn(DeviceState *d, Error **errp)
     memory_region_add_subregion(io,  0xe0,       &dev->ioport);
     memory_region_add_subregion(io,  0xe4,       &dev->flush);
     memory_region_add_subregion(io,  0xe8,       &dev->ioport_byte);
+    memory_region_add_subregion(io,  0xec,       &dev->ioport_ramd);
     memory_region_add_subregion(io,  0x2000,     &dev->irq);
     memory_region_add_subregion(mem, 0xff000000, &dev->iomem);
 }

and a testcase to tests/endianness-test.c.

Paolo