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

[Stefano Stabellini <sstabellini@kernel.org>]

On Fri, 23 Jun 2017, Julien Grall wrote:
> 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.

We discussed these barriers at length when I published the pvcalls and
xen 9pfs protocols, see for example
alpine.DEB.2.10.1612021318340.2777@sstabellini-ThinkPad-X260. Please
refer to "Ring Usage" in docs/misc/9pfs.markdown and "Workflow" in
docs/misc/pvcalls.markdown. I would like to keep them consistent across
protocols (the console protocol works exactly like pvcalls and xen 9pfs
in that respect).


> > > +    /*
> > > +     * 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