Re: [PATCH RFC 12/15] xen/arm: generate vpl011 node on device tree for domU

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 06/13/2018 11:15 PM, 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.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 70 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b4f560f..ff65057 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1470,6 +1470,70 @@ static int make_timer_domU_node(const struct domain *d, void *fdt)
>       return res;
>   }
>   
> +static void set_interrupt(gic_interrupt_t *interrupt, unsigned int irq,

The definition of interrupt looks suspicious. gic_interrupt_t is defined 
as be32[3]. Here you pass a pointer, so interrupt type would be __be32 
**, that you crudely cast to __be32* below.

Most likely you don't want to pass a pointer here and just use the type 
gic_interrupt_t. Because it is an array, then there will be no issue.

> +                          unsigned int cpumask, unsigned int level)
> +{
> +    __be32 *cells = (__be32 *) interrupt;

Explicit cast are always a bad idea. If you need one, then mostly likely 
you did something wrong :). In that case interrupt type is __be32** and 
you cast to __be32*. If you change the type as suggested above, then the 
cast will not be necessary here.

> +    int is_ppi = (irq < 32);
> +
> +    irq -= (is_ppi) ? 16: 32; /* PPIs start at 16, SPIs at 32 */
> +
> +    /* See linux Documentation/devictree/bindings/arm/gic.txt */
> +    dt_set_cell(&cells, 1, is_ppi); /* is a PPI? */
> +    dt_set_cell(&cells, 1, irq);
> +    dt_set_cell(&cells, 1, (cpumask << 8) | level);
> +}

We already have a function to generate PPI interrupt 
(set_interrupt_ppi). Would it be possible to extend it to support interrupt?

Most likely, you will want to use set_interrupt(...) everywhere and just 
drop set_interrupt_ppi.

> +
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
> +static int 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");
> +    if (res)

Coding style:

if ( ... )

> +        return res;
> +
> +    res = fdt_property_string(fdt, "compatible", "arm,sbsa-uart");

To make clear, you are exposing 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 your guest may require some changes in 
their driver.

I think this is a small price to pay, but I wanted to make sure you 
don't expect the guest to drive the UART the same way a PL011.

> +    if (res)

Coding style

> +        return res;
> +
> +    cells = &reg[0];
> +    dt_child_set_range(&cells, addrcells, sizecells, GUEST_PL011_BASE,
> +            GUEST_PL011_SIZE);

The indentation looks wrong here.

> +    if (res)

Coding style

> +        return res;
> +    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> +    if (res)

Coding style

> +        return res;
> +
> +    set_interrupt(&intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> +    res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> +    if (res)

Coding style

> +        return res;
> +
> +    res = fdt_property_cell(fdt, "interrupt-parent",
> +                            PHANDLE_GIC);
> +    if (res)

Coding style

> +        return res;
> +
> +    /* Use a default baud rate of 115200. */
> +    fdt_property_u32(fdt, "current-speed", 115200);
> +
> +    res = fdt_end_node(fdt);
> +    if (res)

Coding style

> +        return res;
> +
> +    return 0;
> +}
> +#endif
> +
>   #define DOMU_DTB_SIZE 4096
>   static int prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>   {
> @@ -1531,6 +1595,12 @@ static int prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>       if ( ret )
>           goto err;
>   
> +#ifdef CONFIG_SBSA_VUART_CONSOLE.
> +    ret = make_vpl011_uart_node(d, kinfo->fdt, addrcells, sizecells);

I would prefer if don't expose the pl011 by default to a guest and 
provide a way to enable it for a given guest

> +    if ( ret )
> +        goto err;
> +#endif
> +
>       ret = fdt_end_node(kinfo->fdt);
>       if ( ret < 0 )
>           goto err;
> 

Cheers,

-- 
Julien Grall

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