QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison
@ 2019-08-13 10:29 Dr. David Alan Gilbert (git)
  2019-08-13 10:29 ` [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections Dr. David Alan Gilbert (git)
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-13 10:29 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst; +Cc: peter.maydell

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



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

* [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections
  2019-08-13 10:29 [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
@ 2019-08-13 10:29 ` Dr. David Alan Gilbert (git)
  2019-08-13 12:58   ` Philippe Mathieu-Daudé
  2019-08-13 10:29 ` [Qemu-devel] [PATCH 2/2] vhost: Fix memory region section comparison Dr. David Alan Gilbert (git)
  2019-08-13 12:06 ` [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Paolo Bonzini
  2 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-13 10:29 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst; +Cc: peter.maydell

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



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

* [Qemu-devel] [PATCH 2/2] vhost: Fix memory region section comparison
  2019-08-13 10:29 [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
  2019-08-13 10:29 ` [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections Dr. David Alan Gilbert (git)
@ 2019-08-13 10:29 ` Dr. David Alan Gilbert (git)
  2019-08-13 12:06 ` [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Paolo Bonzini
  2 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-08-13 10:29 UTC (permalink / raw)
  To: qemu-devel, pbonzini, mst; +Cc: peter.maydell

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



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

* Re: [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison
  2019-08-13 10:29 [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
  2019-08-13 10:29 ` [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections Dr. David Alan Gilbert (git)
  2019-08-13 10:29 ` [Qemu-devel] [PATCH 2/2] vhost: Fix memory region section comparison Dr. David Alan Gilbert (git)
@ 2019-08-13 12:06 ` Paolo Bonzini
  2 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2019-08-13 12:06 UTC (permalink / raw)
  To: Dr. David Alan Gilbert (git), qemu-devel, mst; +Cc: peter.maydell

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


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

* Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections
  2019-08-13 10:29 ` [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections Dr. David Alan Gilbert (git)
@ 2019-08-13 12:58   ` Philippe Mathieu-Daudé
  2019-08-14 17:25     ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-13 12:58 UTC (permalink / raw)
  To: Dr. David Alan Gilbert (git), qemu-devel, pbonzini, mst; +Cc: peter.maydell

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
>   *
> 


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

* Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections
  2019-08-13 12:58   ` Philippe Mathieu-Daudé
@ 2019-08-14 17:25     ` Dr. David Alan Gilbert
  2019-08-14 17:35       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-08-14 17:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: peter.maydell, pbonzini, qemu-devel, mst

* 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


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

* Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections
  2019-08-14 17:25     ` Dr. David Alan Gilbert
@ 2019-08-14 17:35       ` Philippe Mathieu-Daudé
  2019-08-14 17:44         ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-08-14 17:35 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: peter.maydell, pbonzini, qemu-devel, mst

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
> 


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

* Re: [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections
  2019-08-14 17:35       ` Philippe Mathieu-Daudé
@ 2019-08-14 17:44         ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-08-14 17:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: peter.maydell, pbonzini, qemu-devel, mst

* 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


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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13 10:29 [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Dr. David Alan Gilbert (git)
2019-08-13 10:29 ` [Qemu-devel] [PATCH 1/2] memory: Align and add helper for comparing MemoryRegionSections Dr. David Alan Gilbert (git)
2019-08-13 12:58   ` Philippe Mathieu-Daudé
2019-08-14 17:25     ` Dr. David Alan Gilbert
2019-08-14 17:35       ` Philippe Mathieu-Daudé
2019-08-14 17:44         ` Dr. David Alan Gilbert
2019-08-13 10:29 ` [Qemu-devel] [PATCH 2/2] vhost: Fix memory region section comparison Dr. David Alan Gilbert (git)
2019-08-13 12:06 ` [Qemu-devel] [PATCH 0/2] Fix MemoryRegionSection alignment and comparison Paolo Bonzini

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