Re: [PATCH v8 23/27] ARM: vITS: handle INV command

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

On 10/05/17 16:11, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 12/04/17 18:20, Julien Grall wrote:
>> On 12/04/17 01:44, Andre Przywara wrote:
>>
>>> +{
>>> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
>>
>> The locking is likely to wrong here too (see patch #2). For instance
>> with a MOVI then INV on interrupt enabled.
>>
>>> +
>>> +    if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>>> +    {
>>> +        if ( !list_empty(&p->inflight) &&
>>> +             !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>>> +            gic_raise_guest_irq(v, vlpi, p->lpi_priority);
>>> +    }
>>> +    else
>>> +    {
>>> +        clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>>> +        list_del_init(&p->lr_queue);
>>> +    }
>>> +}
>>> +
>>> +static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr)
>>> +{
>>> +    struct domain *d = its->d;
>>> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
>>> +    uint32_t eventid = its_cmd_get_id(cmdptr);
>>> +    struct pending_irq *p;
>>> +    unsigned long flags;
>>> +    struct vcpu *vcpu;
>>> +    uint32_t vlpi;
>>> +    int ret = -1;
>>> +
>>> +    /* Translate the event into a vCPU/vLPI pair. */
>>> +    if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) )
>>> +        return -1;
>>> +
>>> +    if ( vlpi == INVALID_LPI )
>>> +        return -1;
>>> +
>>> +    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
>>> +
>>> +    p = d->arch.vgic.handler->lpi_to_pending(d, vlpi);
>>> +    if ( !p )
>>> +        goto out_unlock;
>>
>> As said on v5, this could be simpler and use the pending_irqs in the
>> device. That would be an improvement though. So a would be good.
>
> Originally I found it more straight-forward to use the one existing
> interface (the rbtree) we also use in the VGIC part, which would allow
> us to handle locking or ref-counting in one central place.
> But indeed the ITS command handling has all the data we need to find the
> pending_irq directly from the virtual device.
> So I replaced all lpi_to_pending() calls in those handlers with a new
> function gicv3_its_get_event_pending_irq(), which looks up the struct
> from an ITS/device/event triple.
> I take and keep the its->lock for the runtime of these functions, so
> those events and their memory will not vanish meanwhile.
>
> Does that make sense?

It makes sense to keep the ref-counting in one central place. But it is 
better to avoid reading guest memory and therefore avoid most of 
checking and overhead to translate the IPA to PA.

That's why I suggested to use pending_irqs :).

Cheers,

-- 
Julien Grall

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