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

[Qemu-devel] [PATCH 0/2] 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


Dr. David Alan Gilbert (2):
  memory: Align and add helper for comparing 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 1/2] memory: Align and add helper for comparing MemoryRegionSections

[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 unnecsssary alignment
and provide an 'eq' routine to safely compare MRS's.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/exec/memory.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 606250172a..ce62e847bd 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -487,15 +487,27 @@ 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;
 };
 
+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

Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections

[Philippe Mathieu-Daudé]

Hi David,

On 8/13/19 12:29 PM, Dr. David Alan Gilbert (git) wrote:
> 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 unnecsssary alignment

"unnecessary"

This is enough change to be in its own commit.

---

> and provide an 'eq' routine to safely compare MRS's.

This is another change, and should be squashed in the next patch IMO.
Doesn't Clang warn about unused 'static inline' btw?

> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/exec/memory.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 606250172a..ce62e847bd 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -487,15 +487,27 @@ 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;
>  };
>  
> +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
>   *
> 

Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Hi David,
> 
> On 8/13/19 12:29 PM, Dr. David Alan Gilbert (git) wrote:
> > 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 unnecsssary alignment
> 
> "unnecessary"

Oops thanks.

> This is enough change to be in its own commit.
> 
> ---
> 
> > and provide an 'eq' routine to safely compare MRS's.
> 
> This is another change, and should be squashed in the next patch IMO.

OK, what I'll do is I'll split this one into two ; I feel better
having the extra function here separate from the next commit.

> Doesn't Clang warn about unused 'static inline' btw?

I was using gcc; but we seem to have loads of static inline's - what
would make this one different?

Dave

> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/exec/memory.h | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 606250172a..ce62e847bd 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -487,15 +487,27 @@ 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;
> >  };
> >  
> > +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
> >   *
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections

[Philippe Mathieu-Daudé]

On 8/14/19 7:25 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> Hi David,
>>
>> On 8/13/19 12:29 PM, Dr. David Alan Gilbert (git) wrote:
>>> 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 unnecsssary alignment
>>
>> "unnecessary"
> 
> Oops thanks.
> 
>> This is enough change to be in its own commit.
>>
>> ---
>>
>>> and provide an 'eq' routine to safely compare MRS's.
>>
>> This is another change, and should be squashed in the next patch IMO.
> 
> OK, what I'll do is I'll split this one into two ; I feel better
> having the extra function here separate from the next commit.

Thanks, feel free to add to both:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>> Doesn't Clang warn about unused 'static inline' btw?
> 
> I was using gcc; but we seem to have loads of static inline's - what
> would make this one different?

I guess I was confused with 'static (no-inline)' :)

>>>
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>>  include/exec/memory.h | 14 +++++++++++++-
>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index 606250172a..ce62e847bd 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -487,15 +487,27 @@ 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;
>>>  };
>>>  
>>> +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
>>>   *
>>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 8/14/19 7:25 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> >> Hi David,
> >>
> >> On 8/13/19 12:29 PM, Dr. David Alan Gilbert (git) wrote:
> >>> 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 unnecsssary alignment
> >>
> >> "unnecessary"
> > 
> > Oops thanks.
> > 
> >> This is enough change to be in its own commit.
> >>
> >> ---
> >>
> >>> and provide an 'eq' routine to safely compare MRS's.
> >>
> >> This is another change, and should be squashed in the next patch IMO.
> > 
> > OK, what I'll do is I'll split this one into two ; I feel better
> > having the extra function here separate from the next commit.
> 
> Thanks, feel free to add to both:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

> >> Doesn't Clang warn about unused 'static inline' btw?
> > 
> > I was using gcc; but we seem to have loads of static inline's - what
> > would make this one different?
> 
> I guess I was confused with 'static (no-inline)' :)
> 
> >>>
> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>> ---
> >>>  include/exec/memory.h | 14 +++++++++++++-
> >>>  1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h
> >>> index 606250172a..ce62e847bd 100644
> >>> --- a/include/exec/memory.h
> >>> +++ b/include/exec/memory.h
> >>> @@ -487,15 +487,27 @@ 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;
> >>>  };
> >>>  
> >>> +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
> >>>   *
> >>>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/2] 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 false 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 0/2] Fix MemoryRegionSection alignment and comparison

[Paolo Bonzini]

On 13/08/19 12:29, Dr. David Alan Gilbert (git) wrote:
> 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
> 
> 
> Dr. David Alan Gilbert (2):
>   memory: Align and add helper for comparing MemoryRegionSections
>   vhost: Fix memory region section comparison
> 
>  hw/virtio/vhost.c     |  9 +++++++--
>  include/exec/memory.h | 14 +++++++++++++-
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I think it can be merged through the vhost tree.

Paolo