[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Julien Grall <Julien.Grall@xxxxxxx>
  • Date: Wed, 25 Sep 2019 21:02:49 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=zgxOjYTSaiz+Cj0HwSVdyHuP5DzVFZhvNREmyKn6sqc=; b=UpLYOy8cOTO0eTOn36eyxxye0LbU6Burfoa3csubo4JQzvqkiBYh8xHI5tLyVRH/31ypdEivojS0W8809LlW0TAPSwzD/MkOv4VmkTDEHkPlGrwUp0kR0oSR1CgwZ1rL0hMukdukWSEXtLonAEhCg7vRBMWMDYoIVqjxV4+XoNlq1J+7/40rTMoBuy9S7E4oeHZJOR+a5zQ/iCB0n2XEUf/lwS43JYb0GRRqB86DczsH21vDwS9VqhWUUAbaw6w9RX425W0+bA2VOlkBM4pf4jpVhiRb+e5aY2mOBg1s/ke5ydf1IFTaNsU+jR4T8IIM/hG7tvguvJHPqZsfRY4OOQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=W3B6cX4obRYKhAkxsv5S5GpH79WfbtQo8soVqGtOYnZpkHUVcARdNjAx56xwu7pJk4lbblwpD7E0SN9lr67dWwmsftT8N1BVWsw4GWLbE00s9pxH0BqW/qxK0DVGMe7lISQL2NXXBIpDjqU1g6+FLGx9uU6mU4X3j9XQMiGgEkoC+Zdj5tUkdRFU7uMq51fgGmQat2zw1X4q/zCTK2dlNzGSaR5nv6HSOvsDj8wwuP6pzqPI5q4OOnxcKUHhUdMHSVOOH/qAu7M1LJF8NPvQsqDgX9aYPKntRtYoKbUYt2enQEbApOmMNSx7foSsJufTP8G4E99RoDL32VBKZqxJLQ==
  • Authentication-results: spf=temperror (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; lists.xen.org; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;lists.xen.org; dmarc=none action=none header.from=arm.com;
  • Authentication-results-original: spf=none (sender IP is ) smtp.mailfrom=Julien.Grall@xxxxxxx;
  • Cc: Stefano Stabellini <stefanos@xxxxxxxxxx>, "andrii_anisov@xxxxxxxx" <andrii_anisov@xxxxxxxx>, Achin Gupta <Achin.Gupta@xxxxxxx>, "xen-devel@xxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxx>, nd <nd@xxxxxxx>, "Volodymyr_Babchuk@xxxxxxxx" <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 25 Sep 2019 21:03:11 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: True
  • Original-authentication-results: spf=none (sender IP is ) smtp.mailfrom=Julien.Grall@xxxxxxx;
  • Thread-index: AQHVc9H3bYctjOIg50y1qa3V1Yo1qac84YUA
  • Thread-topic: [PATCH v5 4/8] xen/arm: copy dtb fragment to guest dtb


On 25/09/2019 19:49, 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->guest_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.
> 
> 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 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  | 156 +++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/kernel.h |   2 +-
>   2 files changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 32f85cd959..9d985d3bbe 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>
> @@ -1700,6 +1701,154 @@ 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)

Well, the introduce of scan_passthrough_prop makes little sense as you 
are not using until a follow-up patch. I am ok if you introduce it here, 
but this should at least be mentioned in the commit message.

> +{
> +    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) {

Coding style:

if ( ... )
{

> +        dprintk(XENLOG_ERR, "Partial FDT is not a valid Flat Device Tree");
> +        return -EINVAL;
> +    }
> +
> +    res = fdt_check_header(pfdt);
> +    if (res) {

Same here.

> +        dprintk(XENLOG_ERR, "Failed to check the partial FDT (%d)", res);
> +        return -EINVAL;
> +    }
> +
> +    if (fdt_totalsize(pfdt) > size) {

Same here.

> +        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->guest_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);

NIT: This is not aligned correctly.

> +            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);
Same here.

> +            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.
> @@ -1755,6 +1904,13 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>       if ( ret )
>           goto err;
>   

I would add a comment here mentioning that the code below always need to 
be called before the rest of the DT is generated. This is because you 
depend on the value of the field guest_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 760434369b..7f5e659561 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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.