Re: [Xen-devel] [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb

[Julien Grall <julien.grall@xxxxxxx>]

(+ Wei and Ian)

Hi Stefano,

On 12/5/18 5:28 PM, 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.
The code below is basically a copy from libxl, right? If so, this should 
be specified in the commit message.
Also, the license is different in libxl compare to the hypervisor 
(LGPLv2.1 vs GPLv2). So is there any issue to copy that code in the 
hypervisor?
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c  | 77 ++++++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/kernel.h |  2 +-
>   2 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b0ec3f0..cc6b464 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>
> @@ -1669,6 +1670,59 @@ static int __init make_vpl011_uart_node(const struct 
> domain *d, void *fdt)
>   }
>   #endif
>   
> +static int __init copy_properties(void *fdt, void *pfdt, int nodeoff)
> +{
> +    int propoff, nameoff, r;
> +    const struct fdt_property *prop;
> +
> +    for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> +          propoff >= 0;
> +          propoff = fdt_next_property_offset(pfdt, propoff) ) {
Coding style:

for ( ... )
{

> +
> +        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 copy_node(void *fdt, void *pfdt, int nodeoff, int depth)
> +{
> +    int r;
> +
> +    r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
> +    if ( r )
> +        return r;
> +
> +    r = copy_properties(fdt, pfdt, nodeoff);
> +    if ( r )
> +        return r;
> +
> +    for ( nodeoff = fdt_first_subnode(pfdt, nodeoff);
> +          nodeoff >= 0;
> +          nodeoff = fdt_next_subnode(pfdt, nodeoff) ) {
Same here.

> +        r = copy_node(fdt, pfdt, nodeoff, depth + 1);
> +        if ( r )
> +            return r;
> +    }
> +
> +    if ( nodeoff != -FDT_ERR_NOTFOUND )
> +        return nodeoff;
> +
> +    r = fdt_end_node(fdt);
> +    if ( r )
> +        return r;
> +
> +    return 0;
> +}
> +
>   /*
>    * 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.
> @@ -1740,6 +1794,29 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>               goto err;
>       }
>   
> +    if ( kinfo->dtb_bootmodule ) {
> +        int nodeoff, res;
> +        void *pfdt;
> +
> +        pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +                             kinfo->dtb_bootmodule->size);
This suggests the DTB fragment should be cacheable. Can this be written 
in the documentation?
> +        if ( pfdt == NULL )
> +            return -EFAULT;
> +
> +        if ( fdt_magic(pfdt) != FDT_MAGIC )
> +            return -EINVAL;
> +
> +        nodeoff = fdt_path_offset(pfdt, "/passthrough");
> +        if (nodeoff < 0)
> +            return nodeoff;
> +
> +        res = copy_node(kinfo->fdt, pfdt, nodeoff, 0);
> +        if ( res )
> +            return res;
I would also copy /aliases as it may be used by the users to refer 
easily a node.
> +
> +        iounmap(pfdt);
> +    }
> +
>       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 33f3e72..720dec4 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