Re: [Xen-devel] [PATCH v8 05/27] ARM: GICv3: forward pending LPIs to guests

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi Julien,

On 10/05/17 18:17, Julien Grall wrote:
> 
> 
> On 05/10/2017 06:14 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 10/05/17 12:07, Julien Grall wrote:
>>>
>>>
>>> On 05/10/2017 11:47 AM, Andre Przywara wrote:
>>>> Hi,
>>>
>>> Hi Andre,
>>>
>>>> On 12/04/17 11:44, Julien Grall wrote:
>>>>> On 12/04/17 01:44, Andre Przywara wrote:
>>>>>> +/* Retrieve the priority of an LPI from its struct pending_irq. */
>>>>>> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi)
>>>>>> +{
>>>>>> +    struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi);
>>>>>> +
>>>>>> +    if ( !p )
>>>>>> +        return GIC_PRI_IRQ;
>>>>>
>>>>> Why the check here? And why returning GIC_PRI_IRQ?
>>>>
>>>> Because you surely want to avoid dereferencing NULL?
>>>> I changed the return value to 0xff, which is the lowest priority.
>>>> Frankly I think we could just return anything, as we will stop handling
>>>> this LPI anyway a bit later in the code if p is NULL here.
>>>
>>> I agree that you want to prevent NULL. But we also want to avoid return
>>> fake value because there was a caller that didn't bother to check
>>> whether the interrupt is valid at first hand.
>>
>> Well, I changed the sequence in vgic_vcpu_inject_irq() back to be:
>>
>>     priority = vgic_get_virq_priority(v, virq);
>>
>>     spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>     n = irq_to_pending(v, virq);
>>
>> mostly to prevent the locking order (rank vs. VCPU lock) issue you
>> mentioned. We read the latest priority value upfront, but only use it
>> later if the pending_irq is valid. I don't see how this should create
>> problems. Eventually this will be solved properly by the pending_irq
>> lock.
>>
>>> If you ever have NULL here then there is a latent BUG in your code
>>> somewhere else.
>>
>> Not in this case.
> 
> Because of the locking issue? I know there are locking issue, but it
> does not mean we should introduce bad code just for workaround them for
> the time being...

No, because we now (as before) call vgic_get_virq_priority() without any
locks, so anything could happen. And in the spirit of checking *every*
irq_to_pending() call I'd rather protect this case properly (without
trading NULL pointer exceptions for ASSERTs).

Please look at the new code and tell me if you still don't like it.

Cheers,
Andre.

>>
>>> Ignoring the NULL and return a fake value is likely not
>>> the right solution for development.
>>>
>>> I can see two solutions for this:
>>>     - ASSERT(p)
>>>     - if ( !p )
>>>       {
>>>          ASSERT_UNREACHABLE();
>>>              return 0xff;
>>>       }
>>>
>>> The later would still return a dumb value but at least we would catch
>>> programming mistake during development.
>>
>> I think this solution asks for the ASSERT to trigger in corner cases: If
>> the LPI fired on the host, but got unmapped shortly afterwards. In this
>> case vgic_vcpu_inject_irq() can be reached with an invalid LPI number,
>> and we handle this properly when irq_to_pending() returns NULL.
>> But in this case get_priority() will be called with the same invalid
>> LPI, so should be able to cope with that as well.
>> Again this will eventually be solved properly with the per-IRQ lock.
> 
> See above. I still prefer to see the ASSERT firing time to time than bad
> code going in staging.
> 
> Cheers,
> 

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