Re: [Xen-devel] [PATCH v2 06/27] ARM: GICv3 ITS: introduce device mapping

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 03/04/17 21:41, Julien Grall wrote:
> 
> 
> On 04/03/2017 09:08 PM, Andre Przywara wrote:
>> Hi,
> 
> Hi Andre,
> 
>> On 22/03/17 17:29, Julien Grall wrote:
>>>> +int gicv3_its_map_guest_device(struct domain *d,
>>>> +                               paddr_t host_doorbell, uint32_t
>>>> host_devid,
>>>> +                               paddr_t guest_doorbell, uint32_t
>>>> guest_devid,
>>>> +                               uint32_t nr_events, bool valid)
>>>> +{
>>>> +    void *itt_addr = NULL;
>>>> +    struct host_its *hw_its;
>>>> +    struct its_devices *dev = NULL, *temp;
>>>> +    struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent
>>>> = NULL;
>>>> +    int ret = -ENOENT;
>>>> +
>>>> +    hw_its = gicv3_its_find_by_doorbell(host_doorbell);
>>>> +    if ( !hw_its )
>>>> +        return ret;
>>>> +
>>>> +    /* check for already existing mappings */
>>>> +    spin_lock(&d->arch.vgic.its_devices_lock);
>>>> +    while ( *new )
>>>> +    {
>>>> +        temp = rb_entry(*new, struct its_devices, rbnode);
>>>> +
>>>> +        parent = *new;
>>>> +        if ( !compare_its_guest_devices(temp, guest_doorbell,
>>>> guest_devid) )
>>>> +        {
>>>> +            if ( !valid )
>>>> +                rb_erase(&temp->rbnode, &d->arch.vgic.its_devices);
>>>> +
>>>> +            spin_unlock(&d->arch.vgic.its_devices_lock);
>>>> +
>>>> +            if ( valid )
>>>
>>> Again, a printk(XENLOG_GUEST...) here would be useful to know which host
>>> DeviceID was associated to the guest DeviceID.
>>
>> I added a gprintk(XENLOG_DEBUG, ), which I think is more appropriate (as
>> it may spam the console when some stupid guest is running). Let me know
>> if you want to have a different loglevel.
> 
> I don't think this is more appropriate. gprintk will print the domain ID
> of the current domain, whilst this function will be called by the
> toolstack in the future.
> 
> Furthemore, if you look at the implementation of gprintk you will notice
> that it is basically turning into printk(XENLOG_GUEST...) and adding
> information of the current vCPU.
> 
> What matters for ratelimiting is XENLOG_GUEST.
> 
>>
>>>> +                return -EBUSY;
>>>> +
>>>> +            return remove_mapped_guest_device(temp);
>>>
>>> Just above you removed the device from the RB-tree but this function may
>>> fail and never free the memory. This means that memory will be leaked
>>> leading to a potential denial of service.
>>
>> So I fixed this case in v4, though there is still a tiny chance of a
>> memleak: if the MAPD(V=0) command fails. We can't free the ITT table
>> then, really, because it still belongs to the ITS. I don't think we can
>> do much about it, though.
> 
> This is a leak and even tiny is quite worrying. How do you plan to
> address this in the future? What is the best thing to do?

In an ideal world we would probably have a spillover queue for ITS
commands. Whenever a command can't be queued to the hardware ITS queue
immediately, the guest should be put to wait somehow and the commands
recorded, to be executed whenever the hardware command queue gets free
slots again.
However I think we can't really do this today, because we don't have a
good way of putting a guest to sleep when it trapped on an MMIO access.
When in the future device assignment / de-assignment is handled via a
hypercall, we can probably address this more easily.

As for the likeliness: I think this is extremely rare. With our large
command queue and the ITS running at multiple hundred MHz I don't think
we will ever run into this situation, really, especially not with just Dom0.

So for now are we OK with this comment noting the corner case? Maybe a TODO?

Cheers,
Andre.

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