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

[Julien Grall <julien.grall@arm.com>]

Hi,

On 07/06/17 00:02, Stefano Stabellini wrote:
> On Tue, 6 Jun 2017, Bhupinder Thakur wrote:
>> +static uint8_t vpl011_read_data(struct domain *d)
>> +{
>> +    unsigned long flags;
>> +    uint8_t data = 0;
>> +    struct vpl011 *vpl011 = &d->arch.vpl011;
>> +    struct xencons_interface *intf = vpl011->ring_buf;
>> +    XENCONS_RING_IDX in_cons = intf->in_cons;
>> +    XENCONS_RING_IDX in_prod = intf->in_prod;
>
> After reading the indexes, we always need barriers. In this case:
>
>   smp_rmb();

Well, there are already barrier with the spinlock. However, I am bit 
concern about reading those index without the lock taken. You can have 
concurrent call to vpl011_read_data happening and therefore the indexes 
may have changed when the lock will be taken.

>
>
>> +    VPL011_LOCK(d, flags);
>> +
>> +    /*
>> +     * It is expected that there will be data in the ring buffer when this
>> +     * function is called since the guest is expected to read the data register
>> +     * only if the TXFE flag is not set.
>> +     * If the guest still does read when TXFE bit is set then 0 will be returned.
>> +     */
>> +    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>> +    {
>> +        data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>> +        in_cons += 1;
>> +        intf->in_cons = in_cons;
>> +        smp_mb();
>> +    }
>> +    else
>> +    {
>> +        gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>> +    }
>> +
>> +    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
>> +    {
>> +        vpl011->uartfr |= RXFE;
>> +        vpl011->uartris &= ~RXI;
>> +    }
>> +    vpl011->uartfr &= ~RXFF;
>> +    VPL011_UNLOCK(d, flags);
>
> I am pretty sure that the PV console protocol requires us to notify the
> other end even on reads. We need to add a notify_via_xen_event_channel
> here, I think.

I would agree here. On the previous version, I asked Bhupinder to 
explain why it is necessary and he said: "On second thoughs, 
notification is not required".

>
>
>> +    return data;
>> +}
>> +
>> +static void vpl011_write_data(struct domain *d, uint8_t data)
>> +{
>> +    unsigned long flags;
>> +    struct vpl011 *vpl011 = &d->arch.vpl011;
>> +    struct xencons_interface *intf = vpl011->ring_buf;
>> +    XENCONS_RING_IDX out_cons = intf->out_cons;
>> +    XENCONS_RING_IDX out_prod = intf->out_prod;
>
>   smp_mb()

Same remark as above.

[...]

>> +static void vpl011_data_avail(struct domain *d)
>> +{
>> +    unsigned long flags;
>> +    struct vpl011 *vpl011 = &d->arch.vpl011;
>> +    struct xencons_interface *intf = vpl011->ring_buf;
>> +    XENCONS_RING_IDX in_cons = intf->in_cons;
>> +    XENCONS_RING_IDX in_prod = intf->in_prod;
>> +    XENCONS_RING_IDX out_cons = intf->out_cons;
>> +    XENCONS_RING_IDX out_prod = intf->out_prod;
>> +    XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
>
>   smb_mb()

Ditto.

Cheers,

-- 
Julien Grall

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