Re: [PATCH 04/17 v5] xen/arm: vpl011: Add SBSA UART emulation in Xen

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

Hi Stefano,

On 22/06/17 23:53, Stefano Stabellini wrote:
> On Thu, 22 Jun 2017, Bhupinder Thakur wrote:
>> +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, out_prod;
>> +
>> +    VPL011_LOCK(d, flags);
>> +
>> +    out_cons = intf->out_cons;
>> +    out_prod = intf->out_prod;
>> +
>> +    smp_rmb();
>
> This should be
>        smp_mb();

To speed up discussion, it would have been nice to give a bit more 
details why you think smp_rmb() is not enough.

In this case, I think smp_rmb() is fine because all the write we care 
depends on out_cons and out_prod. So the processor cannot re-order it.

>> +    /*
>> +     * It is expected that the ring is not full when this function is called
>> +     * as the guest is expected to write to the data register only when the
>> +     * TXFF flag is not set.
>> +     * In case the guest does write even when the TXFF flag is set then the
>> +     * data will be silently dropped.
>> +     */
>> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>> +         sizeof (intf->out) )
>> +    {
>> +        intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
>> +        out_prod += 1;
>> +        smp_wmb();
>> +        intf->out_prod = out_prod;
>> +    }
>> +    else
>> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>> +
>> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
>> +         sizeof (intf->out) )
>> +    {
>> +        vpl011->uartfr |= TXFF;
>> +        vpl011->uartris &= ~TXI;
>> +    }
>> +
>> +    vpl011->uartfr |= BUSY;
>> +
>> +    vpl011->uartfr &= ~TXFE;
>> +
>> +    vpl011_update_interrupt_status(d);
>> +
>> +    VPL011_UNLOCK(d, flags);
>> +
>> +    /*
>> +     * Send an event to console backend to indicate that there is
>> +     * data in the OUT ring buffer.
>> +     */
>> +    notify_via_xen_event_channel(d, vpl011->evtchn);
>> +}

Cheers,

-- 
Julien Grall

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