[0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)
mbox series

Message ID 20191214155614.19004-1-philmd@redhat.com
Headers show
Series
  • Simplify memory_region_add_subregion_overlap(..., priority=0)
Related show

Message

Philippe Mathieu-Daudé Dec. 14, 2019, 3:56 p.m. UTC
Hi,

In this series we use coccinelle to replace:
- memory_region_add_subregion_overlap(..., priority=0)
+ memory_region_add_subregion(...)

Rationale is the code is easier to read, and reviewers don't
have to worry about overlapping because it isn't used.

Last patch is a minor cleanup in variable names.

I expect each subsystem maintainer to take the subsystem patches.

Regards,

Phil.

Philippe Mathieu-Daudé (8):
  hw/arm/nrf51_soc: Use memory_region_add_subregion() when priority is 0
  hw/arm/raspi: Use memory_region_add_subregion() when priority is 0
  hw/arm/xlnx-versal: Use memory_region_add_subregion() when priority is
    0
  hw/i386/intel_iommu: Use memory_region_add_subregion when priority is
    0
  hw/mips/boston: Use memory_region_add_subregion() when priority is 0
  hw/vfio/pci: Use memory_region_add_subregion() when priority is 0
  target/i386: Use memory_region_add_subregion() when priority is 0
  target/i386/cpu: Use 'mr' for MemoryRegion variables

 target/i386/cpu.h            |  2 +-
 hw/arm/bcm2835_peripherals.c |  4 ++--
 hw/arm/nrf51_soc.c           | 14 +++++++-------
 hw/arm/raspi.c               |  2 +-
 hw/arm/xlnx-versal-virt.c    |  3 +--
 hw/arm/xlnx-versal.c         |  4 ++--
 hw/i386/intel_iommu.c        | 11 ++++-------
 hw/mips/boston.c             | 14 +++++++-------
 hw/vfio/pci.c                |  3 +--
 target/i386/cpu.c            | 18 +++++++++---------
 target/i386/kvm.c            |  2 +-
 11 files changed, 36 insertions(+), 41 deletions(-)

Comments

Peter Maydell Dec. 14, 2019, 4:28 p.m. UTC | #1
On Sat, 14 Dec 2019 at 15:56, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi,
>
> In this series we use coccinelle to replace:
> - memory_region_add_subregion_overlap(..., priority=0)
> + memory_region_add_subregion(...)
>
> Rationale is the code is easier to read, and reviewers don't
> have to worry about overlapping because it isn't used.

So our implementation of these two functions makes them
have the same behaviour, but the documentation comments
in memory.h describe them as different: a subregion added
with memory_region_add_subregion() is not supposed to
overlap any other subregion unless that other subregion
was explicitly marked as overlapping. My intention with the
API design here was that using the _overlap() version is
a statement of intent -- this region is *expected* to be
overlapping with some other regions, which hopefully
have a priority set so they go at the right order wrt this one.
Use of the non-overlap function says "I don't expect this
to overlap". (It doesn't actually assert that it doesn't
overlap because we have some legacy uses, notably
in the x86 PC machines, which do overlap without using
the right function, which we've never tried to tidy up.)

We used to have some #if-ed out code in memory.c which
was able to detect incorrect overlap, but it got removed
in commit b613597819587. I thought then and still do
that rather than removing code and API distinctions that
allow us to tell if the board code has done something
wrong (unintentional overlap, especially unintentional
overlap at the same priority value) it would be better to
fix the board bugs so we could enable the warnings/asserts...

