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

[Julien Grall <julien.grall@arm.com>]

On 13/06/17 11:57, Bhupinder Thakur wrote:
> Hi Julien,

Hi Bhupinder,

>
>>>
>>>  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@arm.com>)
>>
>>> +
>>> +    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.

>> What if the other end of the ring has put more data whilst reading one
>> character?
> It will raise an event when the other end puts more data and in the
> event handling function data_available(), it will clear the RXFE bit.

And this is fine because the lock is here to protect uartfr/uartis I guess?

[...]

>>> +
>>> +    /* Map the guest PFN to Xen address space. */
>>> +    rc =  prepare_ring_for_helper(d,
>>> +                                  gfn_x(info->gfn),
>>> +                                  &vpl011->ring_page,
>>> +                                  &vpl011->ring_buf);
>>> +    if ( rc < 0 )
>>> +        goto out;
>>> +
>>> +    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>>> +    if ( !rc )
>>> +    {
>>> +        rc = -EINVAL;
>>> +        goto out1;
>>> +    }
>>> +
>>> +    register_mmio_handler(d, &vpl011_mmio_handler,
>>> +                          GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
>>
>>
>> Again, you register MMIO handler but never remove them. So if this call
>> fail, you will end up with the handlers existing but the rest
>> half-initialized which likely lead to a segfault, or worst leaking data.
> This function does not return a status. So there is no way to find out
> if the mmio handlers were
> registered successfully.

That's not my point. register_mmio_handler should never fail. However, 
the code below (alloc_unbount_...) may fail. And you bail

> I am removing the mmio handlers in the error
> legs in domain_vpl011_init().

Xen does not have any helper to revert the behavior of 
register_mmio_handler and I don't seem to introduce why. So how do you 
do it?

Anyway, you will not need to worry about removing MMIO handler if you 
move the call after all the call that may fail.

>>
>>> +
>>> +    spin_lock_init(&vpl011->lock);
>>> +
>>> +    rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
>>> +                                         vpl011_notification);
>>> +    if ( rc < 0 )
>>
>> You
> I think this is a type.

hmmm likely.

>
>>>
>>> +        goto out2;
>>> +
>>> +    vpl011->evtchn = info->evtchn = rc;
>>> +
>>> +    return 0;
>>> +
>>> +out2:
>>> +    xfree(d->arch.vmmio.handlers);
>>> +    vgic_free_virq(d, GUEST_VPL011_SPI);
>>> +
>>> +out1:
>>> +    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
>>> +
>>> +out:
>>> +    return rc;
>>> +}
>>> +
>>> +void domain_vpl011_deinit(struct domain *d)
>>> +{
>>> +    struct vpl011 *vpl011 = &d->arch.vpl011;
>>> +
>>> +    if ( !vpl011->ring_buf )
>>
>>
>> You will bail out if ring_buf is NULL. However, if you called
>> domain_vpl011_init first and it failed, you may have ring_buf set but the
>> rest not fully updated. This means that you will free garbagge.
>>
>> I think this could be solved by reinitialize ring_buf if an error occur in
>> domain_vpl011_init.
> destroy_ring_for_helper() sets the first parameter to NULL incase it fails.

Fine. I think it is a bit fragile, but I don't see why someone would 
decide to remove it without checking all the callers.

[...]

>>> +#ifdef CONFIG_VPL011_CONSOLE
>>> +int domain_vpl011_init(struct domain *d,
>>> +                       struct vpl011_init_info *info);
>>> +void domain_vpl011_deinit(struct domain *d);
>>> +#else
>>> +static inline int domain_vpl011_init(struct domain *d,
>>> +                                     struct vpl011_init_info *info)
>>> +{
>>> +    return -ENOSYS;
>>> +}
>>> +
>>> +static inline void domain_vpl011_deinit(struct domain *d) { }
>>> +#endif
>>> +
>>> +#endif
>>> +
>>
>>
>> Please drop this newline.
> You mean the newline between the #endifs or after the last #endif?

Yes.

Cheers,

-- 
Julien Grall

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