Re: [Qemu-devel] [PATCH RFC 1/3] memory: allow MemoryRegion's priority field to accept negative values

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2013-09-02 at 15:38 +0100, Peter Maydell wrote:
> On 2 September 2013 15:13, 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.
> 
> This comment is not exactly accurate. Allowing priority to
> be signed is just a convenience: you can achieve exactly
> the same effect by specifying some positive priority for
> everything you map into the region and having the background
> region be priority zero. (If you care at all about priorities
> then everything being mapped into the region should be
> happening under the control of your code anyway.)
> 
> 
> > 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;
> 
> This is unrelated to MemoryRegion priorities -- it controls the
> order in which listener callbacks are called. Don't try to change
> both at once (and you only need the MR priorities anyway.)
> 
> >      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 886f838..dfb3ae6 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;
> 
> This isn't a complete set of changes. For instance
> memory_region_set_address() has a local variable 'priority'
> which should be signed now.
> 
> sysbus_mmio_map_common() and sysbus_mmio_map_overlap()
> have priority arguments which need to change to match.
Missed that :(
Making necessary changes and send a new version
Thanks,
Marcel

> 
> thanks
> -- PMM
>