Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap

[Brian Woods <brian.woods@xxxxxxxxxx>]

On Thu, Oct 17, 2019 at 10:20:11AM +0100, Julien Grall wrote:
> Hi,
> 
> Sorry for the late answer.
> 
> On 11/10/2019 20:07, Brian Woods wrote:
> >Which is why I wanted to put it where it was in the patch.  Where the
> >user would see the warning after the information about the memory
> >modules were printed (and fair early).
> 
> I had a think about it, dumping the modules informations before is useful if
> you know that you have one module max per kind. So you avoid to print the
> modules address/size in the warning.
> 
> However, it is possible to have multiple kernel module (as long as they
> don't have the same start address), you could end up with the following
> message:
> 
> "WARNING: modules Kernel and Kernel overlap"
> 
> To make the message more meaningful, we would need to print the modules
> address/size. Therefore, I don't view that it is important to check
> overlapping in early_print_info(). In this case I would favor any code that
> don't add a double for loop.

Well, adding that information would be easy enough and cheap.  It would
make it multiline prinktk though:
WARNING: memory modules over lap:
        start_addr-end_addr: modulename
        start_addr-end_addr: modulename

If we're not doing that though, would it make sense to have a initdata
bool that checks it in add_boot_module() and then prints a simple
warning that there's a memory module overlap in early_print_info()?
That way there's no nested for loop and it gets printed where all the
addresses get printed (so you can actually figure out where the overlap
is).

> While thinking about this case, it made me realize that we only check the
> start address to consider a match. This means if the size is different, then
> it will be ignored. I think we ought to throw at least warning for this case
> as well.
> 
> Would you mind to have a look?

When you say starting address, do you mean like in the orginal patch?
If so, there's no functional change in checking the starts of n on m and
m on n then checking the start and end of n on m.

> >
> >Either way, take your pick of location and if it's only debug or not and
> >I can write it up and test it.
> 
> I would still prefer in add_boot_module(). See why above.

I wrote I suggested above and tested it so that'll be sent out soon.

Brian

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel