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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,


>>>> +}
>>>> +
>>>> +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();
>

>>> 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.
>
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.

I will move the smp_mb() before index is incremented.

>>> 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?
Yes.

>
> [...]
>
>>>> +
>>>> +    /* 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.
>
I will move this call to the last.

>>>
>>>> +
>>>> +    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.
ok. I have removed the newline between the endifs.

Regards,
Bhupinder

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