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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel