| Message ID | 20191214155614.19004-1-philmd@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Simplify memory_region_add_subregion_overlap(..., priority=0) | expand |
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
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).
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
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.
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*.
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
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
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
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(-)