[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.