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

[Julien Grall <julien.grall@xxxxxxx>]

On 13/06/2017 09:58, Bhupinder Thakur wrote:
> Hi Julien,
Hi Bhupinder,

> > > > +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.
> Is there a possibility of concurrent access since this function is
> executed as part of
> trap handling which will serialize access to this function?
There are no locking in the common trap handling. The emulation should 
take care of the locking because multiple vCPU can concurrently access 
the MMIO region.
[....]

> > > 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".
> I understand that xenconsole is currently using the event notification
> as an indication to read
> data from the ring buffer. For writing data, it keeps checking
> periodically if there is space in the
> ring buffer. If there is space then it writes more data.
You should not assume that xenconsoled is the only backend console. One 
could decide to implement its own.
Cheers,

--
Julien Grall

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