thanks
-- PMM
Philippe Mathieu-Daudé Dec. 14, 2019, 6:17 p.m. UTC | #2
On 12/14/19 5:28 PM, Peter Maydell wrote:
> On Sat, 14 Dec 2019 at 15:56, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi,
>>
>> In this series we use coccinelle to replace:
>> - memory_region_add_subregion_overlap(..., priority=0)
>> + memory_region_add_subregion(...)
>>
>> Rationale is the code is easier to read, and reviewers don't
>> have to worry about overlapping because it isn't used.
> 
> So our implementation of these two functions makes them
> have the same behaviour, but the documentation comments
> in memory.h describe them as different: a subregion added
> with memory_region_add_subregion() is not supposed to
> overlap any other subregion unless that other subregion
> was explicitly marked as overlapping. My intention with the
> API design here was that using the _overlap() version is
> a statement of intent -- this region is *expected* to be
> overlapping with some other regions, which hopefully
> have a priority set so they go at the right order wrt this one.

I didn't notice the documentation differences, now it is clear.

> Use of the non-overlap function says "I don't expect this
> to overlap". (It doesn't actually assert that it doesn't
> overlap because we have some legacy uses, notably
> in the x86 PC machines, which do overlap without using
> the right function, which we've never tried to tidy up.)
> 
> We used to have some #if-ed out code in memory.c which
> was able to detect incorrect overlap, but it got removed
> in commit b613597819587. I thought then and still do
> that rather than removing code and API distinctions that
> allow us to tell if the board code has done something
> wrong (unintentional overlap, especially unintentional
> overlap at the same priority value) it would be better to
> fix the board bugs so we could enable the warnings/asserts...

Maybe we can a warning if priority=0, to force board designers to use 
explicit priority (explicit overlap).
Peter Maydell Dec. 14, 2019, 8:01 p.m. UTC | #3
On Sat, 14 Dec 2019 at 18:17, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Maybe we can a warning if priority=0, to force board designers to use
> explicit priority (explicit overlap).

Priority 0 is fine, it's just one of the possible positive and
negative values. I think what ideally we would complain about
is where we see an overlap and both the regions involved
have the same priority value, because in that case which
one the guest sees is implicitly dependent on (I think) which
order the subregions were added, which is fragile if we move
code around. I'm not sure how easy that is to test for or how
much of our existing code violates it, though.

