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

[Brian Woods <brian.woods@xxxxxxxxxx>]

On Thu, Oct 10, 2019 at 04:39:07PM +0100, Julien Grall wrote:
> Hi Brian,
> 
> Thank you for the patch.
> 
> On 10/9/19 8:47 PM, Brian Woods wrote:
> >It's possible for a misconfigured device tree to cause Xen to crash when
> >there are overlapping addresses in the memory modules.  Add a warning
> >when printing the addresses to let the user know there's a possible
> >issue when DEBUG is enabled.
> >
> >Signed-off-by: Brian Woods <brian.woods@xxxxxxxxxx>
> >---
> >sample output:
> >...
> >(XEN) MODULE[0]: 0000000001400000 - 000000000153b8f1 Xen
> >(XEN) MODULE[1]: 00000000076d2000 - 00000000076dc080 Device Tree
> >(XEN) MODULE[2]: 00000000076df000 - 0000000007fff364 Ramdisk
> >(XEN) MODULE[3]: 0000000000080000 - 0000000003180000 Kernel
> >(XEN)  RESVD[0]: 00000000076d2000 - 00000000076dc000
> >(XEN)  RESVD[1]: 00000000076df000 - 0000000007fff364
> >(XEN)
> >(XEN) WARNING: modules Xen          and Kernel       overlap
> >(XEN)
> >(XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0 
> >maxcpus=1 timer_slop=0
> >...
> >
> >  xen/arch/arm/bootfdt.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> >diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> >index 08fb59f..3cb0c43 100644
> >--- a/xen/arch/arm/bootfdt.c
> >+++ b/xen/arch/arm/bootfdt.c
> >@@ -387,6 +387,23 @@ static void __init early_print_info(void)
> >                 mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
> >      }
> >      printk("\n");
> >+
> >+#ifndef NDEBUG
> >+    /*
> >+     * Assuming all combinations are checked, only the starting address
> >+     * has to be checked if it's in another memory module's range.
> >+     */
> >+    for ( i = 0 ; i < mods->nr_mods; i++ )
> >+        for ( j = 0 ; j < mods->nr_mods; j++ )
> >+            if ( (i != j) &&
> >+                 (mods->module[i].start >= mods->module[j].start) &&
> >+                 (mods->module[i].start <
> >+                  mods->module[j].start + mods->module[j].size) )
> >+                printk("WARNING: modules %-12s and %-12s overlap\n",
> >+                       boot_module_kind_as_string(mods->module[i].kind),
> >+                       boot_module_kind_as_string(mods->module[j].kind));
> 
> I am not entirely happy with the double for-loop here.
> 
> As we already go through all the modules in add_boot_module(). Could you
> look whether this check could be part of it?
> 
> This should also allow to have this check for non-debug build as well.
> 
> Cheers,
> 
> -- 
> Julien Grall

To make sure the module is going to get added, you'd need to do the
check after the for loop.  This means there's going to be multiple for
loops just spread over the course of adding the boot modules rather than
one place.

I had this before but decided against it but after changing it to both
starts rather than the stand and end (ends look much uglier), it looks
cleaner.

    for ( i = 0 ; i < mods->nr_mods-1; i++ )
        for ( j = i+1 ; j < mods->nr_mods; j++ )
            if ( ((mods->module[i].start >= mods->module[j].start) &&
                  (mods->module[i].start <=
                   mods->module[j].start + mods->module[j].size)) ||
                 ((mods->module[j].start >= mods->module[i].start) &&
                  (mods->module[j].start <=
                   mods->module[i].start + mods->module[i].size)) )
                printk("WARNING: modules %-12s and %-12s overlap\n",
                       boot_module_kind_as_string(mods->module[i].kind),
                       boot_module_kind_as_string(mods->module[j].kind));

That's also a possibility.

I just don't see a way around it, computationally.  You can split where
the loops are executed but in the end the same amount of checks/total
iterations have to be run.

I was talking to someone and he suggested you could just check Xen at
early boot and then check other modules later.

-- 
Brian Woods

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