From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJ0Or-0006gJ-2f for qemu-devel@nongnu.org; Mon, 09 Sep 2013 08:17:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJ0Ol-0005d5-2t for qemu-devel@nongnu.org; Mon, 09 Sep 2013 08:17:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57142) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJ0Ok-0005ct-Rb for qemu-devel@nongnu.org; Mon, 09 Sep 2013 08:16:55 -0400 Message-ID: <1378729016.3072.16.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 09 Sep 2013 15:16:56 +0300 In-Reply-To: References: <1378131189-25538-1-git-send-email-marcel.a@redhat.com> <1378131189-25538-2-git-send-email-marcel.a@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 1/3] memory: allow MemoryRegion's priority field to accept negative values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Paolo Bonzini , Anthony Liguori , QEMU Developers , Andreas =?ISO-8859-1?Q?F=E4rber?= , "Michael S. Tsirkin" On Mon, 2013-09-02 at 15:38 +0100, Peter Maydell wrote: > On 2 September 2013 15:13, 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. > > 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 > > --- > > 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 >