Re: [PATCH 03/12 v3] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Bhupinder Thakur <bhupinder.thakur@linaro.org>]

Hi Julien,

Thanks for your comments.

>
>
>> +static bool vpl011_reg32_check_access(int size)
>
>
> Please pass hsr_dabt in parameter rather than the size directly. Which BTW
> should have really be unsigned int.
>
ok.

>> +{
>> +    return (size == DABT_DOUBLE_WORD)? false : true;
>
>
> This could be simplified with (size != DABT_DOUBLE_WORD). Also please add a
> comment explaining why we allow all the sizes but 64-bit.
>
ok.

>> +}
>> +
>> +static void vpl011_update_spi(struct domain *d)
>
>
> Please rename this function to vpl011_update.
>
ok.
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    if ( vpl011->uartris & vpl011->uartimsc )
>
>
> Likely you want a todo to say we need to revisit that when the vGIC is
> handling properly level interrupt.
>
ok.

>> +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>> +}
>> +
>> +static void vpl011_read_data(struct domain *d, uint8_t *data)
>
>
> If a pointer is passed to be filled, then I would expect an error value to
> be returned.
>
> I would usually pointer if the function has to fill a structure or it is not
> possible to use the return.
>
> In this case, the value is 8-bit and you don't have return value. So return
> the value rather than passing a pointer.
>
I will return the data.


>
>> +        *data = intf->in[VPL011_RING_IDX_MASK(in_cons, intf->in)];
>> +        smp_mb();
>> +        intf->in_cons = in_cons + 1;
>> +    }
>
>
> For debugging purpose, it would be good to have a message if the ring is
> empty.
ok.

>
>> +
>> +    if ( VPL011_IN_RING_EMPTY(intf) )
>> +    {
>> +        vpl011->uartfr |= RXFE;
>> +        vpl011->uartris &= ~RXI;
>> +    }
>> +    vpl011->uartfr &= ~RXFF;
>> +    VPL011_UNLOCK(d, flags);
>> +
>> +    notify_via_xen_event_channel(d, vpl011->evtchn);
>
>
> It would be good to explain why you need to notification here.
>
On second thoughts, notification is not required here.


>
>> +        intf->out[VPL011_RING_IDX_MASK(out_prod, intf->out)] = data;
>> +        smp_wmb();
>> +        intf->out_prod = out_prod + 1;
>> +    }
>
>
> For debugging purpose, it would be useful to get a message if the ring is
> full.
>
ok.
>> +
>> +    if ( VPL011_OUT_RING_FULL(intf) )
>> +    {
>> +        vpl011->uartfr |= TXFF;
>> +        vpl011->uartris &= ~TXI;
>> +    }
>> +
>> +    vpl011->uartfr |= BUSY;
>
>
> I am not sure to understand why you set the bit BUSY here.

Since the OUT ring buffer is non-empty here, this bit is set to
indicate that there is data in the ring buffer (which is emulating a
UART transmit FIFO). As per pl011 TRM, this bit should be set if there
is data in the transmit fifo.

>
>> +
>> +    vpl011->uartfr &= ~TXFE;
>> +
>> +    VPL011_UNLOCK(d, flags);
>> +
>> +    notify_via_xen_event_channel(d, vpl011->evtchn);
>
>
> Same as the previous notify_*.
>
ok. I will add a comment on why we are sending an event here.

