Re: [Xen-devel] [PATCH 01/11] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall <julien.grall@xxxxxxx>]

Hi Bhupinder,

On 21/02/17 11:25, Bhupinder Thakur wrote:
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> new file mode 100644
> index 0000000..88ba968
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
[...]

> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, 
> void *priv)
See my comment on the vpl011_mmio_read about the structure of the function.

> +{
> +    unsigned char ch = r;
> +
> +    switch (info->gpa - GUEST_PL011_BASE)
> +    {
> +        case VPL011_UARTCR_OFFSET:
This register is not required in the SBSA UART.

> +            v->domain->arch.vpl011.control = r;
> +            break;
> +        case VPL011_UARTDR_OFFSET:
> +            vpl011_write_data(v->domain, ch);
The implicit casting of register_t to ch is a bit confusing. Also I 
would prefer to use uint8_t as it reflects the size of the field.
Lastly, what if vpl011_write_data is returning an error?

> +            break;
> +        case VPL011_UARTIMSC_OFFSET:
> +            v->domain->arch.vpl011.intr_mask = r;
You need to sanitize the value written and make sure reserved field and 
Write As Ones register and handle correctly (see the spec).
> +            if ( (v->domain->arch.vpl011.raw_intr_status & 
> v->domain->arch.vpl011.intr_mask) )
This line and the line below are over 80 columns. Also the code in 
general would benefits if you define a local variable to point to 
v->domain->arch.vpl011.
> +                vgic_vcpu_inject_spi(v->domain, 
> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
I don't think we need an HVM_PARAM for the pl011 SPI. We cannot hardcode 
this in the guest layout (see include/public/arch-arm.h).
> +            break;
> +        case VPL011_UARTICR_OFFSET:
> +            /*
> +             * clear all bits which are set in the input
> +             */
> +            v->domain->arch.vpl011.raw_intr_status &= ~r;
> +            if ( (v->domain->arch.vpl011.raw_intr_status & 
> v->domain->arch.vpl011.intr_mask) )
> +            {
{ } are not necessary.

> +                vgic_vcpu_inject_spi(v->domain, 
> (int)v->domain->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
> +            }
The if is exactly the same as the on in IMSC. This is the call of an 
helper to check the interrupt state and inject one if necessary.
> +            break;
> +        case VPL011_UARTRSR_OFFSET: // nothing to clear
The coding style for single line comment in Xen is /* ... */

Also, I think we may want to handle Overrun error as the ring may be full.

> +            break;
> +        case VPL011_UARTFR_OFFSET: // these are all RO registers
> +        case VPL011_UARTRIS_OFFSET:
> +        case VPL011_UARTMIS_OFFSET:
> +        case VPL011_UARTDMACR_OFFSET:
Please have a look at the vGICv2/vGICv3 emulation see how we implemented 
write ignore register.
> +            break;
> +        default:
> +            printk ("vpl011_mmio_write: switch case not handled %d\n", 
> (int)(info->gpa - GUEST_PL011_BASE));
See my comment about the prinkt in the read emulation.

> +            break;
> +    }
> +
> +    return VPL011_EMUL_OK;
> +}
> +
> +static const struct mmio_handler_ops vpl011_mmio_handler = {
> +    .read = vpl011_mmio_read,
> +    .write = vpl011_mmio_write,
> +};
> +
> +
> +
Only newline is sufficient. This was mention by Konrad and I will try to 
avoid repeating his comments.
> +int vpl011_map_guest_page(struct domain *d)
> +{
> +    int rc=0;
> +
> +    /*
> +     * map the guest PFN to Xen address space
> +     */
> +    rc = prepare_ring_for_helper(d,
> +                                 
> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_PFN],
> +                                 &d->arch.vpl011.ring_page,
> +                                 (void **)&d->arch.vpl011.ring_buf);
Why do you need the cast?

Also I cannot find any code in this series which destroy the ring. 
Please have a helper called vpl011_unmap_guest_page to do this job and 
call when the domain is destroyed.
> +    if ( rc < 0 )
> +    {
> +        printk("Failed to map vpl011 guest PFN\n");
> +    }
> +
> +    return rc;
> +}
> +
> +static int vpl011_data_avail(struct domain *d)
> +{
> +    int rc=0;
> +    unsigned long flags;
> +
> +    struct console_interface *intf=(struct console_interface 
> *)d->arch.vpl011.ring_buf;
> +
> +    VPL011_LOCK(d, flags);
> +
> +    /*`
> +     * check IN ring buffer
> +     */
> +    if ( !VPL011_IN_RING_EMPTY(intf) )
> +    {
> +        /*
> +         * clear the RX FIFO empty flag as the ring is not empty
> +         */
> +        d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFE);
> +
> +        /*
> +         * if the buffer is full then set the RX FIFO FULL flag
> +         */
> +        if ( VPL011_IN_RING_FULL(intf) )
> +            d->arch.vpl011.flag |= (VPL011_UARTFR_RXFF);
> +
> +        /*
> +         * set the RX interrupt status
> +         */
> +        d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_RXRIS);
> +    }
> +
> +    /*
> +     * check OUT ring buffer
> +     */
> +    if ( !VPL011_OUT_RING_FULL(intf) )
> +    {
> +        /*
> +         * if the buffer is not full then clear the TX FIFO full flag
> +         */
> +        d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFF);
> +
> +        /*
> +         * set the TX interrupt status
> +         */
> +        d->arch.vpl011.raw_intr_status |= (VPL011_UARTRIS_TXRIS);
> +
> +        if ( VPL011_OUT_RING_EMPTY(intf) )
> +        {
> +            /*
> +             * clear the uart busy flag and set the TX FIFO empty flag
> +             */
> +            d->arch.vpl011.flag &= ~(VPL011_UARTFR_BUSY);
> +            d->arch.vpl011.flag |= (VPL011_UARTFR_TXFE);
> +        }
> +    }
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    /*
> +     * send an interrupt if it is not masked
> +     */
> +    if ( (d->arch.vpl011.raw_intr_status & d->arch.vpl011.intr_mask) )
> +        vgic_vcpu_inject_spi(d, 
> (int)d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ]);
See my comment above about having an helper for this code.

