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

[Andre Przywara <andre.przywara@xxxxxxx>]

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.

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

Cheers,
Andre.

>>
>>> AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the
>>> priority. Or else, you are in big trouble.
>>
>> That depends on where and when you call it, which I don't want to make
>> any assumptions about.
>> In my latest version the vgic_get_virq_priority() call now stays at the
>> very beginning of vgic_vcpu_inject_irq(), so at this point the LPI could
>> have been unmapped meanwhile.
>> Surely you will bail out handling this LPI later in the code if it
>> returns NULL here, but for the sake of this function I think we need the
>> check.
>> To me it looks a bit like having this abstraction here is a bit
>> pointless and complicates things, but well ....
> 
> Well, I am not against very defensive programming. I am more against
> returning a fake value that may impact the rest of the vGIC (not even
> mentioning the lack of comment explain why). After all, the priority is
> controlled by the guest and not the hypervisor.
> 
> I suggested few way above to catch those errors.
> 
> Cheers,
> 

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