Re: [Xen-devel] [PATCH v6 4/8] xen/arm: copy dtb fragment to guest dtb

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 9/27/19 12:11 AM, Stefano Stabellini wrote:
> Read the dtb fragment corresponding to a passthrough device from memory
> at the location referred to by the "multiboot,device-tree" compatible
> node.
>
> Add a new field named dtb_bootmodule to struct kernel_info to keep track
> of the dtb fragment location.
>
> Copy the fragment to the guest dtb (only /aliases and /passthrough).
>
> Set kinfo->phandle_gic based on the phandle of the special "/gic"
> node in the device tree fragment. "/gic" is a dummy node in the dtb
> fragment that represents the gic interrupt controller. Other properties
> in the dtb fragment might refer to it (for instance interrupt-parent of
> a device node). We reuse the phandle of "/gic" from the dtb fragment as
> the phandle of the full GIC node that will be created for the guest
> device tree. That way, when we copy properties from the device tree
> fragment to the domU device tree the links remain unbroken.
>
> scan_passthrough_prop is introduced here and not used in this patch but
> it will be used by later patches.
>
> 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>
Acked-by: Julien Grall <julien.grall@xxxxxxx>

Cheers,

> ----
> Changes in v6:
> - code style
> - in-code comment
> - commit message improvements
>
> Changes in v5:
> - code style
> - in-code comment
> - remove depth parameter from scan_pfdt_node
> - for instead of loop in domain_handle_dtb_bootmodule
> - move "gic" check to domain_handle_dtb_bootmodule
> - add check_partial_fdt
> - use DT_ROOT_NODE_ADDR/SIZE_CELLS_DEFAULT
> - add scan_passthrough_prop parameter, set it to false for "/aliases"
>
> Changes in v4:
> - use recursion in the implementation
> - rename handle_properties to handle_prop_pfdt
> - rename scan_pt_node to scan_pfdt_node
> - pass kinfo to handle_properties
> - use uint32_t instead of u32
> - rename r to res
> - add "passthrough" and "aliases" check
> - add a name == NULL check
> - code style
> - move DTB fragment scanning earlier, before DomU GIC node creation
> - set guest_phandle_gic based on "/gic"
> - in-code comment
>
> 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  | 164 +++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/kernel.h |   2 +-
>   2 files changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b25abe8d08..08d6d238e3 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>
> @@ -1698,6 +1699,157 @@ static int __init make_vpl011_uart_node(struct 
> kernel_info *kinfo)
>   }
>   #endif
>   
> +static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> +                                   const void *pfdt, int nodeoff,
> +                                   uint32_t address_cells, uint32_t size_cells,
> +                                   bool scan_passthrough_prop)
> +{
> +    void *fdt = kinfo->fdt;
> +    int propoff, nameoff, res;
> +    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);
> +        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> +                           prop->data, fdt32_to_cpu(prop->len));
> +        if ( res )
> +            return res;
> +    }
> +
> +    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> +    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
> +}
> +
> +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
> +                                 int nodeoff,
> +                                 uint32_t address_cells, uint32_t size_cells,
> +                                 bool scan_passthrough_prop)
> +{
> +    int rc = 0;
> +    void *fdt = kinfo->fdt;
> +    int node_next;
> +
> +    rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
> +    if ( rc )
> +        return rc;
> +
> +    rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells,
> +                          scan_passthrough_prop);
> +    if ( rc )
> +        return rc;
> +
> +    address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells",
> +                                        DT_ROOT_NODE_ADDR_CELLS_DEFAULT);
> +    size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
> +                                     DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
> +
> +    node_next = fdt_first_subnode(pfdt, nodeoff);
> +    while ( node_next > 0 )
> +    {
> +        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> +                       scan_passthrough_prop);
> +        node_next = fdt_next_subnode(pfdt, node_next);
> +    }
> +
> +    return fdt_end_node(fdt);
> +}
> +
> +static int __init check_partial_fdt(void *pfdt, size_t size)
> +{
> +    int res;
> +
> +    if ( fdt_magic(pfdt) != FDT_MAGIC )
> +    {
> +        dprintk(XENLOG_ERR, "Partial FDT is not a valid Flat Device Tree");
> +        return -EINVAL;
> +    }
> +
> +    res = fdt_check_header(pfdt);
> +    if ( res )
> +    {
> +        dprintk(XENLOG_ERR, "Failed to check the partial FDT (%d)", res);
> +        return -EINVAL;
> +    }
> +
> +    if ( fdt_totalsize(pfdt) > size )
> +    {
> +        dprintk(XENLOG_ERR, "Partial FDT totalsize is too big");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int __init domain_handle_dtb_bootmodule(struct domain *d,
> +                                               struct kernel_info *kinfo)
> +{
> +    void *pfdt;
> +    int res, node_next;
> +
> +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +                         kinfo->dtb_bootmodule->size);
> +    if ( pfdt == NULL )
> +        return -EFAULT;
> +
> +    res = check_partial_fdt(pfdt, kinfo->dtb_bootmodule->size);
> +    if ( res < 0 )
> +        return res;
> +
> +    for ( node_next = fdt_first_subnode(pfdt, 0);
> +          node_next > 0;
> +          node_next = fdt_next_subnode(pfdt, node_next) )
> +    {
> +        const char *name = fdt_get_name(pfdt, node_next, NULL);
> +
> +        if ( name == NULL )
> +            continue;
> +
> +        /*
> +         * Only scan /gic /aliases /passthrough, ignore the rest.
> +         * They don't have to be parsed in order.
> +         *
> +         * Take the GIC phandle value from the special /gic node in the
> +         * DTB fragment.
> +         */
> +        if ( dt_node_cmp(name, "gic") == 0 )
> +        {
> +            kinfo->phandle_gic = fdt_get_phandle(pfdt, node_next);
> +            continue;
> +        }
> +
> +        if ( dt_node_cmp(name, "aliases") == 0 )
> +        {
> +            res = scan_pfdt_node(kinfo, pfdt, node_next,
> +                                 DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
> +                                 DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> +                                 false);
> +            if ( res )
> +                return res;
> +            continue;
> +        }
> +        if ( dt_node_cmp(name, "passthrough") == 0 )
> +        {
> +            res = scan_pfdt_node(kinfo, pfdt, node_next,
> +                                 DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
> +                                 DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> +                                 true);
> +            if ( res )
> +                return res;
> +            continue;
> +        }
> +    }
> +
> +    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.
> @@ -1753,6 +1905,18 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>       if ( ret )
>           goto err;
>   
> +    /*
> +     * domain_handle_dtb_bootmodule has to be called before the rest of
> +     * the device tree is generated because it depends on the value of
> +     * the field phandle_gic.
> +     */
> +    if ( kinfo->dtb_bootmodule )
> +    {
> +        ret = domain_handle_dtb_bootmodule(d, kinfo);
> +        if ( ret )
> +            return ret;
> +    }
> +
>       ret = make_gic_domU_node(kinfo);
>       if ( ret )
>           goto err;
> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> index 43fa87545e..874aa108a7 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;
--
Julien Grall

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