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

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

Comments

Marcel Apfelbaum - Sept. 9, 2013, 11:11 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(-)
Peter Maydell - Sept. 9, 2013, 11:28 a.m.
On 9 September 2013 12:11, Marcel Apfelbaum <marcel.a@redhat.com> 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>
> ---
>  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;

You haven't addressed any of the points I made reviewing
the first version of this. Please don't just ignore
code review, or people will stop reviewing your code.

I think it would also be nice to update docs/memory.txt
to say explicitly that priority is signed and why this
is useful, something like this:

====begin====
Priority values are signed, and the default value is
zero. This means that you can use
memory_region_add_subregion_overlap() both to specify
a region that must sit 'above' any others (with a
positive priority) and also a background region that
sits 'below' others (with a negative priority).
====endit====

(in the 'Overlapping regions and priority' section
of that document).

thanks
-- PMM
Marcel Apfelbaum - Sept. 9, 2013, 12:03 p.m.
On Mon, 2013-09-09 at 12:28 +0100, Peter Maydell wrote:
> On 9 September 2013 12:11, Marcel Apfelbaum <marcel.a@redhat.com> 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>
> > ---
> >  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;
> 
> You haven't addressed any of the points I made reviewing
> the first version of this. Please don't just ignore
> code review, or people will stop reviewing your code.
Hi Peter,
I really value your comments and I did acted upon them.
Basically all the changes of this version are based on your comments, thanks!

I did answer to your comment and I was going to remove it,
but I missed it again :(. Sorry for that.
I will remove it of course in the following version.

> 
> I think it would also be nice to update docs/memory.txt
> to say explicitly that priority is signed and why this
> is useful, something like this:
Of course I will, thanks!

> 
> ====begin====
> Priority values are signed, and the default value is
> zero. This means that you can use
> memory_region_add_subregion_overlap() both to specify
> a region that must sit 'above' any others (with a
> positive priority) and also a background region that
> sits 'below' others (with a negative priority).
> ====endit====
> 
> (in the 'Overlapping regions and priority' section
> of that document).
> 
> thanks
> -- PMM
>

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;