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, October 14, 2021 2:01 AM
> 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 12/10/2021 03:42, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: Monday, October 11, 2021 6:49 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 09/10/2021 09:47, Penny Zheng wrote:
> >>> Hi Julien
> >>
> >> Hi Penny,
> >>
> >>>> -----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?
> >>
> >> It is in fact the other way around. The problem will appear if the
> >> host UART MMIO region is smaller than the one we will emulate for the
> guest PL011.
> >>
> >
> > Sorry to keep bothering.
> > Is it that because when the UART MMIO region is smaller than the one
> > we emulated, register(DR, RSR, FR, ...) will not be at the place where we
> emulated?
> 
> Let me give an example to clarify my point. On some Hardware (IIRC pine64),
> the UART MMIO region is less than 4KB. In fact, there are multiple device
> within the same 4KB region.
> 
> At the moment, we are removing those devices because we can't assign to a
> domain a region that is not page aligned (4KB today). But I can see some
> benefits to be able to assign such devices to different domain/xen.
> To support them, we would need to trap the region and then forward only
> access to address the domain can access.
> 
> The PL011 we emulate for the guest require a 4KB region. So this would
> overlap with other device in the same region we may want to trap.
> 
> For is not an issue for the reasons I mentionned above. However, I think it 
> is a
> good idea to harden the code and add a check/comment when we know
> potential pitfalls.
> 

Understood, a thousand thanks for the detailed explanation! ;)

> Cheers,
> 
> --
> Julien Grall