thanks
-- PMM
Michael S. Tsirkin Dec. 15, 2019, 9:51 a.m. UTC | #4
On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> (It doesn't actually assert that it doesn't
> overlap because we have some legacy uses, notably
> in the x86 PC machines, which do overlap without using
> the right function, which we've never tried to tidy up.)

It's not exactly legacy uses.

To be more exact, the way the non overlap versions
are *used* is to mean "I don't care what happens when they overlap"
as opposed to "will never overlap".

There are lots of regions where guest can make things overlapping
but doesn't, e.g. PCI BARs can be programmed to overlap
almost anything.

What happens on real hardware if you then access one of
the BARs is undefined, but programming itself is harmless.
That's why we can't assert.
Michael S. Tsirkin Dec. 15, 2019, 9:54 a.m. UTC | #5
On Sat, Dec 14, 2019 at 08:01:46PM +0000, Peter Maydell wrote:
> On Sat, 14 Dec 2019 at 18:17, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > Maybe we can a warning if priority=0, to force board designers to use
> > explicit priority (explicit overlap).
> 
> Priority 0 is fine, it's just one of the possible positive and
> negative values. I think what ideally we would complain about
> is where we see an overlap and both the regions involved
> have the same priority value, because in that case which
> one the guest sees is implicitly dependent on (I think) which
> order the subregions were added, which is fragile if we move
> code around. I'm not sure how easy that is to test for or how
> much of our existing code violates it, though.
> 
> thanks
> -- PMM

Problem is it's not uncommon for guests to create such
configs, and then just never access them.
So the thing to do would be to complain *on access*.
Peter Maydell Dec. 15, 2019, 3:27 p.m. UTC | #6
On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> > (It doesn't actually assert that it doesn't
> > overlap because we have some legacy uses, notably
> > in the x86 PC machines, which do overlap without using
> > the right function, which we've never tried to tidy up.)
>
> It's not exactly legacy uses.
>
> To be more exact, the way the non overlap versions
> are *used* is to mean "I don't care what happens when they overlap"
> as opposed to "will never overlap".

Almost all of the use of the non-overlap versions is
for "these are never going to overlap" -- devices or ram at
fixed addresses in the address space that can't
ever be mapped over by anything else. If you want
"can overlap but I don't care which one wins" then
that would be more clearly expressed by using the _overlap()
version but just giving everything that can overlap there
the same priority.

> There are lots of regions where guest can make things overlapping
> but doesn't, e.g. PCI BARs can be programmed to overlap
> almost anything.
>
> What happens on real hardware if you then access one of
> the BARs is undefined, but programming itself is harmless.
> That's why we can't assert.

Yeah, good point, for the special case where it's the
guest that's determining the addresses where something's
mapped we might want to allow the behaviour to fall out
of the implementation. (You could instead specify set of
priorities that makes the undefined-behaviour something
specific, rather than just an emergent property of
the implementation QEMU happens to have, but it seems
a bit hard to justify.)

thanks
-- PMM
Michael S. Tsirkin Dec. 16, 2019, 11:39 a.m. UTC | #7
On Sun, Dec 15, 2019 at 03:27:12PM +0000, Peter Maydell wrote:
> On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> > > (It doesn't actually assert that it doesn't
> > > overlap because we have some legacy uses, notably
> > > in the x86 PC machines, which do overlap without using
> > > the right function, which we've never tried to tidy up.)
> >
> > It's not exactly legacy uses.
> >
> > To be more exact, the way the non overlap versions
> > are *used* is to mean "I don't care what happens when they overlap"
> > as opposed to "will never overlap".
> 
> Almost all of the use of the non-overlap versions is
> for "these are never going to overlap" -- devices or ram at
> fixed addresses in the address space that can't
> ever be mapped over by anything else. If you want
> "can overlap but I don't care which one wins" then
> that would be more clearly expressed by using the _overlap()
> version but just giving everything that can overlap there
> the same priority.

Problem is device doesn't always know whether something can overlap it.
Imagine device A at a fixed address.
Guest can program device B to overlap the fixed address.
How is device A supposed to know this can happen?



> > There are lots of regions where guest can make things overlapping
> > but doesn't, e.g. PCI BARs can be programmed to overlap
> > almost anything.
> >
> > What happens on real hardware if you then access one of
> > the BARs is undefined, but programming itself is harmless.
> > That's why we can't assert.
> 
> Yeah, good point, for the special case where it's the
> guest that's determining the addresses where something's
> mapped we might want to allow the behaviour to fall out
> of the implementation. (You could instead specify set of
> priorities that makes the undefined-behaviour something
> specific, rather than just an emergent property of
> the implementation QEMU happens to have, but it seems
> a bit hard to justify.)
> 
> thanks
> -- PMM
Peter Maydell Dec. 16, 2019, 11:46 a.m. UTC | #8
On Mon, 16 Dec 2019 at 11:40, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Dec 15, 2019 at 03:27:12PM +0000, Peter Maydell wrote:
> > On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> > > > (It doesn't actually assert that it doesn't
> > > > overlap because we have some legacy uses, notably
> > > > in the x86 PC machines, which do overlap without using
> > > > the right function, which we've never tried to tidy up.)
> > >
> > > It's not exactly legacy uses.
> > >
> > > To be more exact, the way the non overlap versions
> > > are *used* is to mean "I don't care what happens when they overlap"
> > > as opposed to "will never overlap".
> >
> > Almost all of the use of the non-overlap versions is
> > for "these are never going to overlap" -- devices or ram at
> > fixed addresses in the address space that can't
> > ever be mapped over by anything else. If you want
> > "can overlap but I don't care which one wins" then
> > that would be more clearly expressed by using the _overlap()
> > version but just giving everything that can overlap there
> > the same priority.
>
> Problem is device doesn't always know whether something can overlap it.
> Imagine device A at a fixed address.
> Guest can program device B to overlap the fixed address.
> How is device A supposed to know this can happen?

That's why (the original intention was) only one of the
regions needs to be marked 'overlap OK', not both.

thanks
-- PMM