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

[Julien Grall <Julien.Grall@arm.com>]

Hi,

On 17/10/2019 20:48, Brian Woods wrote:
> 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

Why do you need a multiline? A single 80-charaters should really be 
sufficient.

> 
> 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).
Please no. There are no need to add a bool just for the sake of getting 
all the print together.

The more that if you print all the information as I suggested above, you 
don't need to have it printed by early_print_info().

To be honest, I really don't think this is Xen job to check that you 
specify your modules correctly. There are other way to screw up your 
device-tree anyway (like overlap in memory banks or reserved region...).

The modules overlap can really only happen if you try to have your DT 
pre-generated and don't bother to use the bootloader (U-boot/Grub) 
script to generate your DT/modules.

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

No. I meant that you could have a device-tree describing two modules 
starting at the same address, but with a different size.

See the check in add_boot_module() to see if a module already exist of 
the same kind.

Cheers,

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