Re: [Xen-devel] [PATCH 06/11] xen/arm: vpl011: Add a new pl011 uart node in the guest DT in the toolstack

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Feb 21, 2017 at 04:56:03PM +0530, Bhupinder Thakur wrote:
> Add a new pl011 uart node
>     - Get the pl011 spi virq from Xen using a hvm call
>     - Add a new device tree node in the guest DT for SBSA pl011 uart 
> containing the IRQ
>       (read above) and the MMIO address range to be used by the guest
> 
> The format for the node is specified in 
> Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt.

Why don't you just copy-n-paste it in? It is only 10 linues.

> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..34c7e39 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -130,9 +130,10 @@ static struct arch_info {
>      const char *guest_type;
>      const char *timer_compat;
>      const char *cpu_compat;
> +    const char *uart_compat;
>  } arch_info[] = {
> -    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
> -    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
> +    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" 
> },
> +    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
>  };
>  
>  /*
> @@ -590,6 +591,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
> +                           const struct arch_info *ainfo,
> +                           struct xc_dom_image *dom, uint64_t irq)

Hm, mayube my editor is wrong but these arguments don't appear
to be right..

> +{
> +    int res;
> +    gic_interrupt intr;
> +
> +    res = fdt_begin_node(fdt, "sbsa-pl011");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
> +    if (res) return res;
> +
> +    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> +                            1,
> +                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
> +    if (res) 
> +        return res;

Shouldn't we free them?

> +
> +    set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);

0xF looks like a good candidate for #define.
> +
> +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> +    if (res) return res;

Again, shouldn't we free it if we things go south?
> +
> +    fdt_property_u32(fdt, "current-speed", 115200);

So perhaps a #define?
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>  static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                               const struct xc_dom_image *dom)
>  {
> @@ -790,6 +823,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_build_info *info,
>      int rc, res;
>      size_t fdt_size = 0;
>      int pfdt_size = 0;
> +    uint64_t vpl011_irq=0;

Does it have to be =0 ?
>  
>      const libxl_version_info *vers;
>      const struct arch_info *ainfo;
> @@ -889,6 +923,13 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>  
> +        /* 
> +         * get the vpl011 VIRQ and use it for creating a vpl011 node entry
> +         */
> +        if ( !xc_hvm_param_get(dom->xch, dom->guest_domid, 
> HVM_PARAM_VPL011_VIRQ, 
> +                                                               &vpl011_irq) )

Why is the &vpl011 so far right? It should be right after the (

> +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom, vpl011_irq) );

Ah, so it will just fail with an goto out and leak.
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>  
> @@ -933,9 +974,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>      val |= GUEST_EVTCHN_PPI;
>      rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ,
>                            val);
> +

Ahem?
>      if (rc)
>          return rc;
>  
> +

AHEM??
>      rc = libxl__prepare_dtb(gc, info, state, dom);
>      if (rc) goto out;
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel