Re: [PATCH v9 20/28] ARM: GICv3: handle unmapped LPIs

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

Hi Stefano,

On 23/05/17 00:48, Stefano Stabellini wrote:
> On Fri, 19 May 2017, Stefano Stabellini wrote:
>> On Thu, 11 May 2017, Andre Przywara wrote:
>>> When LPIs get unmapped by a guest, they might still be in some LR of
>>> some VCPU. Nevertheless we remove the corresponding pending_irq
>>> (possibly freeing it), and detect this case (irq_to_pending() returns
>>> NULL) when the LR gets cleaned up later.
>>> However a *new* LPI may get mapped with the same number while the old
>>> LPI is *still* in some LR. To avoid getting the wrong state, we mark
>>> every newly mapped LPI as PRISTINE, which means: has never been in an
>>> LR before. If we detect the LPI in an LR anyway, it must have been an
>>> older one, which we can simply retire.
>>> Before inserting such a PRISTINE LPI into an LR, we must make sure that
>>> it's not already in another LR, as the architecture forbids two
>>> interrupts with the same virtual IRQ number on one CPU.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  xen/arch/arm/gic.c         | 55 +++++++++++++++++++++++++++++++++++++++++-----
>>>  xen/include/asm-arm/vgic.h |  6 +++++
>>>  2 files changed, 56 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index fd3fa05..8bf0578 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -375,6 +375,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
>>>  {
>>>      ASSERT(!local_irq_is_enabled());
>>>
>>> +    clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
>>> +
>>>      gic_hw_ops->update_lr(lr, p, state);
>>>
>>>      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>>> @@ -442,12 +444,41 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
>>>  #endif
>>>  }
>>>
>>> +/*
>>> + * Find an unused LR to insert an IRQ into. If this new interrupt is a
>>> + * PRISTINE LPI, scan the other LRs to avoid inserting the same IRQ twice.
>>> + */
>>> +static int gic_find_unused_lr(struct vcpu *v, struct pending_irq *p, int lr)
>>> +{
>>> +    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>>> +    unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
>>> +    struct gic_lr lr_val;
>>> +
>>> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
>>> +
>>> +    if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
>>
>> Maybe we should add an "unlikely".
>>
>> I can see how this would be OKish at runtime, but at boot time there
>> might be a bunch of PRISTINE_LPIs, but no MAPDs have been issued yet,
>> right?

You cannot have any PRISTINE_LPIs without any MAPDs done. This bit will 
be set when you do the first MAPTI.

>>
>> I have a suggestion, I'll leave it to you and Julien if you want to do
>> this now, or maybe consider it as a TODO item. I am OK either way (I
>> don't want to delay the ITS any longer).
>>
>> I am thinking we should do this scanning only after at least one MAPD
>> has been issued for a given cpu at least once. I would resurrect the
>> idea of a DISCARD flag, but not on the pending_irq, that I believe it's
>> difficult to handle, but a single global DISCARD flag per struct vcpu.
>>
>> On MAPD, we set DISCARD for the target vcpu of the LPI we are dropping.
>> Next time we want to inject a PRISTINE_IRQ on that cpu interface, we
>> scan all LRs for interrupts with a NULL pending_irq. We remove those
>> from LRs, then we remove the DISCARD flag.
>>
>> Do you think it would work?

I don't understand the point to do that. Ok, you will get the first 
PRISTINE_LPI "fast" (though likely LRs will be empty), but all the other 
will be "slow" (though likely LRs will be empty too).

The pain to implement your suggestion does not seem to be worth it so far.

Cheers,

-- 
Julien Grall

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