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

[Stefano Stabellini <sstabellini@kernel.org>]

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

I don't like to rely on the barriers within spin_lock, which might or
might not change in future implementations, for PV protocols operations,
which need explicit barrier. But it is true the code would work today
from that perspective.

You are also right that with the current implementation vpl011_read_data
(and vpl011_write_data) could be called twice simultaneously. For that
to work correctly, we have to move the indexes reads after VPL011_LOCK.
In addition, we also need the explicit memory barrier

  smp_rmb()

after the indexes reads to be in sync with the other end. In other words:

  VPL011_LOCK(d, flags);

  in_cons = intf->in_cons;
  in_prod = intf->in_prod;

  smp_rmb();

  /* rest of the code */


The same for the other function.

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