> +
> +    if ( !VPL011_OUT_RING_EMPTY(intf) )
> +    {
> +        /*
> +         * raise an interrupt to dom0
The backend console does not necessarily live in DOM0. Anyway, Konrad 
requested to drop the comment and I am happy with that.
> +         */
> +        rc = raw_evtchn_send(d,
> +                    
> d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], NULL);
You are using a function that is been defined in a later patch (i.w #3). 
All the patch should be able to compile without applying upcoming patch 
to allow bisection.
Ideally, Xen should still be functional for every patch. But it is a 
best effort.
> +
> +        if ( rc < 0 )
> +            printk("Failed to send vpl011 interrupt to dom0\n");
> +    }
> +
> +    return rc;
> +}
> +
> +int vpl011_read_data(struct domain *d, unsigned char *data)
This function does not seem to be used outside of this file. So why do 
you export it?
> +{
> +    int rc=0;
> +    unsigned long flags;
> +    struct console_interface *intf=(struct console_interface 
> *)d->arch.vpl011.ring_buf;
> +
> +    *data = 0;
> +
> +    VPL011_LOCK(d, flags);
> +
> +    /*
> +     * if there is data in the ring buffer then copy it to the output buffer
> +     */
> +    if ( !VPL011_IN_RING_EMPTY(intf) )
> +    {
> +        *data = intf->in[MASK_VPL011CONS_IDX(intf->in_cons++, intf->in)];
> +    }
> +
> +    /*
> +     * if the ring buffer is empty then set the RX FIFO empty flag
> +     */
> +    if ( VPL011_IN_RING_EMPTY(intf) )
> +    {
> +        d->arch.vpl011.flag |= (VPL011_UARTFR_RXFE);
> +        d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_RXRIS);
> +    }
> +
> +    /*
> +     * clear the RX FIFO full flag
> +     */
> +    d->arch.vpl011.flag &= ~(VPL011_UARTFR_RXFF);
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    return rc;
> +}
> +
> +int vpl011_write_data(struct domain *d, unsigned char data)
Same has for vpl011_read_data.

> +{
> +    int rc=0;
> +    unsigned long flags;
> +    struct console_interface *intf=(struct console_interface 
> *)d->arch.vpl011.ring_buf;
> +
> +    VPL011_LOCK(d, flags);
> +
> +    /*
> +     * if there is space in the ring buffer then write the data
> +     */
> +    if ( !VPL011_OUT_RING_FULL(intf) )
> +    {
> +        intf->out[MASK_VPL011CONS_IDX(intf->out_prod++, intf->out)] = data;
> +        smp_wmb();
> +    }
> +
> +    /*
> +     * if there is no space in the ring buffer then set the
> +     * TX FIFO FULL flag
> +     */
> +    if ( VPL011_OUT_RING_FULL(intf) )
> +    {
> +        d->arch.vpl011.flag |= (VPL011_UARTFR_TXFF);
> +        d->arch.vpl011.raw_intr_status &= ~(VPL011_UARTRIS_TXRIS);
> +    }
> +
> +    /*
> +     * set the uart busy status
> +     */
> +    d->arch.vpl011.flag |= (VPL011_UARTFR_BUSY);
> +
> +    /*
> +     * clear the TX FIFO empty flag
> +     */
> +    d->arch.vpl011.flag &= ~(VPL011_UARTFR_TXFE);
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    /*
> +     * raise an event to dom0 only if it is the first character in the buffer
> +     */
> +    if ( VPL011_RING_DEPTH(intf, out) == 1 )
> +    {
> +        rc = raw_evtchn_send(d,
> +                d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN], 
> NULL);
> +
> +        if ( rc < 0 )
> +            printk("Failed to send vpl011 interrupt to dom0\n");
> +    }
> +
> +    return rc;
> +}
> +
> +static void vpl011_notification(struct vcpu *v, unsigned int port)
> +{
> +    vpl011_data_avail(v->domain);
vpl011_data_avail is returning an error but you don't check. What is the 
point of it then?
> +}
> +
> +int domain_vpl011_init(struct domain *d)
I was expected a destroy function to clean-up the vpl011 emulation.

> +{
> +    int rc=0;
> +
> +    /*
> +     * register xen event channel
> +     */
> +    rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id,
> +                                                        vpl011_notification);
This line looks wrong to me. The console backend may not live in the 
same domain as the toolstack.
> +    if (rc < 0)
> +    {
> +        printk ("Failed to allocate vpl011 event channel\n");
> +        return rc;
> +    }
> +    d->arch.hvm_domain.params[HVM_PARAM_VPL011_CONSOLE_EVTCHN] = rc;
> +
> +    /*
> +     * allocate an SPI VIRQ for the guest
> +     */
> +    d->arch.hvm_domain.params[HVM_PARAM_VPL011_VIRQ] = vgic_allocate_spi(d);
It makes little sense to hardcode the MMIO region and not the SPI. Also, 
I would rather avoid to introduce new HVM_PARAM when not necessary. So 
hardcoding the value looks more sensible to me.
> +
> +    /*
> +     * register mmio handler
> +     */
> +    register_mmio_handler (d, &vpl011_mmio_handler, GUEST_PL011_BASE, 
> GUEST_PL011_SIZE, NULL);
Coding style. No space between the function name and (.

> +
> +    /*
> +     * initialize the lock
> +     */
> +    spin_lock_init(&d->arch.vpl011.lock);
> +
> +    /*
> +     * clear the flag, control and interrupt status registers
> +     */
> +    d->arch.vpl011.control = 0;
> +    d->arch.vpl011.flag = 0;
> +    d->arch.vpl011.intr_mask = 0;
> +    d->arch.vpl011.intr_clear = 0;
> +    d->arch.vpl011.raw_intr_status = 0;
> +    d->arch.vpl011.masked_intr_status = 0;

The domain structure is already zeroed. So no need to 0 it again.

> +
> +    return 0;
> +}
Missing e-macs magic here.

> diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vpl011.h
> new file mode 100644
> index 0000000..f2c577f
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.h
Header should live in include.

[...]

> +#ifndef _VPL011_H_
> +
> +#define _VPL011_H_
> +
> +/*
> + * register offsets
> + */
We already define the PL011 register in include/asm-arm/pl011-uart.h. 
Please re-use them rather than re-inventing the wheel.
> +#define VPL011_UARTDR_OFFSET    0x0 // data register (RW)
> +#define VPL011_UARTRSR_OFFSET   0x4 // receive status and error clear register 
> (RW)
> +#define VPL011_UARTFR_OFFSET    0x18 // flag register (RO)
> +#define VPL011_UARTRIS_OFFSET   0x3c // raw interrupt status register (RO)
> +#define VPL011_UARTMIS_OFFSET   0x40 // masked interrupt status register (RO)
> +#define VPL011_UARTIMSC_OFFSET  0x38 // interrupt mask set/clear register (RW)
> +#define VPL011_UARTICR_OFFSET   0x44 // interrupt clear register (WO)
> +#define VPL011_UARTCR_OFFSET    0x30 // uart control register
> +#define VPL011_UARTDMACR_OFFSET 0x48 // uart dma control register

[...]

> +#define MASK_VPL011CONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1))
> +struct console_interface {
> +    char in[1024];
> +    char out[2048];
> +    uint32_t in_cons, in_prod;
> +    uint32_t out_cons, out_prod;
> +};
> +#endif
The communication between Xen and the console backend is based on the PV 
console ring. It would be easier to re-use it rather than recreating one.
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index f2ecbc4..7e2feac 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -237,4 +237,10 @@ config FAST_SYMBOL_LOOKUP
>           The only user of this is Live patching.
>
>           If unsure, say Y.
> +
> +config VPL011_CONSOLE
> +       bool "Emulated pl011 console support"
> +       default y
> +       ---help---
> +         Allows a guest to use pl011 UART as a console
>  endmenu
This should go in arch/arm/Kconfig

> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2d6fbb1..ff2403a 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -40,6 +40,7 @@ struct vtimer {
>          uint64_t cval;
>  };
>
> +
Spurious line.

>  struct arch_domain
>  {
>  #ifdef CONFIG_ARM_64
> @@ -131,6 +132,20 @@ struct arch_domain
>      struct {
>          uint8_t privileged_call_enabled : 1;
>      } monitor;
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> +    struct vpl011 {
> +        void *ring_buf;
> +        struct page_info *ring_page;
> +        uint32_t    flag;               /* flag register */
> +        uint32_t    control;            /* control register */
> +        uint32_t    intr_mask;          /* interrupt mask register*/
> +        uint32_t    intr_clear;         /* interrupt clear register */
> +        uint32_t    raw_intr_status;    /* raw interrupt status register */
> +        uint32_t    masked_intr_status; /* masked interrupt register */
> +        spinlock_t  lock;
> +    } vpl011;
> +#endif
I think the structure should be defined in vpl011.h.

>  }  __cacheline_aligned;
>
>  struct arch_vcpu
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..1d4548f 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -410,6 +410,10 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_ACPI_BASE 0x20000000ULL
>  #define GUEST_ACPI_SIZE 0x02000000ULL
>
> +/* PL011 mappings */
> +#define GUEST_PL011_BASE    0x22000000ULL
> +#define GUEST_PL011_SIZE    0x00001000ULL
> +
>  /*
>   * 16MB == 4096 pages reserved for guest to use as a region to map its
>   * grant table in.
> @@ -420,6 +424,7 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
>  #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
>
> +
Spurious line.

>  #define GUEST_RAM_BANKS   2
>
>  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
Cheers,

--
Julien Grall

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