QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison
@ 2019-08-14 17:55 Dr. David Alan Gilbert (git)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 1/3] memory: Align MemoryRegionSections fields Dr. David Alan Gilbert (git)
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-14 17:55 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst
  Cc: peter.maydell, maxime.coquelin, marcandre.lureau

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This fixes a symptom I've seen on vhost-user on aarch64 where the
daemon would be falsely notified of memory region changes that didn't
exist.
The underlying problem was me memcmp'ing MemoryRegionSections even
though they had padding in.

(Discovered while getting virtiofs working on aarch)

Dave

v2
  Split 1st patch and fix spelling [Philippe's review]

Dr. David Alan Gilbert (3):
  memory: Align MemoryRegionSections fields
  memory: Provide an equality function for MemoryRegionSections
  vhost: Fix memory region section comparison

 hw/virtio/vhost.c     |  9 +++++++--
 include/exec/memory.h | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.21.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Qemu-devel] [PATCH v2 1/3] memory: Align MemoryRegionSections fields
  2019-08-14 17:55 [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
@ 2019-08-14 17:55 ` Dr. David Alan Gilbert (git)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 2/3] memory: Provide an equality function for MemoryRegionSections Dr. David Alan Gilbert (git)
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-14 17:55 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst
  Cc: peter.maydell, maxime.coquelin, marcandre.lureau

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

MemoryRegionSection includes an Int128 'size' field;
on some platforms the compiler causes an alignment of this to
a 128bit boundary, leaving 8 bytes of dead space.
This deadspace can be filled with junk.

Move the size field to the top avoiding unnecessary alignment.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 606250172a..a74a58c289 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -487,10 +487,10 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
  * @nonvolatile: this section is non-volatile
  */
 struct MemoryRegionSection {
+    Int128 size;
     MemoryRegion *mr;
     FlatView *fv;
     hwaddr offset_within_region;
-    Int128 size;
     hwaddr offset_within_address_space;
     bool readonly;
     bool nonvolatile;
-- 
2.21.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Qemu-devel] [PATCH v2 2/3] memory: Provide an equality function for MemoryRegionSections
  2019-08-14 17:55 [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 1/3] memory: Align MemoryRegionSections fields Dr. David Alan Gilbert (git)
@ 2019-08-14 17:55 ` Dr. David Alan Gilbert (git)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 3/3] vhost: Fix memory region section comparison Dr. David Alan Gilbert (git)
  2019-08-15  2:16 ` [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison no-reply
  3 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-14 17:55 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst
  Cc: peter.maydell, maxime.coquelin, marcandre.lureau

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Provide a comparison function that checks all the fields are the same.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/memory.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index a74a58c289..ce62e847bd 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -496,6 +496,18 @@ struct MemoryRegionSection {
     bool nonvolatile;
 };
 
+static inline bool MemoryRegionSection_eq(MemoryRegionSection *a,
+                                          MemoryRegionSection *b)
+{
+    return a->mr == b->mr &&
+           a->fv == b->fv &&
+           a->offset_within_region == b->offset_within_region &&
+           a->offset_within_address_space == b->offset_within_address_space &&
+           int128_eq(a->size, b->size) &&
+           a->readonly == b->readonly &&
+           a->nonvolatile == b->nonvolatile;
+}
+
 /**
  * memory_region_init: Initialize a memory region
  *
-- 
2.21.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] vhost: Fix memory region section comparison
  2019-08-14 17:55 [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 1/3] memory: Align MemoryRegionSections fields Dr. David Alan Gilbert (git)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 2/3] memory: Provide an equality function for MemoryRegionSections Dr. David Alan Gilbert (git)
@ 2019-08-14 17:55 ` Dr. David Alan Gilbert (git)
  2019-08-15  2:16 ` [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison no-reply
  3 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-14 17:55 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst
  Cc: peter.maydell, maxime.coquelin, marcandre.lureau

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Using memcmp to compare structures wasn't safe,
as I found out on ARM when I was getting falce miscompares.

Use the helper function for comparing the MRSs.

Fixes: ade6d081fc33948e56e6

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/vhost.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index bc899fc60e..2ef4bc720f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -451,8 +451,13 @@ static void vhost_commit(MemoryListener *listener)
         changed = true;
     } else {
         /* Same size, lets check the contents */
-        changed = n_old_sections && memcmp(dev->mem_sections, old_sections,
-                         n_old_sections * sizeof(old_sections[0])) != 0;
+        for (int i = 0; i < n_old_sections; i++) {
+            if (!MemoryRegionSection_eq(&old_sections[i],
+                                        &dev->mem_sections[i])) {
+                changed = true;
+                break;
+            }
+        }
     }
 
     trace_vhost_commit(dev->started, changed);
-- 
2.21.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison
  2019-08-14 17:55 [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
                   ` (2 preceding siblings ...)
  2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 3/3] vhost: Fix memory region section comparison Dr. David Alan Gilbert (git)
@ 2019-08-15  2:16 ` no-reply
  3 siblings, 0 replies; 5+ messages in thread
From: no-reply @ 2019-08-15  2:16 UTC (permalink / raw)
  To: dgilbert
  Cc: peter.maydell, mst, qemu-devel, maxime.coquelin,
	marcandre.lureau, pbonzini

Patchew URL: https://patchew.org/QEMU/20190814175535.2023-1-dgilbert@redhat.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

  CC      xtensa-linux-user/accel/stubs/kvm-stub.o
  CC      xtensa-linux-user/accel/tcg/tcg-runtime.o
  CC      xtensa-linux-user/accel/tcg/tcg-runtime-gvec.o
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-x86_64] Error 1
make: *** [Makefile:472: x86_64-linux-user/all] Error 2
make: *** Waiting for unfinished jobs....


The full log is available at
http://patchew.org/logs/20190814175535.2023-1-dgilbert@redhat.com/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 17:55 [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 1/3] memory: Align MemoryRegionSections fields Dr. David Alan Gilbert (git)
2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 2/3] memory: Provide an equality function for MemoryRegionSections Dr. David Alan Gilbert (git)
2019-08-14 17:55 ` [Qemu-devel] [PATCH v2 3/3] vhost: Fix memory region section comparison Dr. David Alan Gilbert (git)
2019-08-15  2:16 ` [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison no-reply

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox