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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 24 Dec 2018, Julien Grall wrote:
> (+ 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?

Wei confirmed that it is OK. I'll add a note about this in the commit
message.


> > 
> > 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 ( ... )
> {

OK


> > +
> > +        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.

OK


> > +        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?

OK


> > +        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.

OK


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