* [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap @ 2019-10-09 19:47 Brian Woods 2019-10-10 15:39 ` Julien Grall 2019-10-17 20:07 ` [Xen-devel] [PATCH v2] " Brian Woods 0 siblings, 2 replies; 16+ messages in thread From: Brian Woods @ 2019-10-09 19:47 UTC (permalink / raw) To: xen-devel Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk 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@xilinx.com> --- 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)); +#endif + for ( i = 0 ; i < cmds->nr_mods; i++ ) printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start, cmds->cmdline[i].dt_name, -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-09 19:47 [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap Brian Woods @ 2019-10-10 15:39 ` Julien Grall 2019-10-11 16:43 ` Brian Woods 2019-10-17 20:07 ` [Xen-devel] [PATCH v2] " Brian Woods 1 sibling, 1 reply; 16+ messages in thread From: Julien Grall @ 2019-10-10 15:39 UTC (permalink / raw) To: Brian Woods, xen-devel Cc: Volodymyr Babchuk, Stefano Stabellini, Julien Grall 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@xilinx.com> > --- > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-10 15:39 ` Julien Grall @ 2019-10-11 16:43 ` Brian Woods 2019-10-11 16:58 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-11 16:43 UTC (permalink / raw) To: Julien Grall Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel 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@xilinx.com> > >--- > >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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-11 16:43 ` Brian Woods @ 2019-10-11 16:58 ` Julien Grall 2019-10-11 18:06 ` Brian Woods 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2019-10-11 16:58 UTC (permalink / raw) To: Brian Woods Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk Hi, On 10/11/19 5:43 PM, Brian Woods wrote: > 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@xilinx.com> >>> --- >>> 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 Please at least remove the signature in the e-mail you reply to. The best would be to trim the e-mail and answer right below the specific paragraph. > > 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 don't think you need to do the check after the loop. The only way to go out of the loop in add_boot_module() is when i reached mods->nr_mods. > > 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. What's wrong with: diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 705a917abf..ecd09ec698 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -254,6 +254,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind, mod->domU = false; return mod; } + + if ((mod->start >= start) && + (mod->start < (start + size))) + printk("WARNING: modules...\n"); } mod = &mods->module[mods->nr_mods++]; Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-11 16:58 ` Julien Grall @ 2019-10-11 18:06 ` Brian Woods 2019-10-11 18:17 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-11 18:06 UTC (permalink / raw) To: Julien Grall Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel On Fri, Oct 11, 2019 at 05:58:35PM +0100, Julien Grall wrote: > Hi, > > Please at least remove the signature in the e-mail you reply to. The best > would be to trim the e-mail and answer right below the specific paragraph. > > > > >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 don't think you need to do the check after the loop. The only way to go > out of the loop in add_boot_module() is when i reached mods->nr_mods. See below. > > > >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. > > What's wrong with: > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 705a917abf..ecd09ec698 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -254,6 +254,10 @@ struct bootmodule __init > *add_boot_module(bootmodule_kind kind, > mod->domU = false; > return mod; > } > + > + if ((mod->start >= start) && > + (mod->start < (start + size))) > + printk("WARNING: modules...\n"); > } > > mod = &mods->module[mods->nr_mods++]; > > Cheers, > > -- > Julien Grall For that, you'd need to either check the start and end of the added module or the start of both. So something like: if ( ((mod->start >= start) && (mod->start < (start + size))) || ((start >= mod->start) && (start < (mod->start + mod->size))) ) printk("WARNING: ..."); If you don't you run the risk of having a module overlap but not at the start of the added module (unless there's a guaranteed order). You're still running all of the checks from what I can tell, just not in nested for loop so. Plus I'm not sure how many times add_boot_module gets run and the "mod->kind == kind .." if statement gets run and is true. If the above is true, wouldn't that cause extra checks for the for loop iterations before it was true? Brian _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-11 18:06 ` Brian Woods @ 2019-10-11 18:17 ` Julien Grall 2019-10-11 19:07 ` Brian Woods 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2019-10-11 18:17 UTC (permalink / raw) To: Brian Woods Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk Hi, On 10/11/19 7:06 PM, Brian Woods wrote: > On Fri, Oct 11, 2019 at 05:58:35PM +0100, Julien Grall wrote: > For that, you'd need to either check the start and end of the added > module or the start of both. So something like: > > if ( ((mod->start >= start) && (mod->start < (start + size))) || > ((start >= mod->start) && (start < (mod->start + mod->size))) ) > printk("WARNING: ..."); > > If you don't you run the risk of having a module overlap but not at the > start of the added module (unless there's a guaranteed order). You're > still running all of the checks from what I can tell, just not in nested > for loop so. Plus I'm not sure how many times add_boot_module gets run > and the "mod->kind == kind .." if statement gets run and is true. > If the above is true, wouldn't that cause extra checks for the for loop > iterations before it was true? For non-dom0less case, we are talking about 4 modules max (Xen, Kernel, Initramfs, flask policy). Modules cannot be the shared here. For dom0less, you are unlikely to have that many domains started from Xen. So the number of modules will still be limited (even more if you share it). This code is also only called at boot where there are bigger time consuming part (such as domheap initialization). So I would be surprised if you see any improvement (other than a couple of cycles) in boot time here. Therefore, I would favor a readable solution over a micro-optimized solution here. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-11 18:17 ` Julien Grall @ 2019-10-11 19:07 ` Brian Woods 2019-10-17 9:20 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-11 19:07 UTC (permalink / raw) To: Julien Grall Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel On Fri, Oct 11, 2019 at 07:17:29PM +0100, Julien Grall wrote: > Hi, > > On 10/11/19 7:06 PM, Brian Woods wrote: > >On Fri, Oct 11, 2019 at 05:58:35PM +0100, Julien Grall wrote: > >For that, you'd need to either check the start and end of the added > >module or the start of both. So something like: > > > >if ( ((mod->start >= start) && (mod->start < (start + size))) || > > ((start >= mod->start) && (start < (mod->start + mod->size))) ) > > printk("WARNING: ..."); > > > >If you don't you run the risk of having a module overlap but not at the > >start of the added module (unless there's a guaranteed order). You're > >still running all of the checks from what I can tell, just not in nested > >for loop so. Plus I'm not sure how many times add_boot_module gets run > >and the "mod->kind == kind .." if statement gets run and is true. > >If the above is true, wouldn't that cause extra checks for the for loop > >iterations before it was true? > > For non-dom0less case, we are talking about 4 modules max (Xen, Kernel, > Initramfs, flask policy). Modules cannot be the shared here. > > For dom0less, you are unlikely to have that many domains started from Xen. > So the number of modules will still be limited (even more if you share it). Not arguing that. With the second loop (checking two start addresses) it's only n(n-1)/2 iterations. Even if you had 12 memory modules, it's only 66 iterations. In the large scheme of things, that isn't THAT many. > This code is also only called at boot where there are bigger time consuming > part (such as domheap initialization). So I would be surprised if you see > any improvement (other than a couple of cycles) in boot time here. > > Therefore, I would favor a readable solution over a micro-optimized solution > here. 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). Either way, take your pick of location and if it's only debug or not and I can write it up and test it. Brian _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-11 19:07 ` Brian Woods @ 2019-10-17 9:20 ` Julien Grall 2019-10-17 19:48 ` Brian Woods 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2019-10-17 9:20 UTC (permalink / raw) To: Brian Woods Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk Hi, Sorry for the late answer. On 11/10/2019 20:07, Brian Woods wrote: > On Fri, Oct 11, 2019 at 07:17:29PM +0100, Julien Grall wrote: >> This code is also only called at boot where there are bigger time consuming >> part (such as domheap initialization). So I would be surprised if you see >> any improvement (other than a couple of cycles) in boot time here. >> >> Therefore, I would favor a readable solution over a micro-optimized solution >> here. > > 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. 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? > > 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. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-17 9:20 ` Julien Grall @ 2019-10-17 19:48 ` Brian Woods 2019-10-17 20:23 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-17 19:48 UTC (permalink / raw) To: Julien Grall Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap 2019-10-17 19:48 ` Brian Woods @ 2019-10-17 20:23 ` Julien Grall 0 siblings, 0 replies; 16+ messages in thread From: Julien Grall @ 2019-10-17 20:23 UTC (permalink / raw) To: Brian Woods Cc: Volodymyr Babchuk, xen-devel, nd, Julien Grall, Stefano Stabellini 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap 2019-10-09 19:47 [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap Brian Woods 2019-10-10 15:39 ` Julien Grall @ 2019-10-17 20:07 ` Brian Woods 2019-10-17 20:34 ` Julien Grall 1 sibling, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-17 20:07 UTC (permalink / raw) To: xen-devel Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk 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. Signed-off-by: Brian Woods <brian.woods@xilinx.com> --- v1 -> v2 - removed nested loop and placed check in add_boot_module() Sample output: ... (XEN) MODULE[0]: 0000000001400000 - 0000000001542121 Xen (XEN) MODULE[1]: 0000000003846000 - 0000000003850080 Device Tree (XEN) MODULE[2]: 0000000003853000 - 0000000007fff676 Ramdisk (XEN) MODULE[3]: 0000000000080000 - 0000000003180000 Kernel (XEN) RESVD[0]: 0000000003846000 - 0000000003850000 (XEN) RESVD[1]: 0000000003853000 - 0000000007fff676 (XEN) (XEN) WARNING: overlap detected in the memory module addresses (XEN) (XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0 maxcpus=1 timer_slop=0 ... xen/arch/arm/bootfdt.c | 4 ++++ xen/arch/arm/setup.c | 6 ++++++ xen/include/asm-arm/setup.h | 1 + 3 files changed, 11 insertions(+) diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 08fb59f..f8b34d4 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -387,6 +387,10 @@ static void __init early_print_info(void) mem_resv->bank[j].start + mem_resv->bank[j].size - 1); } printk("\n"); + + if ( mem_module_overlap ) + printk("WARNING: overlap detected in the memory module addresses.\n"); + for ( i = 0 ; i < cmds->nr_mods; i++ ) printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start, cmds->cmdline[i].dt_name, diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 705a917..315a131 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -69,6 +69,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); domid_t __read_mostly max_init_domid; +bool __initdata mem_module_overlap; + static __used void init_done(void) { /* Must be done past setting system_state. */ @@ -254,6 +256,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind, mod->domU = false; return mod; } + + if ( ((mod->start >= start) && (mod->start < start + size)) || + ((start >= mod->start) && (start < mod->start + mod->size)) ) + mem_module_overlap = true; } mod = &mods->module[mods->nr_mods++]; diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index 2f8f24e..4bb1ba1 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -122,6 +122,7 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells, u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name, u32 dflt); +extern bool mem_module_overlap; #endif /* * Local variables: -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap 2019-10-17 20:07 ` [Xen-devel] [PATCH v2] " Brian Woods @ 2019-10-17 20:34 ` Julien Grall 2019-10-17 21:20 ` Brian Woods 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2019-10-17 20:34 UTC (permalink / raw) To: Brian Woods Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk Hi, On Thu, 17 Oct 2019 at 21:08, Brian Woods <brian.woods@xilinx.com> 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. > > Signed-off-by: Brian Woods <brian.woods@xilinx.com> > --- > v1 -> v2 > - removed nested loop and placed check in add_boot_module() > > Sample output: > ... > (XEN) MODULE[0]: 0000000001400000 - 0000000001542121 Xen > (XEN) MODULE[1]: 0000000003846000 - 0000000003850080 Device Tree > (XEN) MODULE[2]: 0000000003853000 - 0000000007fff676 Ramdisk > (XEN) MODULE[3]: 0000000000080000 - 0000000003180000 Kernel > (XEN) RESVD[0]: 0000000003846000 - 0000000003850000 > (XEN) RESVD[1]: 0000000003853000 - 0000000007fff676 > (XEN) > (XEN) WARNING: overlap detected in the memory module addresses > (XEN) > (XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0 maxcpus=1 timer_slop=0 > ... > > xen/arch/arm/bootfdt.c | 4 ++++ > xen/arch/arm/setup.c | 6 ++++++ > xen/include/asm-arm/setup.h | 1 + > 3 files changed, 11 insertions(+) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 08fb59f..f8b34d4 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -387,6 +387,10 @@ static void __init early_print_info(void) > mem_resv->bank[j].start + mem_resv->bank[j].size - 1); > } > printk("\n"); > + > + if ( mem_module_overlap ) > + printk("WARNING: overlap detected in the memory module addresses.\n"); As a user such message would likely put me off. You tell me there are an overlap, but you don't provide more information even if you likely have the information in place. However... > + > for ( i = 0 ; i < cmds->nr_mods; i++ ) > printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start, > cmds->cmdline[i].dt_name, > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 705a917..315a131 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -69,6 +69,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > domid_t __read_mostly max_init_domid; > > +bool __initdata mem_module_overlap; > + > static __used void init_done(void) > { > /* Must be done past setting system_state. */ > @@ -254,6 +256,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind, > mod->domU = false; > return mod; > } > + > + if ( ((mod->start >= start) && (mod->start < start + size)) || > + ((start >= mod->start) && (start < mod->start + mod->size)) ) > + mem_module_overlap = true; ... What's wrong with just dumping the information here directly? Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap 2019-10-17 20:34 ` Julien Grall @ 2019-10-17 21:20 ` Brian Woods 2019-10-17 21:49 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-17 21:20 UTC (permalink / raw) To: Julien Grall Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote: > Hi, > > On Thu, 17 Oct 2019 at 21:08, Brian Woods <brian.woods@xilinx.com> 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. > > > > Signed-off-by: Brian Woods <brian.woods@xilinx.com> > > --- > > v1 -> v2 > > - removed nested loop and placed check in add_boot_module() > > > > Sample output: > > ... > > (XEN) MODULE[0]: 0000000001400000 - 0000000001542121 Xen > > (XEN) MODULE[1]: 0000000003846000 - 0000000003850080 Device Tree > > (XEN) MODULE[2]: 0000000003853000 - 0000000007fff676 Ramdisk > > (XEN) MODULE[3]: 0000000000080000 - 0000000003180000 Kernel > > (XEN) RESVD[0]: 0000000003846000 - 0000000003850000 > > (XEN) RESVD[1]: 0000000003853000 - 0000000007fff676 > > (XEN) > > (XEN) WARNING: overlap detected in the memory module addresses > > (XEN) > > (XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0 maxcpus=1 timer_slop=0 > > ... > > > > xen/arch/arm/bootfdt.c | 4 ++++ > > xen/arch/arm/setup.c | 6 ++++++ > > xen/include/asm-arm/setup.h | 1 + > > 3 files changed, 11 insertions(+) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index 08fb59f..f8b34d4 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -387,6 +387,10 @@ static void __init early_print_info(void) > > mem_resv->bank[j].start + mem_resv->bank[j].size - 1); > > } > > printk("\n"); > > + > > + if ( mem_module_overlap ) > > + printk("WARNING: overlap detected in the memory module addresses.\n"); > > As a user such message would likely put me off. You tell me there are > an overlap, but you don't provide more information even if you likely > have the information in place. However... Well, I suppose the message could be changed to something like: "WARNING: overlap detected in the above memory module addresses." or something to more directly guide the users to the section. Maybe move the 'printk("\n");' after the warning so it's grouped tighter with the module information. > > + > > for ( i = 0 ; i < cmds->nr_mods; i++ ) > > printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start, > > cmds->cmdline[i].dt_name, > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > index 705a917..315a131 100644 > > --- a/xen/arch/arm/setup.c > > +++ b/xen/arch/arm/setup.c > > @@ -69,6 +69,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > > > domid_t __read_mostly max_init_domid; > > > > +bool __initdata mem_module_overlap; > > + > > static __used void init_done(void) > > { > > /* Must be done past setting system_state. */ > > @@ -254,6 +256,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind, > > mod->domU = false; > > return mod; > > } > > + > > + if ( ((mod->start >= start) && (mod->start < start + size)) || > > + ((start >= mod->start) && (start < mod->start + mod->size)) ) > > + mem_module_overlap = true; > > ... What's wrong with just dumping the information here directly? IMO, it is better to have all the information printed in one spot. There is less to go through and easier to find out what is happening. There is also the fact that we do not have to print things twice (2 sets of names, starting addresses and ending addresses per overlap) when it is going to be printed in the near future anyway. The cost of this is just one initdata bool, which while I am not thrilled about, does not seem that expensive (compared to a nested loop or printing out at least (16*2 + 12) * 2 characters per overlap(at least on Arm64)). I do think the message could use some polish, but this approach makes the most sense to me. Brian _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap 2019-10-17 21:20 ` Brian Woods @ 2019-10-17 21:49 ` Julien Grall 2019-10-17 22:34 ` Brian Woods 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2019-10-17 21:49 UTC (permalink / raw) To: Brian Woods Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk On Thu, 17 Oct 2019 at 22:20, Brian Woods <brian.woods@xilinx.com> wrote: > > On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote: > > Hi, > > > > On Thu, 17 Oct 2019 at 21:08, Brian Woods <brian.woods@xilinx.com> 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. > > > > > > Signed-off-by: Brian Woods <brian.woods@xilinx.com> > > > --- > > > v1 -> v2 > > > - removed nested loop and placed check in add_boot_module() > > > > > > Sample output: > > > ... > > > (XEN) MODULE[0]: 0000000001400000 - 0000000001542121 Xen > > > (XEN) MODULE[1]: 0000000003846000 - 0000000003850080 Device Tree > > > (XEN) MODULE[2]: 0000000003853000 - 0000000007fff676 Ramdisk > > > (XEN) MODULE[3]: 0000000000080000 - 0000000003180000 Kernel > > > (XEN) RESVD[0]: 0000000003846000 - 0000000003850000 > > > (XEN) RESVD[1]: 0000000003853000 - 0000000007fff676 > > > (XEN) > > > (XEN) WARNING: overlap detected in the memory module addresses > > > (XEN) > > > (XEN) Command line: console=dtuart dtuart=serial0 dom0_mem=1G bootscrub=0 maxcpus=1 timer_slop=0 > > > ... > > > > > > xen/arch/arm/bootfdt.c | 4 ++++ > > > xen/arch/arm/setup.c | 6 ++++++ > > > xen/include/asm-arm/setup.h | 1 + > > > 3 files changed, 11 insertions(+) > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > index 08fb59f..f8b34d4 100644 > > > --- a/xen/arch/arm/bootfdt.c > > > +++ b/xen/arch/arm/bootfdt.c > > > @@ -387,6 +387,10 @@ static void __init early_print_info(void) > > > mem_resv->bank[j].start + mem_resv->bank[j].size - 1); > > > } > > > printk("\n"); > > > + > > > + if ( mem_module_overlap ) > > > + printk("WARNING: overlap detected in the memory module addresses.\n"); > > > > As a user such message would likely put me off. You tell me there are > > an overlap, but you don't provide more information even if you likely > > have the information in place. However... > > Well, I suppose the message could be changed to something like: > "WARNING: overlap detected in the above memory module addresses." > or something to more directly guide the users to the section. Maybe > move the 'printk("\n");' after the warning so it's grouped tighter with > the module information. My point stands even for this sort of message. You know the exact overlap, so why would you hide it from the users? > > > > + > > > for ( i = 0 ; i < cmds->nr_mods; i++ ) > > > printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start, > > > cmds->cmdline[i].dt_name, > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > > index 705a917..315a131 100644 > > > --- a/xen/arch/arm/setup.c > > > +++ b/xen/arch/arm/setup.c > > > @@ -69,6 +69,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > > > > > domid_t __read_mostly max_init_domid; > > > > > > +bool __initdata mem_module_overlap; > > > + > > > static __used void init_done(void) > > > { > > > /* Must be done past setting system_state. */ > > > @@ -254,6 +256,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind, > > > mod->domU = false; > > > return mod; > > > } > > > + > > > + if ( ((mod->start >= start) && (mod->start < start + size)) || > > > + ((start >= mod->start) && (start < mod->start + mod->size)) ) > > > + mem_module_overlap = true; > > > > ... What's wrong with just dumping the information here directly? > > IMO, it is better to have all the information printed in one spot. > There is less to go through and easier to find out what is happening. > There is also the fact that we do not have to print things twice (2 sets > of names, starting addresses and ending addresses per overlap) when it > is going to be printed in the near future anyway. The cost of this is > just one initdata bool, which while I am not thrilled about, does not > seem that expensive (compared to a nested loop or printing out at least > (16*2 + 12) * 2 characters per overlap(at least on Arm64)). Again, this is boot code and not a path that is going to be called hundreds of time. So performance is the last thing I care in this patch. If we try to help the users by telling them there is an overlap between modules, then we should do it properly and tell them the exact overlap. Otherwise this is nearly as pointless as a crash later on in the boot process. I also don't want a double for loop or any additional global variable when it can be done by simply adding a check in add_boot_module(). Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap 2019-10-17 21:49 ` Julien Grall @ 2019-10-17 22:34 ` Brian Woods 2019-10-18 15:41 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Brian Woods @ 2019-10-17 22:34 UTC (permalink / raw) To: Julien Grall Cc: Brian Woods, Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel On Thu, Oct 17, 2019 at 10:49:15PM +0100, Julien Grall wrote: > On Thu, 17 Oct 2019 at 22:20, Brian Woods <brian.woods@xilinx.com> wrote: > > > > On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote: > > > Hi, > > > > > > As a user such message would likely put me off. You tell me there are > > > an overlap, but you don't provide more information even if you likely > > > have the information in place. However... > > > > Well, I suppose the message could be changed to something like: > > "WARNING: overlap detected in the above memory module addresses." > > or something to more directly guide the users to the section. Maybe > > move the 'printk("\n");' after the warning so it's grouped tighter with > > the module information. > > My point stands even for this sort of message. You know the exact > overlap, so why would you hide it from the users? We're not hiding it. You're not cluttering up the log with the same data multiple times. See below. > > > > > > ... What's wrong with just dumping the information here directly? > > > > IMO, it is better to have all the information printed in one spot. > > There is less to go through and easier to find out what is happening. > > There is also the fact that we do not have to print things twice (2 sets > > of names, starting addresses and ending addresses per overlap) when it > > is going to be printed in the near future anyway. The cost of this is > > just one initdata bool, which while I am not thrilled about, does not > > seem that expensive (compared to a nested loop or printing out at least > > (16*2 + 12) * 2 characters per overlap(at least on Arm64)). > > Again, this is boot code and not a path that is going to be called > hundreds of time. So performance is the last thing I care in this > patch. > > If we try to help the users by telling them there is an overlap > between modules, then we should do it properly and tell them the exact > overlap. Otherwise this is nearly as pointless as a crash later on in > the boot process. > > I also don't want a double for loop or any additional global variable > when it can be done by simply adding a check in add_boot_module(). This isn't about performance (other than the nested for), this is about providing a relatively clean and sane log to read. It's not that difficult to go through the addresses and see conflicts. This also keeps it all in one part of the log and shorter without losing information. Shorter and well structured logs (without losing info) makes it easier to read. Making logs easier to read helps everyone. Showing the addresses and module name itself will take 2 lines assuming you stay within 80 chars. (16*2 + 12) * 2 = 88, that's without spaces, '0x's or any sort of message explaining what's actually going wrong. The module names and addresses will be printed out anyway in the near future, so why not group them together? The purpose of the warning is to tell the user something is wrong, both messages do that and provide the information to determine what's wrong. Brian _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [PATCH v2] xen/arm: add warning if memory modules overlap 2019-10-17 22:34 ` Brian Woods @ 2019-10-18 15:41 ` Julien Grall 0 siblings, 0 replies; 16+ messages in thread From: Julien Grall @ 2019-10-18 15:41 UTC (permalink / raw) To: Brian Woods, Julien Grall Cc: Volodymyr Babchuk, Stefano Stabellini, Julien Grall, xen-devel Hi Brian, On 17/10/2019 23:34, Brian Woods wrote: > On Thu, Oct 17, 2019 at 10:49:15PM +0100, Julien Grall wrote: >> On Thu, 17 Oct 2019 at 22:20, Brian Woods <brian.woods@xilinx.com> wrote: >>> >>> On Thu, Oct 17, 2019 at 09:34:51PM +0100, Julien Grall wrote: >>>> Hi, >>>> >>>> As a user such message would likely put me off. You tell me there are >>>> an overlap, but you don't provide more information even if you likely >>>> have the information in place. However... >>> >>> Well, I suppose the message could be changed to something like: >>> "WARNING: overlap detected in the above memory module addresses." >>> or something to more directly guide the users to the section. Maybe >>> move the 'printk("\n");' after the warning so it's grouped tighter with >>> the module information. >> >> My point stands even for this sort of message. You know the exact >> overlap, so why would you hide it from the users? > > We're not hiding it. You're not cluttering up the log with the same > data multiple times. See below. While the values are the same, the data is printed in a different way to help the users. > >>>> >>>> ... What's wrong with just dumping the information here directly? >>> >>> IMO, it is better to have all the information printed in one spot. >>> There is less to go through and easier to find out what is happening. >>> There is also the fact that we do not have to print things twice (2 sets >>> of names, starting addresses and ending addresses per overlap) when it >>> is going to be printed in the near future anyway. The cost of this is >>> just one initdata bool, which while I am not thrilled about, does not >>> seem that expensive (compared to a nested loop or printing out at least >>> (16*2 + 12) * 2 characters per overlap(at least on Arm64)). >> >> Again, this is boot code and not a path that is going to be called >> hundreds of time. So performance is the last thing I care in this >> patch. >> >> If we try to help the users by telling them there is an overlap >> between modules, then we should do it properly and tell them the exact >> overlap. Otherwise this is nearly as pointless as a crash later on in >> the boot process. >> >> I also don't want a double for loop or any additional global variable >> when it can be done by simply adding a check in add_boot_module(). > > This isn't about performance (other than the nested for), this is about > providing a relatively clean and sane log to read. It's not that > difficult to go through the addresses and see conflicts. This also > keeps it all in one part of the log and shorter without losing > information. Shorter and well structured logs (without losing info) > makes it easier to read. Making logs easier to read helps everyone. > > Showing the addresses and module name itself will take 2 lines assuming > you stay within 80 chars. (16*2 + 12) * 2 = 88, that's without spaces, > '0x's or any sort of message explaining what's actually going wrong. > The module names and addresses will be printed out anyway in the near > future, so why not group them together? Here again you argue about the performance and smaller message... This is a warning (so not printed in the normal course) and Xen is likely to break afterwards. So what you want here is a big fat warning and not a small one and easy to miss. > > The purpose of the warning is to tell the user something is wrong, both > messages do that and provide the information to determine what's wrong We both can probably figure out with more or less some effort. If you have 2-3 modules that's fine. But if you have 10, then it is becoming more complex. My time is quite valuable and therefore I want the hypervisor to help me find. To make an analogy, would you like if your compiler tells you "There is a bug in file X" but does not tell you the exact line? I personally wouldn't, even if the file is fairly small. So while I am happy to se a way to check the modules, I dislike this approach. So: Nacked-by: Julien Grall <julien.grall@arm.com> Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-10-18 15:41 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-09 19:47 [Xen-devel] [PATCH] xen/arm: add warning if memory modules overlap Brian Woods 2019-10-10 15:39 ` Julien Grall 2019-10-11 16:43 ` Brian Woods 2019-10-11 16:58 ` Julien Grall 2019-10-11 18:06 ` Brian Woods 2019-10-11 18:17 ` Julien Grall 2019-10-11 19:07 ` Brian Woods 2019-10-17 9:20 ` Julien Grall 2019-10-17 19:48 ` Brian Woods 2019-10-17 20:23 ` Julien Grall 2019-10-17 20:07 ` [Xen-devel] [PATCH v2] " Brian Woods 2019-10-17 20:34 ` Julien Grall 2019-10-17 21:20 ` Brian Woods 2019-10-17 21:49 ` Julien Grall 2019-10-17 22:34 ` Brian Woods 2019-10-18 15:41 ` Julien Grall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).