>> +}
>> +
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
>> *r, void *priv)
>> +{
>> +    uint8_t ch;
>
>
> This could be restricted to the case DR.
>
ok.

>> +    struct hsr_dabt dabt = info->dabt;
>> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>
>
> Please use either unsigned int or uint32_t. Something would be really wrong
> if the offset is negative here.
ok.
>
>> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
>
>
> Please add a comment why the check is the same for all registers.
>
ok.

>> +
>> +    switch ( vpl011_reg )
>> +    {
>> +    case DR:
>
>
> As mentioned above, you could do:
>
> {
>      uint8_t ch;
>      ....
> }
>
>> +        vpl011_read_data(v->domain, &ch);
>> +        *r = ch;
>
>
> Please use vreg_reg32_extract(...).
>
ok.

>> +        break;
>
>
> I admit I would prefer the "return 1;" here rather than "break;". This makes
> easier to follow the emulation for a given register.
>
> I would even be in favor of duplicating the "if ( !vpl011... )" in each case
> for the same reason.
Do you mean that I should repeat the vpl011_reg32_check_access() and
return for each switch case?

>
>> +
>> +    case RSR:
>> +        /* It always returns 0 as there are no physical errors. */
>> +        *r = 0;
>
>
> Note that I am ok to keep the 0 like that here.
>
>> +        break;
>> +
>> +    case FR:
>> +        *r = vreg_reg32_extract(vpl011->uartfr, info);
>> +        break;
>> +
>> +    case RIS:
>> +        *r = vreg_reg32_extract(vpl011->uartris, info);
>> +        break;
>> +
>> +    case MIS:
>> +        *r = vreg_reg32_extract(vpl011->uartris
>> +                                & vpl011->uartimsc, info);
>
>
> The coding style in Xen is to split after the &. E.g
>
> vpl011->uartris &
> vpl011->uartimsc
>
ok.

> Also, none of the pl011 register emulation is using any locking. This is
> quite surprising to me. So can you explain why?

In vpl011_mmio_read(), since pl011 registers are only read, it should
be ok to let
them be read without any locking.
>
>
>> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info,
>> register_t r, void *priv)
>> +{
>> +    uint8_t ch = ((struct uartdr_reg *)&r)->data;
>
>
> This should be restricted to the DR case below. But I am not sure to
> understand the purpose the structure here. It makes less readable and also
> you only use it for the write case.
There was a review comment earlier to make the typecasting more
readable because earlier I was doing r & 0xff to retrieve the data. So
I added a structure which tells the layout of the DR. I guess I can
use vreg_reg32_extract() to extract uint8_t value from register_t r.

>
>> +    struct hsr_dabt dabt = info->dabt;
>> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>
>
> Same as above for vpl011_reg.
ok.
>
>> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
>
>
> Same as the previous vpl011_reg32_check_access... and all of my remarks in
> the read emulation stand for the write one.
>
>> +
>> +    switch ( vpl011_reg )
>> +    {
>> +    case DR:
>> +        vpl011_write_data(v->domain, ch);
>> +        break;
>> +
>> +    case RSR: /* Nothing to clear. */
>> +        break;
>> +
>> +    case FR:
>> +    case RIS:
>> +    case MIS:
>> +        goto write_ignore;
>> +
>> +    case IMSC:
>> +        vreg_reg32_update(&vpl011->uartimsc, r, info);
>> +        vpl011_update_spi(v->domain);
>
>
> I would have expected some locking here.
Yes here lock should be taken.

>
>> +        break;
>> +
>> +    case ICR:
>> +        vreg_reg32_clearbits(&vpl011->uartris, r, info);
>> +        vpl011_update_spi(v->domain);
>
>
> Same here.
Yes here lock should be taken.
>
>
>> +        break;
>> +
>> +    default:
>> +        gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
>> +                dabt.reg, vpl011_reg);
>> +        return 0;
>> +    }
>> +
>> +write_ignore:
>> +    return 1;
>> +
>> +bad_width:
>> +    gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
>> +            dabt.size, dabt.reg, vpl011_reg);
>> +    domain_crash_synchronous();
>> +    return 0;
>> +
>> +}
>> +
>> +static const struct mmio_handler_ops vpl011_mmio_handler = {
>> +    .read = vpl011_mmio_read,
>> +    .write = vpl011_mmio_write,
>> +};
>> +
>> +int vpl011_map_guest_page(struct domain *d, xen_pfn_t gfn)
>
>
> This function should either have the prototype defined in an header if used
> outside or static.
>
> Also, I have asked to use gfn_t and not xen_pfn_t. The former is a typesafe
> avoiding mix between MFN and GFN.
>
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    /* Map the guest PFN to Xen address space. */
>> +    return prepare_ring_for_helper(d,
>> +                                   gfn,
>> +                                   &vpl011->ring_page,
>> +                                   &vpl011->ring_buf);
>
>
> Looking at the usage it is only used in domain_vpl011_init. So I am not
> convinced of the of usefulness of this wrapper.
>
Yes. I will remove the wrapper function.

