From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJ0Bg-0008Os-Fv for qemu-devel@nongnu.org; Mon, 09 Sep 2013 08:03:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJ0BW-0002CR-SR for qemu-devel@nongnu.org; Mon, 09 Sep 2013 08:03:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53759) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJ0BW-0002C4-K6 for qemu-devel@nongnu.org; Mon, 09 Sep 2013 08:03:14 -0400 Message-ID: <1378728191.3072.5.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 09 Sep 2013 15:03:11 +0300 In-Reply-To: References: <1378725114-13197-1-git-send-email-marcel.a@redhat.com> <1378725114-13197-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 v2 1/2] 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 , "Michael S. Tsirkin" , QEMU Developers , Jan Kiszka On Mon, 2013-09-09 at 12:28 +0100, Peter Maydell wrote: > On 9 September 2013 12:11, 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 > > --- > > 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 >