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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 25 Sep 2019, Julien Grall wrote:
> On 24/09/2019 22:06, Stefano Stabellini wrote:
> > On Wed, 11 Sep 2019, Julien Grall wrote:
> > > On 8/21/19 4:53 AM, Stefano Stabellini wrote:
> > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > 
> > > > ----
> > > > 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  | 112
> > > > +++++++++++++++++++++++++++++++++++
> > > >    xen/include/asm-arm/kernel.h |   2 +-
> > > >    2 files changed, 113 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > index cd585f05ca..c71b9f2889 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>
> > > > @@ -1713,6 +1714,111 @@ 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)
> > > 
> > > Why do you need address_cells and size_cells in parameter?
> > 
> > Yes, it will be necessary for later patches.
> 
> ok.
> 
> > 
> > 
> > > > +{
> > > > +    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, int depth,
> > > > +                                 uint32_t address_cells, uint32_t
> > > > size_cells)
> > > > +{
> > > > +    int rc = 0;
> > > > +    void *fdt = kinfo->fdt;
> > > > +    int node_next;
> > > > +    const char *name = fdt_get_name(pfdt, nodeoff, NULL);
> > > > +
> > > > +    /*
> > > > +     * Take the GIC phandle value from the special /gic node in the DTB
> > > > +     * fragment.
> > > > +     */
> > > > +    if ( depth == 1 && dt_node_cmp(name, "gic") == 0 )
> > > > +    {
> > > > +        kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, nodeoff);
> > > > +        return 0;
> > > > +    }
> > > 
> > > I don't like this solution. You are bypassing most of the function just
> > > for
> > > the benefits of have the name in hand. Can this be done separately? This
> > > would
> > > also avoid to have an extra parameter (depth) for the only benefits of
> > > this
> > > check.
> > 
> > All right, I'll change it and remove depth.
> 
> Thinking again about this function, you will allow a users to describe a
> device in the node /aliases. So there are more to do in this function.

Yes, that's true. I can pass a parameter to make sure the proper
behavior is applied to /aliases (only copy) and /passthrough (copy and
look for device assignment properties).


> > > > +
> > > > +    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);
> > > > +    if ( rc )
> > > > +        return rc;
> > > > +
> > > > +    address_cells = device_tree_get_u32(pfdt, nodeoff,
> > > > "#address-cells",
> > > > +                                        address_cells);
> > > > +    size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
> > > > +                                     size_cells);
> > > 
> > > I am pretty sure I mention it before (though not on this patch...), this
> > > is
> > > not matching the DT spec. address_cells and size_cells are not propagated
> > > to
> > > the next level. So these should be DT_ROOT_NODE_{ADDR,
> > > SIZE}_CELLS_DEFAULT.
> > 
> > They are only propagated from parent to children, not from parent to
> > grandchildren. This function is recursive. In this case we are reading
> > #address-cells and #size-cells just to pass it on by 1 level as by the
> > spec. Am I misunderstanding something?
> 
> I am afraid this is not correct. device_tree_get_u32 take the default number
> of cells as 3rd parameter. This is used if the property does not exist.
> 
> So if the children does not have the two properties, then you will end up to
> use the parent's value as default when parsing grandchildren "reg" properties.

I understand your point now. I'll fix it.


> > > > +
> > > > +    node_next = fdt_first_subnode(pfdt, 0);
> > > > +    while ( node_next > 0 )
> > > > +    {
> > > 
> > > Why do we have to go through the all the nodes of the first level? Can't
> > > we
> > > just lookup for the path and copy the node as libxl does?
> > 
> > Yes, we could do that, but fdt_path_offset is implemented as a loop
> > anyway and we would still have the same "gic", "aliases" and
> > "passthrough" checks. I tried the change but the code doesn't look much
> > nicer and we would end up increasing runtime complexity.
> 
> This is ok as long as they don't depend on each other. This is not very clear
> from the code that "/gic" does not need to be parsed first, so you may want to
> explain in a comment.

OK, I'll clarify

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel