[Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison

[Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison

[Dr. David Alan Gilbert (git)]

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

[Qemu-devel] [PATCH v2 1/3] memory: Align MemoryRegionSections fields

[Dr. David Alan Gilbert (git)]

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

[Qemu-devel] [PATCH v2 2/3] memory: Provide an equality function for MemoryRegionSections

[Dr. David Alan Gilbert (git)]

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

[Qemu-devel] [PATCH v2 3/3] vhost: Fix memory region section comparison

[Dr. David Alan Gilbert (git)]

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

Re: [Qemu-devel] [PATCH v2 0/3] Fix MemoryRegionSection alignment and comparison

[no-reply]

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

[PULL 0/3] vhost: fixes

[Michael S. Tsirkin]

A very quiet cycle. Did I miss a bunch of patches? if so, let me know.

The following changes since commit 240ab11fb72049d6373cbbec8d788f8e411a00bc:

  Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190924' into staging (2019-09-24 15:36:31 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 3fc4a64cbaed2ddee4c60ddc06740b320e18ab82:

  vhost: Fix memory region section comparison (2019-09-25 10:16:39 -0400)

----------------------------------------------------------------
vhost: fixes

Misc fixes related to memory region handling.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

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

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

[PULL 1/3] memory: Align MemoryRegionSections fields

[Michael S. Tsirkin]

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>
Message-Id: <20190814175535.2023-2-dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@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 a30245c25a..a1e6d846cc 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -495,10 +495,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;
-- 
MST

[PULL 2/3] memory: Provide an equality function for MemoryRegionSections

[Michael S. Tsirkin]

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>
Message-Id: <20190814175535.2023-3-dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@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 a1e6d846cc..6e67043ee0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -504,6 +504,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
  *
-- 
MST

[PULL 3/3] vhost: Fix memory region section comparison

[Michael S. Tsirkin]

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 ("vhost: Regenerate region list from changed sections list")
Cc: qemu-stable@nongnu.org
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20190814175535.2023-4-dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@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 34accdf615..2386b511f3 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);
-- 
MST

Re: [PULL 0/3] vhost: fixes

[Peter Maydell]

On Wed, 25 Sep 2019 at 15:30, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> A very quiet cycle. Did I miss a bunch of patches? if so, let me know.
>
> The following changes since commit 240ab11fb72049d6373cbbec8d788f8e411a00bc:
>
>   Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190924' into staging (2019-09-24 15:36:31 +0100)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 3fc4a64cbaed2ddee4c60ddc06740b320e18ab82:
>
>   vhost: Fix memory region section comparison (2019-09-25 10:16:39 -0400)
>
> ----------------------------------------------------------------
> vhost: fixes
>
> Misc fixes related to memory region handling.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM