|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 2/6] xen/arm: copy dtb fragment to guest dtb
On Fri, 9 Aug 2019, Julien Grall wrote:
> 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?
I have renamed handle_properties to handle_prop_pfdt, to make it clear
that this one is specific to partial device tree fragments.
> It is also not entirely clear from there variable name what is the difference
> between fdt and pfdt.
I have clarified and reduced the list of parameters by passing a kinfo
instead of domain and fdt separately.
> Also, no more u32 in the code please. This is now part of the CODING_STYLE.
Interesting, I haven't been following. Should I use uint32_t? What's the
recommended practice for fixed sized integers now?
> > +{
> > + 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.
OK
> > + 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.
OK
> > + 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.
Very good point! I'll fix it.
> > +
> > + 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?
I'll add a check.
> > + 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)?
This goes away with the change described below.
> > +
> > + /*
> > + * 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(....);
> }
Yes, I have followed your suggestion, and I have now a recursive
implementation without any calls to dt_tree_for_each_node, very similar
to the one you described here.
> > +
> > + 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.
OK
> > + 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;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |