Re: [Xen-devel] [PATCH v3 19/25] xen/arm: generate vpl011 node on device tree for domU

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/08/18 00:28, Stefano Stabellini wrote:
> > Introduce vpl011 support to guests started from Xen: it provides a
> > simple way to print output from a guest, as most guests come with a
> > pl011 driver. It is also able to provide a working console with
> > interrupt support.
> > 
> > The UART exposed to the guest is a SBSA compatible UART and not a PL011.
> > SBSA UART is a subset of PL011 r1p5. A full PL011 implementation in Xen
> > would just be too difficult, so guests may require some drivers changes.
> > 
> > Enable vpl011 conditionally if the user requested it.
> > 
> > Make set_interrupt_ppi able to handle non-PPI and rename it
> > set_interrupt.
> 
> It would have been better to have this change in a separate patch.

I'll separate it


> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - use bool
> > - retain BUG_ON(irq < 16)
> > - add vpl011 bool to kinfo
> > - return error of vpl011 is required but CONFIG_SBSA_VUART_CONSOLE is
> >    missing
> > 
> > Changes in v2:
> > - code style fixes
> > - make set_interrupt_ppi generic
> > - rename set_interrupt_ppi to set_interrupt
> > - only make the vpl011 node if the option was enabled
> > ---
> >   xen/arch/arm/domain_build.c | 95
> > ++++++++++++++++++++++++++++++++++++++-------
> >   xen/arch/arm/kernel.h       |  3 ++
> >   2 files changed, 83 insertions(+), 15 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 167a56e..f9fa484 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -619,19 +619,20 @@ static int __init write_properties(struct domain *d,
> > struct kernel_info *kinfo,
> >     typedef __be32 gic_interrupt_t[3];
> >   -static void __init set_interrupt_ppi(gic_interrupt_t interrupt,
> > -                                     unsigned int irq,
> > -                                     unsigned int cpumask,
> > -                                     unsigned int level)
> > +static void __init set_interrupt(gic_interrupt_t interrupt,
> > +                                 unsigned int irq,
> > +                                 unsigned int cpumask,
> > +                                 unsigned int level)
> >   {
> >       __be32 *cells = interrupt;
> > +    bool is_ppi = !!(irq < 32);
> >         BUG_ON(irq < 16);
> > -    BUG_ON(irq >= 32);
> > +    irq -= (is_ppi) ? 16: 32; /* PPIs start at 16, SPIs at 32 */
> >         /* See linux
> > Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
> > -    dt_set_cell(&cells, 1, 1); /* is a PPI */
> > -    dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
> > +    dt_set_cell(&cells, 1, is_ppi); /* is a PPI? */
> > +    dt_set_cell(&cells, 1, irq);
> >       dt_set_cell(&cells, 1, (cpumask << 8) | level);
> >   }
> >   @@ -752,7 +753,7 @@ static int __init make_hypervisor_node(struct domain
> > *d,
> >        *  - All CPUs
> >        *  TODO: Handle properly the cpumask;
> >        */
> > -    set_interrupt_ppi(intr, d->arch.evtchn_irq, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> >       res = fdt_property_interrupts(fdt, &intr, 1);
> >       if ( res )
> >           return res;
> > @@ -1029,15 +1030,15 @@ static int __init make_timer_node(const struct
> > domain *d, void *fdt,
> >         irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> >       dt_dprintk("  Secure interrupt %u\n", irq);
> > -    set_interrupt_ppi(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> >         irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> >       dt_dprintk("  Non secure interrupt %u\n", irq);
> > -    set_interrupt_ppi(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> >         irq = timer_get_irq(TIMER_VIRT_PPI);
> >       dt_dprintk("  Virt interrupt %u\n", irq);
> > -    set_interrupt_ppi(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> >         res = fdt_property_interrupts(fdt, intrs, 3);
> >       if ( res )
> > @@ -1605,9 +1606,9 @@ static int __init make_timer_domU_node(const struct
> > domain *d, void *fdt)
> >               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);
> > +    set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> > +    set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf,
> > DT_IRQ_TYPE_LEVEL_LOW);
> >         res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
> >       if ( res )
> > @@ -1622,6 +1623,56 @@ static int __init make_timer_domU_node(const struct
> > domain *d, void *fdt)
> >       return res;
> >   }
> >   +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > +static int __init make_vpl011_uart_node(const struct domain *d, void *fdt,
> > +                                 int addrcells, int sizecells)
> > +{
> > +    int res;
> > +    gic_interrupt_t intr;
> > +    int reg_size = addrcells + sizecells;
> > +    int nr_cells = reg_size;
> > +    __be32 reg[nr_cells];
> > +    __be32 *cells;
> > +
> > +    res = fdt_begin_node(fdt, "sbsa-pl011");
> 
> Node should contain the address in the name. Also, the name should be SBSA
> UART (which happens to be based on PL011 :)).

OK


> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_string(fdt, "compatible", "arm,sbsa-uart");
> > +    if ( res )
> > +        return res;
> > +
> > +    cells = &reg[0];
> > +    dt_child_set_range(&cells, addrcells, sizecells, GUEST_PL011_BASE,
> > +                       GUEST_PL011_SIZE);
> > +    if ( res )
> > +        return res;
> > +    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +    if ( res )
> > +        return res;
> > +
> > +    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> > +
> > +    res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "interrupt-parent",
> > +                            GUEST_PHANDLE_GIC);
> > +    if ( res )
> > +        return res;
> > +
> > +    /* Use a default baud rate of 115200. */
> > +    fdt_property_u32(fdt, "current-speed", 115200);
> > +
> > +    res = fdt_end_node(fdt);
> > +    if ( res )
> > +        return res;
> > +
> > +    return 0;
> > +}
> > +#endif
> > +
> >   /*
> >    * 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.
> > @@ -1683,6 +1734,18 @@ static int __init prepare_dtb_domU(struct domain *d,
> > struct kernel_info *kinfo)
> >       if ( ret )
> >           goto err;
> >   +    if ( kinfo->vpl011 )
> > +    {
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > +        ret = make_vpl011_uart_node(d, kinfo->fdt, addrcells, sizecells);
> > +        if ( ret )
> > +            goto err;
> > +#else
> > +        ret = -EINVAL;
> > +        goto err;
> > +#endif
> 
> I would prefer:
> 
> 
> ret = -EINVAL
> #ifdef CONFIG_SBSA...
>       ret = make...
> #endif
>       if ( ret )
>         goto err;

Yes, that's better


> 
> > +    }
> > +
> >       ret = fdt_end_node(kinfo->fdt);
> >       if ( ret < 0 )
> >           goto err;
> > @@ -2537,7 +2600,7 @@ static int __init construct_domU(struct domain *d,
> > struct dt_device_node *node)
> >   {
> >       struct kernel_info kinfo = {};
> >       int rc;
> > -    u32 mem;
> > +    u32 mem, len;
> >         printk("*** LOADING DOMU ***\n");
> >   @@ -2550,6 +2613,8 @@ static int __init construct_domU(struct domain *d,
> > struct dt_device_node *node)
> >           return -EINVAL;
> >       kinfo.unassigned_mem = (paddr_t)mem << 10;
> >   +    kinfo.vpl011 = dt_get_property(node, "vpl011", &len) != NULL;
> > +
> >       d->vcpu = xzalloc_array(struct vcpu *, d->max_vcpus);
> >       if ( !d->vcpu )
> >           return -ENOMEM;;
> > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> > index 4a65289..9c9e568 100644
> > --- a/xen/arch/arm/kernel.h
> > +++ b/xen/arch/arm/kernel.h
> > @@ -33,6 +33,9 @@ struct kernel_info {
> >       paddr_t dtb_paddr;
> >       paddr_t initrd_paddr;
> >   +    /* Enable pl011 emulation */
> > +    bool vpl011;
> > +
> >       /* loader to use for this kernel */
> >       void (*load)(struct kernel_info *info);
> >       /* loader specific state */
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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