|
[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
On Wed, 25 Sep 2019, Julien Grall wrote:
> 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.
I'll mention it
> > +{
> > + 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.
I'll fix the coding style issues
>
> > + 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.
OK
> > + 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |