| Message ID | 1378725114-13197-2-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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;
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(-)