[Qemu-arm] [Qemu-devel] [PATCH v2 11/20] hw/virtio: Access MemoryRegion with MemOp

[tony.nguyen]

On 17/07/19 08:06, Paolo Bonzini wrote:

> My main concern is that MO_BE/MO_LE/MO_TE do not really apply to the 

> memory.c paths.  MO_BSWAP is never passed into the MemOp, even if target 

> endianness != host endianness.

> 

> Therefore, you could return MO_TE | MO_{8,16,32,64} from this function, 

> and change memory_region_endianness_inverted to test 

> HOST_WORDS_BIGENDIAN instead of TARGET_WORDS_BIGENDIAN.  Then the two

> MO_BSWAPs (one from MO_TE, one from adjust_endianness because

> memory_region_endianness_inverted returns true) cancel out if the

> memory region's endianness is the same as the host's but different

> from the target's.

> 

> Some care is needed in virtio_address_space_write and zpci_write_bar.  I 

> think the latter is okay, while the former could do something like this:

> 

> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c

> index ce928f2429..61885f020c 100644

> --- a/hw/virtio/virtio-pci.c

> +++ b/hw/virtio/virtio-pci.c

> @@ -541,16 +541,16 @@ 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;

>      }

> -    memory_region_dispatch_write(mr, addr, val, len, MEMTXATTRS_UNSPECIFIED);

> +    memory_region_dispatch_write(mr, addr, val, size_memop(len) & ~MO_BSWAP, 

> MEMTXATTRS_UNSPECIFIED);

>  }

>  

>  static void

Sorry Paolo, I noted the need to take care in virtio_address_space_write and

zpci_write_bar but did not understand.

> Some care is needed in virtio_address_space_write and zpci_write_bar.

Is this advice for my v1 implementation, or in the case of the

MO_TE | MO_{8,16,32,64} idead suggested in the paragraph before?

Signed-off-by: Tony Nguyen <address@hidden>

---

 hw/virtio/virtio-pci.c | 7 +++++--

 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c

index ce928f2..265f066 100644

--- a/hw/virtio/virtio-pci.c

+++ b/hw/virtio/virtio-pci.c

@@ -17,6 +17,7 @@

 

 #include "qemu/osdep.h"

 

+#include "exec/memop.h"

 #include "standard-headers/linux/virtio_pci.h"

 #include "hw/virtio/virtio.h"

 #include "hw/pci/pci.h"

@@ -550,7 +551,8 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr,

         /* As length is under guest control, handle illegal values. */

         return;

     }

-    memory_region_dispatch_write(mr, addr, val, len, MEMTXATTRS_UNSPECIFIED);

+    memory_region_dispatch_write(mr, addr, val, SIZE_MEMOP(len),

+                                 MEMTXATTRS_UNSPECIFIED);

 }

 

 static void

@@ -573,7 +575,8 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr,

     /* Make sure caller aligned buf properly */

     assert(!(((uintptr_t)buf) & (len - 1)));

 

-    memory_region_dispatch_read(mr, addr, &val, len, MEMTXATTRS_UNSPECIFIED);

+    memory_region_dispatch_read(mr, addr, &val, SIZE_MEMOP(len),

+                                MEMTXATTRS_UNSPECIFIED);

     switch (len) {

     case 1:

         pci_set_byte(buf, val);

-- 

1.8.3.1