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

[Julien Grall <julien.grall@xxxxxxx>]

On 06/14/2017 06:47 AM, Bhupinder Thakur wrote:
> Hi Julien,
Hi Bhupinder,

> > > > > +}
> > > > > +
> > > > > +static void vpl011_update(struct domain *d)
> > > > > +{
> > > > > +    struct vpl011 *vpl011 = &d->arch.vpl011;
> > > > > +
> > > > > +    /*
> > > > > +     * TODO: PL011 interrupts are level triggered which means
> > > > > +     * that interrupt needs to be set/clear instead of being
> > > > > +     * injected. However, currently vGIC does not handle level
> > > > > +     * triggered interrupts properly. This function needs to be
> > > > > +     * revisited once vGIC starts handling level triggered
> > > > > +     * interrupts.
> > > > > +     */
> > > > > +    if ( vpl011->uartris & vpl011->uartimsc )
> > > >
> > > >
> > > > The write in uartirs and uartimsc are protected by a lock. Shouldn't it
> > > > be
> > > > the case here too? More that they are not updated atomically.
> > > >
> > > > You probably want to call vpl011_update with vpl011 lock taken to make
> > > > sure
> > > > you don't have any synchronization issue.
> > >
> > > Since we are just reading the values here, I think it is fine to not
> > > take a lock. The
> > > condition will either be true or false.
> >
> > uartris and uartimsc may not be updated atomically because vreg_reg32_update
> > does not guarantee it.
> >
> > So you may read a wrong value here and potentially inject spurious
> > interrupt. This will get much worse when level will fully be supported as
> > you may switch the level of the interrupt by mistake.
> Ok. I will check this condition under lock. But should I call
> vgic_vcpu_inject_spi() also under the same lock? I think we can do
> like this:
>
> vpl011_lock();
> mask =  vpl011->uartris & vpl011->uartimsc;
> vpl011_unlock();
>
> if ( mask )
>     vgic_vcpu_inject_spi();
This is not going to work the day we rework the vGIC to fully support 
level interrupt. If you don't protect vgic_vcpu_inject_spi(), you may 
lower the level by mistake.
As mentioned on a previous mail, I would prefer if vpl011_update is 
called with the lock taken.
[...]

> > How do you know the compiler will generate assembly which contain the data
> > dependency?
> >
> > Likely it will not be the case because in_cons will be used indirectly as we
> > mask it first.
> >
> > > The memory barrier
> > > after the 3 statements ensures that intf->in_cons is updated before
> > > proceeding ahead.
> >
> > Can you explain why?
> >
> > IHMO, what matter here is in_cons to be written after intf->in[...] is read.
> > Otherwise the backend may see in_cons before the character has effectively
> > been read.
> ok. The issue is that the other end (xenconsole running on maybe some
> other CPU) may see the increment operation first before the read
> operation could complete (due to some quirk of memory/cache
> architecture) even though the CPU running the mmio_read() will see the
> effect in the correct order only.
It is not about quirk of memory/cache but data access ordering at the 
processor level. A processor is free to re-order the access if it is 
more efficient. That's why you have the mb/smp_mb barrier to tell the 
processor to no do that.
Cheers,

--
Julien Grall

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