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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Sun, Mar 05, 2017 at 01:04:07PM +0000, Julien Grall wrote:
> Hi Bhupinder,
> 
> CC toolstack maintainers.
> 
> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
> > Add a new pl011 uart node
> >     - Get the pl011 spi virq from Xen using a hvm call
> 
> See my comment on previous patches.
> 
> >     - 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.
> 
> The ordering of the patches in this series is a bit weird. You are exposing
> the pl011 to the guest before all the code to handle it is there. This patch
> should likely be at the end of this series.
> 
> Also, you want a libxl option to allow the user enabling/disable pl011
> emulation. We likely want this emulation disable by default.
> 

Yes, I concur.

> > 
> > 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)
> 
> Why uint64_t for irq?
> 
> > +{
> > +    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;
> > +
> > +    set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> > +
> > +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> > +    if (res) return res;
> > +
> > +    fdt_property_u32(fdt, "current-speed", 115200);
> > +
> > +    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;

Spaces around "=" please.

> > 
> >      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) )
> > +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom, vpl011_irq) );
> > +

Coding style is wrong.

> >          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);
> > +

Stray blank line.

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

Ditto.

Please check libxl/CODING_STYLE and avoid unrelated changes.

Wei.

> >      rc = libxl__prepare_dtb(gc, info, state, dom);
> >      if (rc) goto out;
> > 
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall

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