Re: [Xen-devel] [PATCH v5 05/25] xen/arm: check for multiboot nodes only under /chosen

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/23/18 3:02 AM, Stefano Stabellini wrote:
> > Make sure to only look for multiboot compatible nodes only under
> > /chosen, not under any other paths (depth <= 3).
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > ---
> > 
> > Changes in v5:
> > - add patch
> > - add check on return value of fdt_get_path
> > ---
> >   xen/arch/arm/bootfdt.c | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 8eba42c..a314aca 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -173,7 +173,14 @@ static void __init process_multiboot_node(const void
> > *fdt, int node,
> >       bootmodule_kind kind;
> >       paddr_t start, size;
> >       const char *cmdline;
> > -    int len;
> > +    int len = sizeof("/chosen");
> 
> NIT, I am not convince you win anything with that trick. strlen could be
> optimized by the compiler (we use __builtin_strlen on Arm64).

I could use strlen if you prefer, but given that the string is known at
compile time, it should hopefully be resolved to "8" at compile time
using sizeof. What's wrong with it?


> > +    char path[8]; /* sizeof "/chosen" */
> > +    int ret;
> > +
> > +    /* Check that the node is under "chosen" */
> > +    ret = fdt_get_path(fdt, node, path, len);
> > +    if ( ret == 0 && strncmp(path, "/chosen", len - 1) )
> > +        return;
> 
> This is quite odd. You continue if fdt_get_path() returns an error. Shouldn't
> it be:
> 
> ret || strncmp(path, "/chosen", strlen("/chosen")

You are right, I'll change it


> >         prop = fdt_get_property(fdt, node, "reg", &len);
> >       if ( !prop )
> > @@ -286,8 +293,8 @@ static int __init early_scan_node(const void *fdt,
> >   {
> >       if ( device_tree_node_matches(fdt, node, "memory") )
> >           process_memory_node(fdt, node, name, address_cells, size_cells);
> > -    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module"
> > ) ||
> > -              device_tree_node_compatible(fdt, node, "multiboot,module" ))
> > +    else if ( depth <= 3 && (device_tree_node_compatible(fdt, node,
> > "xen,multiboot-module" ) ||
> > +              device_tree_node_compatible(fdt, node, "multiboot,module" )))
> >           process_multiboot_node(fdt, node, name, address_cells,
> > size_cells);
> >       else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen")
> > )
> >           process_chosen_node(fdt, node, name, address_cells, size_cells);

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