[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.