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
