Patchwork [RFC,v3,1/2] memory: allow MemoryRegion's priority field to accept negative values

login
register
mail settings
Submitter Marcel Apfelbaum
Date Sept. 9, 2013, 11:21 a.m.
Message ID <1378725696-13590-2-git-send-email-marcel.a@redhat.com>
Download mbox | patch
Permalink /patch/273556/
State New
Headers show

Comments

Marcel Apfelbaum - Sept. 9, 2013, 11:21 a.m.
Priority is used to make visible some subregions by obscuring
the parent MemoryRegion addresses overlapping with the subregion.

By allowing the priority to be negative the opposite can be done:
Allow a subregion to be visible on all the addresses not covered
by the parent MemoryRegion or other subregions.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/exec/memory.h | 6 +++---
 memory.c              | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
Michael S. Tsirkin - Sept. 9, 2013, 11:41 a.m.
On Mon, Sep 09, 2013 at 02:21:35PM +0300, Marcel Apfelbaum wrote:
> Priority is used to make visible some subregions by obscuring
> the parent MemoryRegion addresses overlapping with the subregion.
> 
> By allowing the priority to be negative the opposite can be done:
> Allow a subregion to be visible on all the addresses not covered
> by the parent MemoryRegion or other subregions.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Seems harmless enough.

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


> ---
>  include/exec/memory.h | 6 +++---
>  memory.c              | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ebe0d24..6995087 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -153,7 +153,7 @@ struct MemoryRegion {
>      bool flush_coalesced_mmio;
>      MemoryRegion *alias;
>      hwaddr alias_offset;
> -    unsigned priority;
> +    int priority;
>      bool may_overlap;
>      QTAILQ_HEAD(subregions, MemoryRegion) subregions;
>      QTAILQ_ENTRY(MemoryRegion) subregions_link;
> @@ -193,7 +193,7 @@ struct MemoryListener {
>      void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection *section,
>                                 hwaddr addr, hwaddr len);
>      /* Lower = earlier (during add), later (during del) */
> -    unsigned priority;
> +    int priority;
>      AddressSpace *address_space_filter;
>      QTAILQ_ENTRY(MemoryListener) link;
>  };
> @@ -779,7 +779,7 @@ void memory_region_add_subregion(MemoryRegion *mr,
>  void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           hwaddr offset,
>                                           MemoryRegion *subregion,
> -                                         unsigned priority);
> +                                         int priority);
>  
>  /**
>   * memory_region_get_ram_addr: Get the ram address associated with a memory
> diff --git a/memory.c b/memory.c
> index 5a10fd0..984a3dc 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1473,7 +1473,7 @@ void memory_region_add_subregion(MemoryRegion *mr,
>  void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           hwaddr offset,
>                                           MemoryRegion *subregion,
> -                                         unsigned priority)
> +                                         int priority)
>  {
>      subregion->may_overlap = true;
>      subregion->priority = priority;
> -- 
> 1.8.3.1
Peter Maydell - Sept. 9, 2013, 11:45 a.m.
On 9 September 2013 12:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Sep 09, 2013 at 02:21:35PM +0300, Marcel Apfelbaum wrote:
>> Priority is used to make visible some subregions by obscuring
>> the parent MemoryRegion addresses overlapping with the subregion.
>>
>> By allowing the priority to be negative the opposite can be done:
>> Allow a subregion to be visible on all the addresses not covered
>> by the parent MemoryRegion or other subregions.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>
> Seems harmless enough.
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

No, the idea is good but this version is just broken.
See the comments I made on the previous version which
Marcel ignored :-(

-- PMM
Michael S. Tsirkin - Sept. 9, 2013, 11:48 a.m.
On Mon, Sep 09, 2013 at 12:45:12PM +0100, Peter Maydell wrote:
> On 9 September 2013 12:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Sep 09, 2013 at 02:21:35PM +0300, Marcel Apfelbaum wrote:
> >> Priority is used to make visible some subregions by obscuring
> >> the parent MemoryRegion addresses overlapping with the subregion.
> >>
> >> By allowing the priority to be negative the opposite can be done:
> >> Allow a subregion to be visible on all the addresses not covered
> >> by the parent MemoryRegion or other subregions.
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >
> > Seems harmless enough.
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> No, the idea is good but this version is just broken.
> See the comments I made on the previous version which
> Marcel ignored :-(
> 
> -- PMM

You are right, I missed the bugs.
Good catch, thanks.
Michael S. Tsirkin - Sept. 9, 2013, 11:49 a.m.
On Mon, Sep 09, 2013 at 02:48:55PM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 09, 2013 at 12:45:12PM +0100, Peter Maydell wrote:
> > On 9 September 2013 12:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Sep 09, 2013 at 02:21:35PM +0300, Marcel Apfelbaum wrote:
> > >> Priority is used to make visible some subregions by obscuring
> > >> the parent MemoryRegion addresses overlapping with the subregion.
> > >>
> > >> By allowing the priority to be negative the opposite can be done:
> > >> Allow a subregion to be visible on all the addresses not covered
> > >> by the parent MemoryRegion or other subregions.
> > >>
> > >> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > >
> > > Seems harmless enough.
> > >
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > No, the idea is good but this version is just broken.
> > See the comments I made on the previous version which
> > Marcel ignored :-(
> > 
> > -- PMM
> 
> You are right, I missed the bugs.
> Good catch, thanks.
> 

So I guess it's only an Acked-by: Michael S. Tsirkin <mst@redhat.com>
at this point :)

Patch

diff --git a/include/exec/memory.h b/include/exec/memory.h
index ebe0d24..6995087 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -153,7 +153,7 @@  struct MemoryRegion {
     bool flush_coalesced_mmio;
     MemoryRegion *alias;
     hwaddr alias_offset;
-    unsigned priority;
+    int priority;
     bool may_overlap;
     QTAILQ_HEAD(subregions, MemoryRegion) subregions;
     QTAILQ_ENTRY(MemoryRegion) subregions_link;
@@ -193,7 +193,7 @@  struct MemoryListener {
     void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection *section,
                                hwaddr addr, hwaddr len);
     /* Lower = earlier (during add), later (during del) */
-    unsigned priority;
+    int priority;
     AddressSpace *address_space_filter;
     QTAILQ_ENTRY(MemoryListener) link;
 };
@@ -779,7 +779,7 @@  void memory_region_add_subregion(MemoryRegion *mr,
 void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          hwaddr offset,
                                          MemoryRegion *subregion,
-                                         unsigned priority);
+                                         int priority);
 
 /**
  * memory_region_get_ram_addr: Get the ram address associated with a memory
diff --git a/memory.c b/memory.c
index 5a10fd0..984a3dc 100644
--- a/memory.c
+++ b/memory.c
@@ -1473,7 +1473,7 @@  void memory_region_add_subregion(MemoryRegion *mr,
 void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          hwaddr offset,
                                          MemoryRegion *subregion,
-                                         unsigned priority)
+                                         int priority)
 {
     subregion->may_overlap = true;
     subregion->priority = priority;