Re: [Xen-devel] [PATCH v3 18/25] xen/arm: generate a simple device tree for domUs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/08/18 00:28, Stefano Stabellini wrote:
> > Introduce functions to generate a basic domU device tree, similar to the
> > existing functions in tools/libxl/libxl_arm.c.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - remove CONFIG_ACPI for make_chosen_node
> > - remove make_hypervisor_node until all Xen related functionalities
> >    (evtchns, grant table, etc.) work correctly
> > 
> > Changes in v2:
> > - move prepare_dtb rename to previous patch
> > - use switch for the gic version
> > - use arm,gic-400 instead of arm,cortex-a15-gic
> > - add @unit-address in the gic node name
> > - add comment on DOMU_DTB_SIZE
> > ---
> >   xen/arch/arm/domain_build.c | 211
> > +++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 209 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index dfa74e4..167a56e 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1057,7 +1057,6 @@ static int __init make_timer_node(const struct domain
> > *d, void *fdt,
> >       return res;
> >   }
> >   -#ifdef CONFIG_ACPI
> >   /*
> >    * This function is used as part of the device tree generation for Dom0
> >    * on ACPI systems, and DomUs started directly from Xen based on device
> > @@ -1103,7 +1102,6 @@ static int __init make_chosen_node(const struct
> > kernel_info *kinfo)
> >         return res;
> >   }
> > -#endif
> >     static int __init map_irq_to_domain(struct domain *d, unsigned int irq,
> >                                       bool need_mapping, const char
> > *devname)
> > @@ -1496,6 +1494,211 @@ static int __init handle_node(struct domain *d,
> > struct kernel_info *kinfo,
> >       return res;
> >   }
> >   +static int __init make_gic_domU_node(const struct domain *d, void *fdt,
> > int addrcells, int sizecells)
> > +{
> > +    int res = 0;
> > +    int reg_size = addrcells + sizecells;
> > +    int nr_cells = reg_size * 2;
> > +    __be32 reg[nr_cells];
> > +    __be32 *cells;
> 
> You are trying to be save a few lines by handling both GICv2 and GICv3 node in
> a single function. But this only thing it adds is confusion when reading the
> code.
> 
> Please have two functions (one for each GIC version) to generate the DT. This
> would also help to add ITS/GICv2m support in the future.

OK


> > +
> > +    switch ( gic_hw_version() )
> > +    {
> > +    case GIC_V3:
> > +        res = fdt_begin_node(fdt,
> > "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
> > +        break;
> > +    case GIC_V2:
> > +        res = fdt_begin_node(fdt,
> > "interrupt-controller@"__stringify(GUEST_GICD_BASE));
> > +        break;
> > +    default:
> > +        panic("Unsupported GIC version");
> > +    }
> > +
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#address-cells", 0);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> > +    if ( res )
> > +        return res;
> > +
> > +    switch ( gic_hw_version() )
> > +    {
> > +    case GIC_V3:
> > +    {
> > +        const uint64_t gicd_base = GUEST_GICV3_GICD_BASE;
> > +        const uint64_t gicd_size = GUEST_GICV3_GICD_SIZE;
> > +        const uint64_t gicr0_base = GUEST_GICV3_GICR0_BASE;
> > +        const uint64_t gicr0_size = GUEST_GICV3_GICR0_SIZE;
> 
> I am not entirely convinced of the usefulness of those variables.

I can remove

> > +
> > +        res = fdt_property_string(fdt, "compatible", "arm,gic-v3");
> > +        if ( res )
> > +            return res;
> > +
> > +        cells = &reg[0];
> > +        dt_child_set_range(&cells, addrcells, sizecells, gicd_base,
> > gicd_size);
> > +        dt_child_set_range(&cells, addrcells, sizecells, gicr0_base,
> > gicr0_size);
> > +        res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +        break;
> > +    }
> > +    case GIC_V2:
> > +    {
> > +        const uint64_t gicd_base = GUEST_GICD_BASE;
> > +        const uint64_t gicd_size = GUEST_GICD_SIZE;
> > +        const uint64_t gicc_base = GUEST_GICC_BASE;
> > +        const uint64_t gicc_size = GUEST_GICC_SIZE;
> 
> Same here.
>
> > +
> > +        res = fdt_property_string(fdt, "compatible", "arm,gic-400");
> > +        if ( res )
> > +            return res;
> > +
> > +        cells = &reg[0];
> > +        dt_child_set_range(&cells, addrcells, sizecells, gicd_base,
> > gicd_size);
> > +        dt_child_set_range(&cells, addrcells, sizecells, gicc_base,
> > gicc_size);
> > +        break;
> > +    }
> > +    default:
> > +        break;
> > +    }
> > +
> > +    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +    if (res)
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC);
> > +    if (res)
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC);
> > +    if (res)
> > +        return res;
> > +
> > +    res = fdt_end_node(fdt);
> > +
> > +    return res;
> > +}
> > +
> > +static int __init make_timer_domU_node(const struct domain *d, void *fdt)
> > +{
> > +    int res;
> > +    gic_interrupt_t intrs[3];
> > +
> > +    res = fdt_begin_node(fdt, "timer");
> > +    if ( res )
> > +        return res;
> > +
> > +    if (!is_64bit_domain(d))
> 
> Coding style:
> 
> if ( ... )

OK


> > +    {
> > +        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> > +        if ( res )
> > +            return res;
> > +    } else {
> 
> Coding style:
> 
> }
> else
> {

OK


> > +        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    set_interrupt_ppi(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt_ppi(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt_ppi(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > +
> > +    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "interrupt-parent",
> > +                            GUEST_PHANDLE_GIC);
> > +    if (res)
> > +        return res;
> > +
> > +    res = fdt_end_node(fdt);
> 
> Newline here please.

OK


> > +    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 feature.
> 
> s/feature/future/

*blush*


> > + */
> > +#define DOMU_DTB_SIZE 4096
> > +static int __init prepare_dtb_domU(struct domain *d, struct kernel_info
> > *kinfo)
> > +{
> > +    int addrcells, sizecells;
> > +    int ret;
> > +
> > +    addrcells = dt_child_n_addr_cells(dt_host); > +    sizecells =
> > dt_child_n_size_cells(dt_host);
> 
> Why do you use the host DT to find out the the #address-cells and #size-cells?
> Should not this be independent?

Yes, that is a mistake. I'll fix.


> > +
> > +    kinfo->fdt = xmalloc_bytes(DOMU_DTB_SIZE);
> > +    if ( kinfo->fdt == NULL )
> > +        return -ENOMEM;
> > +
> > +    ret = fdt_create(kinfo->fdt, DOMU_DTB_SIZE);
> > +    if ( ret < 0 )
> > +        goto err;
> > +
> > +    ret = fdt_finish_reservemap(kinfo->fdt);
> > +    if ( ret < 0 )
> > +        goto err;
> > +
> > +    ret = fdt_begin_node(kinfo->fdt, "/");
> > +    if ( ret < 0 )
> > +        goto err;
> > +
> > +    ret = fdt_property_cell(kinfo->fdt, "#address-cells", addrcells);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = fdt_property_cell(kinfo->fdt, "#size-cells", sizecells);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = make_chosen_node(kinfo);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = make_psci_node(kinfo->fdt, NULL);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = make_cpus_node(d, kinfo->fdt, NULL);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = make_gic_domU_node(d, kinfo->fdt, addrcells, sizecells);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = make_timer_domU_node(d, kinfo->fdt);
> > +    if ( ret )
> > +        goto err;
> > +
> > +    ret = fdt_end_node(kinfo->fdt);
> > +    if ( ret < 0 )
> > +        goto err;
> > +
> > +    ret = fdt_finish(kinfo->fdt);
> > +    if ( ret < 0 )
> > +        goto err;
> > +
> > +    return 0;
> > +
> > +  err:
> > +    printk("Device tree generation failed (%d).\n", ret);
> > +    xfree(kinfo->fdt);
> 
> Newline here please.

OK


> > +    return -EINVAL;
> > +}
> > +
> >   static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info
> > *kinfo)
> >   {
> >       const p2m_type_t default_p2mt = p2m_mmio_direct_c;
> > @@ -2366,6 +2569,10 @@ static int __init construct_domU(struct domain *d,
> > struct dt_device_node *node)
> >   #endif
> >       allocate_memory(d, &kinfo);
> >   +    rc = prepare_dtb_domU(d, &kinfo);
> > +    if ( rc < 0 )
> > +        return rc;
> > +
> >       return __construct_domain(d, &kinfo);
> >   }
> >   
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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