Re: [Xen-devel] [PATCH v3 2/6] xen/arm: copy dtb fragment to guest dtb

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 8/9/19 12:12 AM, Stefano Stabellini wrote:
> Read the dtb fragment corresponding to a passthrough device from memory
> at the location referred to by the "multiboot,dtb" compatible node.
>
> Copy the fragment to the guest dtb.
>
> Add a dtb_bootmodule field to struct kernel_info to find the dtb
> fragment for a guest.
>
> Some of the code below is taken from tools/libxl/libxl_arm.c. Note that
> it is OK to take LGPL 2.1 code and including it into a GPLv2 code base.
> The result is GPLv2 code.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
>
> ----
> Changes in v3:
> - switch to using device_tree_for_each_node for the copy
>
> Changes in v2:
> - add a note about the code coming from libxl in the commit message
> - copy /aliases
> - code style
> ---
>   xen/arch/arm/domain_build.c  | 103 +++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/kernel.h |   2 +-
>   2 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 00ddb3b05d..70bcdc449d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -14,6 +14,7 @@
>   #include <xen/guest_access.h>
>   #include <xen/iocap.h>
>   #include <xen/acpi.h>
> +#include <xen/vmap.h>
>   #include <xen/warning.h>
>   #include <acpi/actables.h>
>   #include <asm/device.h>
> @@ -1706,6 +1707,102 @@ static int __init make_vpl011_uart_node(const struct 
> domain *d, void *fdt)
>   }
>   #endif
>   
> +static int __init handle_properties(struct domain *d, void *fdt, const void *pfdt, int nodeoff,
> +                                    u32 address_cells, u32 size_cells)
So in this file we already have a function call write_properties. How a 
user will know which one to use?
It is also not entirely clear from there variable name what is the 
difference between fdt and pfdt.
Also, no more u32 in the code please. This is now part of the CODING_STYLE.


> +{
> +    int propoff, nameoff, r;
Please no single letter variable unless this is obvious to understand 
(such as i, j for iteration). This should be ret, rc or res.
Note that somehow you are using the 3 of them in the same patches... I 
am not going to ask for consistency thought.

> +    const struct fdt_property *prop;
> +
> +    for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> +          propoff >= 0;
> +          propoff = fdt_next_property_offset(pfdt, propoff) )
> +    {
> +
> +        if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
> +            return -FDT_ERR_INTERNAL;
> +
> +        nameoff = fdt32_to_cpu(prop->nameoff);
> +        r = fdt_property(fdt, fdt_string(pfdt, nameoff),
> +                         prop->data, fdt32_to_cpu(prop->len));
> +        if ( r )
> +            return r;
> +    }
> +
> +    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> +    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
> +}
> +
> +static int __init scan_pt_node(const void *pfdt,
> +                               int nodeoff, const char *name, int depth,
> +                               u32 address_cells, u32 size_cells,
Same here.

> +                               void *data)
> +{
> +    int rc;
> +    int i, num;
Anything that can't be negative, should be unsigned.

> +    struct kernel_info *kinfo = data;
> +    void *fdt = kinfo->fdt;
> +    int depth_next = depth;
> +    int node_next;
> +
> +    /* no need to parse initial node */
> +    if ( !depth )
> +        return 0;
So this is going to copy what ever is in the partial device-tree. In 
other word, the users can override pretty much anything that Xen 
created. I don't think this is what we want.
Instead, we should only copy nodes under /passthrough and also the /aliases.

> +
> +    rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
fdt_begin_node assume the second parameter will be non-NULL. What if 
fdt_get_name() returns NULL?
> +    if ( rc )
> +        return rc;
> +
> +    rc = handle_properties(kinfo->d, fdt, pfdt, nodeoff,
> +                           address_cells, size_cells);
> +    if ( rc )
> +        return rc;
> +
> +    node_next = fdt_next_node(pfdt, nodeoff, &depth_next);
What if node_next is invalid (i.e because it is negative)?

> +
> +    /*
> +     * If the next node is a sibling, then we need to call
> +     * fdt_end_node once. If the next node is one level up, we need to
> +     * call it twice: once for us and the second time for our parent.
> +     * Both these two conditions are expressed together by depth -
> +     * depth_next + 1.
> +     *
> +     * If we reached the end of the device tree fragment, then it is
> +     * easy: we need to call fdt_end_node once for every level of depth
> +     * to close all open nodes.
> +     */
> +    if ( depth_next < 0 )
> +        num = depth;
> +    else
> +        num = depth - depth_next + 1;
> +
> +    for ( i = 0; i < num; i++ )
> +    {
> +        rc = fdt_end_node(fdt);
> +        if ( rc )
> +            return rc;
> +    }
All this code is a bit complicated because you decided to use 
dt_tree_for_each_node that is not really suitable here. It would be 
possible to use recursion as we did for the dom0 DT thanks to 
fdt_first_subnode, fdt_next_subnode.
Something like:

handle_node
{
     fdt_begin_node(....);
     write_properties(...);
     node = fdt_first_subnode(...);

     while ( node > 0 )
     {
          handle_node(node...);
          node = fdt_next_subnode(...);
     }
     fdt_end_node(....);
}

> +
> +    return 0;
> +}
> +
> +static int __init domain_handle_dtb_bootmodule(struct domain *d,
> +                                               struct kernel_info *kinfo)
> +{
> +    void *pfdt;
> +    int res;
> +
> +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +            kinfo->dtb_bootmodule->size);
Indentation.

> +    if ( pfdt == NULL )
> +        return -EFAULT;
> +
> +    res = device_tree_for_each_node(pfdt, scan_pt_node, kinfo);
> +
> +    iounmap(pfdt);
> +
> +    return res;
> +}
> +
>   /*
>    * The max size for DT is 2MB. However, the generated DT is small, 4KB
>    * are enough for now, but we might have to increase it in the future.
> @@ -1777,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>               goto err;
>       }
>   
> +    if ( kinfo->dtb_bootmodule ) {
> +        ret = domain_handle_dtb_bootmodule(d, kinfo);
> +        if ( ret )
> +            return ret;
> +    }
> +
>       ret = fdt_end_node(kinfo->fdt);
>       if ( ret < 0 )
>           goto err;
> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> index 33f3e72b11..720dec4071 100644
> --- a/xen/include/asm-arm/kernel.h
> +++ b/xen/include/asm-arm/kernel.h
> @@ -28,7 +28,7 @@ struct kernel_info {
>       paddr_t gnttab_size;
>   
>       /* boot blob load addresses */
> -    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
> +    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, 
> *dtb_bootmodule;
>       const char* cmdline;
>       paddr_t dtb_paddr;
>       paddr_t initrd_paddr;
Cheers,

--
Julien Grall

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