>> +}
>> +
>> +static void vpl011_data_avail(struct domain *d)
>> +{
>> +    unsigned long flags;
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +    struct xencons_interface *intf = vpl011->ring_buf;
>> +    RING_IDX in_ring_depth, out_ring_depth;
>
>
> I am a bit confused with the term "depth". It does not seem to fit for a
> ring.
I will use in_ring_qsize and out_ring_qsize instead.

>
>> +
>> +    VPL011_LOCK(d, flags);
>> +
>> +    in_ring_depth = vpl011_queued(intf->in_prod, intf->in_cons,
>> sizeof(intf->in));
>
>
> You introduced a macro VPL011_RING_DEPTH for hiding vpl011_queued, but don't
> use it.
I have removed this macro as much of ring buffer manipultation is
handled through vpl011_queued()
and vpl011_mask().

>
>> +    out_ring_depth = vpl011_queued(intf->out_prod, intf->out_cons,
>> sizeof(intf->out));
>
>
> Ditto.
>
>
>> +
>> +    /* Update the uart rx state if the buffer is not empty. */
>> +    if ( in_ring_depth != 0 )
>> +    {
>> +        vpl011->uartfr &= ~RXFE;
>> +        if ( in_ring_depth == VPL011_RING_MAX_DEPTH(intf, in) )
>> +            vpl011->uartfr |= RXFF;
>> +        vpl011->uartris |= RXI;
>> +    }
>> +
>> +    /* Update the uart tx state if the buffer is not full. */
>> +    if ( out_ring_depth != VPL011_RING_MAX_DEPTH(intf, out) )
>> +    {
>> +        vpl011->uartfr &= ~TXFF;
>> +        vpl011->uartris |= TXI;
>> +        if ( out_ring_depth == 0 )
>> +        {
>> +            vpl011->uartfr &= ~BUSY;
>> +            vpl011->uartfr |= TXFE;
>> +        }
>> +    }
>> +
>> +    VPL011_UNLOCK(d, flags);
>> +
>> +    vpl011_update_spi(d);
>> +}
>> +
>> +
>> +static void vpl011_notification(struct vcpu *v, unsigned int port)
>> +{
>> +    vpl011_data_avail(v->domain);
>> +}
>> +
>> +int domain_vpl011_init(struct domain *d,
>> +                       uint32_t console_domid,
>> +                       xen_pfn_t gfn,
>
>
> This should be gfn_t and not xen_pfn_t.
>
>> +                       evtchn_port_t *evtchn)
>> +{
>> +    int rc;
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>
>
> What if domain_vpl011_init is called twice? After nothing seem to prevent
> the new DOMCTL to be called twice.
>
I will add a check in the init function to return without doing
anything if vpl011 is already initialized.

>> +    rc = vpl011_map_guest_page(d, gfn);
>> +    if ( rc < 0 )
>> +        goto out;
>> +
>> +    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>> +    if ( !rc )
>> +    {
>> +        rc = -EINVAL;
>> +        goto out1;
>> +    }
>> +
>> +    register_mmio_handler(d, &vpl011_mmio_handler,
>> +                          GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
>
>
> You register MMIO handler but never remove them. So if this call fail, you
> will end up with the handlers existing but the rest half-initialized which
> likely lead to a segfault, or worst leaking data.
>
I will free the mmio handlers in the vpl011 deinit function.

>> +
>> +    spin_lock_init(&vpl011->lock);
>> +
>> +    rc = alloc_unbound_xen_event_channel(d, 0, console_domid,
>> +                                         vpl011_notification);
>> +    if (rc < 0)
>
>
> Coding style:
>
> if (  ... )
>
ok.
>> +        goto out2;
>> +
>> +    vpl011->evtchn = *evtchn = rc;
>> +
>> +    vpl011->initialized = true;
>
>
> I am not sure to understand the purpose of this variable. You only use it in
> domain_vpl011_deinit. But you can easily know if you need to free by looking
> at the state of ring_buf.
It is an extraneous variable. I will remove it.

>
>> +
>> +    return 0;
>> +
>> +out2:
>> +    xfree(d->arch.vmmio.handlers);
>> +out1:
>
>
> I would have expected vgic_free_virq to be called in case of error.
>
ok.

>> +    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
>> +out:
>> +    return rc;
>> +}
>> +
>> +void domain_vpl011_deinit(struct domain *d)
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    if ( vpl011->initialized )
>
>
> See my remark about vpl011->initialized above.
Yes, I will use the if ( vpl011->ring_buf ) check instead.
>
> In any case, I would prefer the condition to be inverted to avoid an extra
> layer of tab. I.e
>
> if ( !vpl011->initialized )
>    return;
>
ok.
>
>> +#ifndef _VPL011_H_
>> +
>> +#define _VPL011_H_
>> +
>> +#include <public/io/ring.h>
>> +#include <asm-arm/vreg.h>
>> +
>> +DEFINE_XEN_FLEX_RING(vpl011);
>
>
> I am sure someone already said it in a previous version. The vpl011 is the
> console ring. So why are we defining our own internally?
>
> At least this should have been used by xenconsole, but this is not the
> case... So we should really avoid defining our own ring here and re-use
> public/io/console.h.
>
>
>> +
>> +struct uartdr_reg {
>> +    uint8_t data;
>> +    uint8_t error_status:4;
>> +    uint8_t reserved1:4;
>> +    uint16_t reserved2;
>> +    uint32_t reserved3;
>> +};
>
>
> I don't see any benefits of this structure.
I will remove this structure and use the register value directly after
masking it.
>
>> +
>> +struct vpl011_s {
>
>
> Please remove the _s in the name.
ok.
>
>> +    void *ring_buf;
>> +    struct page_info *ring_page;
>> +    uint32_t    uartfr;     /* Flag register */
>> +    uint32_t    uartcr;     /* Control register */
>> +    uint32_t    uartimsc;   /* Interrupt mask register*/
>> +    uint32_t    uarticr;    /* Interrupt clear register */
>> +    uint32_t    uartris;    /* Raw interrupt status register */
>> +    uint32_t    uartmis;    /* Masked interrupt register */
>> +    spinlock_t  lock;
>> +    evtchn_port_t evtchn;
>> +    bool        initialized; /* Flag which tells whether vpl011 is
>> initialized */
>> +};
>> +
>> +#ifdef CONFIG_VPL011_CONSOLE
>> +int domain_vpl011_init(struct domain *d,
>> +                       uint32_t console_domid,
>> +                       xen_pfn_t gfn,
>> +                       evtchn_port_t *evtchn);
>> +void domain_vpl011_deinit(struct domain *d);
>> +#else
>> +int domain_vpl011_init(struct domain *d,
>
>
> static inline here.
ok.
>
>> +                       uint32_t console_domid,
>> +                       xen_pfn_t gfn,
>> +                       evtchn_port_t *evtchn) { return -ENOSYS; }
>
>
> Please indent correctly { return -ENOSYS };
>
ok.
>> +
>> +static inline void domain_vpl011_deinit(struct domain *d) { }
>
>
> Note that I am ok with the { } on the same line here.
>
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index bd974fb..5f91207 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -322,6 +322,8 @@ struct xen_arch_domainconfig {
>>       *
>>       */
>>      uint32_t clock_frequency;
>> +
>> +    uint32_t console_domid;
>
>
> Why this is here?
I will move it out of here.

Regards,
Bhupinder

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