RE: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Thursday, September 23, 2021 7:14 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native
> UART address and IRQ number for vPL011
> 
> 
> 
> On 23/09/2021 08:11, Penny Zheng wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >
> > We always use a fix address to map the vPL011 to domains. The address
> > could be a problem for domains that are directly mapped.
> >
> > So, for domains that are directly mapped, reuse the address of the
> > physical UART on the platform to avoid potential clashes.
> >
> > Do the same for the virtual IRQ number: instead of always using
> > GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   xen/arch/arm/domain_build.c  | 34 +++++++++++++++++++++++++++-------
> >   xen/arch/arm/vpl011.c        | 34 +++++++++++++++++++++++++++-------
> >   xen/include/asm-arm/vpl011.h |  2 ++
> >   3 files changed, 56 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 120f1ae575..c92e510ae7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -30,6 +30,7 @@
> >
> >   #include <xen/irq.h>
> >   #include <xen/grant_table.h>
> > +#include <xen/serial.h>
> >
> >   static unsigned int __initdata opt_dom0_max_vcpus;
> >   integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -1942,8
> > +1943,11 @@ static int __init make_vpl011_uart_node(struct kernel_info
> *kinfo)
> >       gic_interrupt_t intr;
> >       __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
> >       __be32 *cells;
> > +    struct domain *d = kinfo->d;
> > +    char buf[27];
> >
> > -    res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
> > +    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
> >arch.vpl011.base_addr);
> > +    res = fdt_begin_node(fdt, buf);
> >       if ( res )
> >           return res;
> >
> > @@ -1953,14 +1957,14 @@ static int __init make_vpl011_uart_node(struct
> > kernel_info *kinfo)
> >
> >       cells = &reg[0];
> >       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> > -                       GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
> > +                       GUEST_ROOT_SIZE_CELLS,
> > + d->arch.vpl011.base_addr,
> >                          GUEST_PL011_SIZE);
> >
> >       res = fdt_property(fdt, "reg", reg, sizeof(reg));
> >       if ( res )
> >           return res;
> >
> > -    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> > +    set_interrupt(intr, d->arch.vpl011.virq, 0xf,
> > + DT_IRQ_TYPE_LEVEL_HIGH);
> >
> >       res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> >       if ( res )
> > @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct domain
> *d,
> >       else
> >           allocate_static_memory(d, &kinfo, node);
> >
> > +    /*
> > +     * Initialization before creating its device
> > +     * tree node in prepare_dtb_domU.
> > +     */
> 
> I think it would be better to explain *why* this needs to be done before.
> 
> > +    if ( kinfo.vpl011 )
> > +        rc = domain_vpl011_init(d, NULL);
> > +
> >       rc = prepare_dtb_domU(d, &kinfo);
> >       if ( rc < 0 )
> >           return rc;
> > @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain
> *d,
> >       if ( rc < 0 )
> >           return rc;
> >
> > -    if ( kinfo.vpl011 )
> > -        rc = domain_vpl011_init(d, NULL);
> > -
> >       return rc;
> >   }
> >
> > @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
> >
> >           if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> >           {
> > +            unsigned int vpl011_virq = GUEST_VPL011_SPI;
> 
> Coding style: Add a newline here.
> 
> >               d_cfg.arch.nr_spis = gic_number_lines() - 32;
> >
> > +            /*
> > +             * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
> > +             * set, in which case we'll try to match the hardware.
> > +             *
> > +             * Typically, d->arch.vpl011.virq has the vpl011 irq number
> > +             * but at this point of the boot sequence it is not
> > +             * initialized yet.
> > +             */
> > +            if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
> > +                vpl011_virq = serial_irq(SERHND_DTUART);
> 
> I think we should not continue if the domain is direct-mapped *and* the IRQ
> is not found. Otherwise, this will may just result to potential breakage if
> GUEST_VPL011_SPI happens to be used for an HW device.
> 
> > +
> >               /*
> >                * vpl011 uses one emulated SPI. If vpl011 is requested, make
> >                * sure that we allocate enough SPIs for it.
> >                */
> >               if ( dt_property_read_bool(node, "vpl011") )
> >                   d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> > -                                         GUEST_VPL011_SPI - 32 + 1);
> > +                                         vpl011_virq - 32 + 1);
> >           }
> >
> >           /*
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
> > 895f436cc4..10df25f098 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -29,6 +29,7 @@
> >   #include <xen/mm.h>
> >   #include <xen/sched.h>
> >   #include <xen/console.h>
> > +#include <xen/serial.h>
> >   #include <public/domctl.h>
> >   #include <public/io/console.h>
> >   #include <asm/pl011-uart.h>
> > @@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct
> domain *d)
> >        * status bit has been set since the last time.
> >        */
> >       if ( uartmis & ~vpl011->shadow_uartmis )
> > -        vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
> > +        vgic_inject_irq(d, NULL, vpl011->virq, true);
> >
> >       vpl011->shadow_uartmis = uartmis;
> >   #else
> > -    vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
> > +    vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
> >   #endif
> >   }
> >
> > @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
> >                               void *priv)
> >   {
> >       struct hsr_dabt dabt = info->dabt;
> > -    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> > +    uint32_t vpl011_reg = (uint32_t)(info->gpa -
> > +
> > + v->domain->arch.vpl011.base_addr);
> >       struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> >       struct domain *d = v->domain;
> >       unsigned long flags;
> > @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
> >                                void *priv)
> >   {
> >       struct hsr_dabt dabt = info->dabt;
> > -    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> > +    uint32_t vpl011_reg = (uint32_t)(info->gpa -
> > +
> > + v->domain->arch.vpl011.base_addr);
> >       struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> >       struct domain *d = v->domain;
> >       unsigned long flags;
> > @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d, struct
> vpl011_init_info *info)
> >   {
> >       int rc;
> >       struct vpl011 *vpl011 = &d->arch.vpl011;
> > +    const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
> >
> >       if ( vpl011->backend.dom.ring_buf )
> >           return -EINVAL;
> >
> > +    vpl011->base_addr = GUEST_PL011_BASE;
> > +    vpl011->virq = GUEST_VPL011_SPI;
> > +    if ( is_domain_direct_mapped(d) )
> > +    {
> > +        if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
> > +        {
> > +            vpl011->base_addr = uart->base_addr;
> > +            vpl011->virq = serial_irq(SERHND_DTUART);
> 
> This seems a bit pointless to call serial_irq() twice. How about add a field 
> in
> vuart_info to return the interrupt number?
> 
> > +        }
> > +        else
> > +            printk(XENLOG_ERR
> > +                   "Unable to reuse physical UART address and irq for 
> > vPL011.\n"
> > +                   "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
> > +                   vpl011->base_addr, vpl011->virq);
> > +    }
> > +
> >       /*
> >        * info is NULL when the backend is in Xen.
> >        * info is != NULL when the backend is in a domain.
> > @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct
> vpl011_init_info *info)
> >           }
> >       }
> >
> > -    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> > +    rc = vgic_reserve_virq(d, vpl011->virq);
> >       if ( !rc )
> >       {
> >           rc = -EINVAL;
> > @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d, struct
> vpl011_init_info *info)
> >       spin_lock_init(&vpl011->lock);
> >
> >       register_mmio_handler(d, &vpl011_mmio_handler,
> > -                          GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> > +                          vpl011->base_addr, GUEST_PL011_SIZE, NULL);
> 
> So you are making the assumpption that the UART region will be equal to (or
> bigger) than GUEST_PL011_SIZE. There are definitely UART out where the
> MMIO region is smaller than 4K.
> 

Sorry. I got a few confused here, since I am not very familiar with pl011/UART 
knowledge.

Problems will occur when UART region is bigger than GUEST_PL011_SIZE, since we
are only considering MMIO region of [vpl011->base_addr, vpl011->base_addr + 
GUEST_PL011_SIZE], right?

So I shall add the justification like
ASSERT(uart->size <= GUEST_PL011_SIZE);

> Although, I don't expect them to be passthrough today. So this is probably
> fine to assume that the next 4K is free. Can you add some justification in-
> code and in the commit message?
> 
> >
> >       return 0;
> >
> >   out2:
> > -    vgic_free_virq(d, GUEST_VPL011_SPI);
> > +    vgic_free_virq(d, vpl011->virq);
> >
> >   out1:
> >       if ( vpl011->backend_in_domain ) diff --git
> > a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h index
> > e6c7ab7381..c09abcd7a9 100644
> > --- a/xen/include/asm-arm/vpl011.h
> > +++ b/xen/include/asm-arm/vpl011.h
> > @@ -53,6 +53,8 @@ struct vpl011 {
> >       uint32_t    uarticr;        /* Interrupt clear register */
> >       uint32_t    uartris;        /* Raw interrupt status register */
> >       uint32_t    shadow_uartmis; /* shadow masked interrupt register */
> > +    paddr_t     base_addr;
> > +    unsigned int virq;
> >       spinlock_t  lock;
> >       evtchn_port_t evtchn;
> >   };
> >
> 
> Cheers,
> 
> --
> Julien Grall