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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 13 Jun 2017, Julien Grall wrote:
> > > >  tools/console/daemon/io.c        |   2 +-
> > > >  xen/arch/arm/Kconfig             |   5 +
> > > >  xen/arch/arm/Makefile            |   1 +
> > > >  xen/arch/arm/vpl011.c            | 418
> > > > +++++++++++++++++++++++++++++++++++++++
> > > >  xen/include/asm-arm/domain.h     |   6 +
> > > >  xen/include/asm-arm/pl011-uart.h |   2 +
> > > >  xen/include/asm-arm/vpl011.h     |  74 +++++++
> > > >  xen/include/public/arch-arm.h    |   6 +
> > > >  xen/include/public/io/console.h  |   4 +
> > > 
> > > 
> > > This would require an ACK from Konrad. The addition would also need to be
> > > justified in the commit message. Although, you probably want to split this
> > > change in a separate patch.
> > I will send this change in a separate patch.
> > 
> > > 
> > > >  9 files changed, 517 insertions(+), 1 deletion(-)
> > > >  create mode 100644 xen/arch/arm/vpl011.c
> > > >  create mode 100644 xen/include/asm-arm/vpl011.h
> > > > 
> > > > diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> > > > index 7e6a886..947f13a 100644
> > > > --- a/tools/console/daemon/io.c
> > > > +++ b/tools/console/daemon/io.c
> > > 
> > > 
> > > Can you explain why you change the position of the include in io.c?
> > Since I am including ring.h in console.h, it needs string.h to be
> > included first.
> 
> This should be justified in the commit message.
> 
> [...]
> 
> > > > +}
> > > > +
> > > > +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.
> 
> > 
> > > 
> > > > +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> > > > +}
> > > > +
> > > > +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;
> > > 
> > > 
> > > See my answer on Stefano's e-mail regarding the barrier here.
> > > (<fa3e5003-5c7f-0886-d437-6b643347b4c5@xxxxxxx>)
> > > 
> > > > +
> > > > +    VPL011_LOCK(d, flags);
> > > > +
> > > > +    /*
> > > > +     * It is expected that there will be data in the ring buffer when
> > > > this
> > > > +     * function is called since the guest is expected to read the data
> > > > register
> > > > +     * only if the TXFE flag is not set.
> > > > +     * If the guest still does read when TXFE bit is set then 0 will be
> > > > returned.
> > > > +     */
> > > > +    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
> > > > +    {
> > > > +        data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
> > > > +        in_cons += 1;
> > > > +        intf->in_cons = in_cons;
> > > > +        smp_mb();
> > > 
> > > 
> > > I don't understand why you moved the barrier from between reading the data
> > > and intf->in_cons. You have to ensure the character is read before
> > > updating
> > > in_cons.
> > I thought that since these 3 statements are dependent on in_cons, they
> > would be executed in order due to data dependency.
> 
> 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.

Julien is right, I missed it in my review. The smp_mb() should be before
increasing intf->in_cons:

  smp_mb();
  intf->in_cons = in_cons;

Like you have done correctly in vpl011_write_data. The reason is that
you need to make sure that the read is complete before increasing the
index, because as soon as you do that the other end could overwrite your
data.

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