Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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).