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

[Brian Woods <brian.woods@xilinx.com>]

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