Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Julien Grall <julien.grall@linaro.org>]

On 05/23/2014 06:33 PM, Stefano Stabellini wrote:
> On Thu, 22 May 2014, Julien Grall wrote:
>> On 22/05/14 18:45, Stefano Stabellini wrote:
>>> On Thu, 22 May 2014, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 22/05/14 13:32, Stefano Stabellini wrote:
>>>>> At the moment gic_remove_from_queues doesn't handle the case where the
>>>>> guest kernel disables an irq on a different vcpu compared to the one
>>>>> currently receiving the interrupt.
>>>>> Make sure to take the right vcpu lock before removing the irq from
>>>>> lr_queue.
>>>>
>>>> I see the same issue with vgic_enable_irqs. We may inject to the wrong
>>>> VCPU
>>>> (i.e other than 0).
>>>>
>>>> I think we should have the same case in vgic_enable_irqs.
>>>
>>> I think it would make more sense to print a warning in
>>> vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs.
>>
>> IHMO the warning is not enougth. We may screw your state machine.
> 
> That cannot happen: rank->itargets is actually unused at the moment.

itargets is not used, but nothing prevent a guest to enabled an IRQ on
VCPU1. This can inject the IRQ in VCPU1 while it's already injected in
VCPU0 (assuming the IRQ what disable for a little time).

> 
>> BTW, for your todo:
>>
>>> +    /* TODO: evict the irq from LRs */
>>
>> We should not evict the IRQ from LRs. The guest may disable the IRQ while he
>> is in the IRQ context (and before the IRQ has been EOI). If you drop the IRQs
>> from the LRs, this can result to a maintenance interrupt:
>>
>> "If the specified Interrupt does not exist in the
>> List registers, the GICH_HCR.EOIcount field is incremented, potentially
>> generating a maintenance interrupt."
> 
> It is still better than the alternative: having an LR busy for no reason.
> A maintenance interrupt would be harmless.

Our internal representation (in the status field, still inflight) won't
be update-to-date for IRQ. We either inject a spurious IRQ (if it's a
virtual IRQ), other set active & pending is physical IRQ (which is
invalid from the GIC specification).

Regards,

-- 
Julien Grall