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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 23 May 2017, Julien Grall wrote:
> 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@xxxxxxx>
> > > > ---
> > > >  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.

Let me explain it a bit better, I think I didn't clarify it well enough.
Let me also premise, that this would be fine to do later, it doesn't
have to be part of this patch.

When I wrote MAPD above, I meant actually any commands that delete an
existing pending_irq - vLPI mapping. Specifically, DISCARD, and MAPD
when the 

    if ( !valid )
        /* Discard all events and remove pending LPIs. */
        its_unmap_device(its, devid);

code path is taken, which should not be the case at boot time, right?
Are there any other commands that remove a pending_irq - vLPI mapping
that I missed?

The idea is that we could add a VGIC_V3_LPIS_DISCARD flag to arch_vcpu.
VGIC_V3_LPIS_DISCARD is set on a DISCARD command, and on a MAPD (!valid)
command. If VGIC_V3_LPIS_DISCARD is not set, there is no need to scan
anything. If VGIC_V3_LPIS_DISCARD is set *and* we want to inject a
PRISTINE_IRQ, then we do the scanning.

When we do the scanning, we check all LRs for NULL pending_irq structs.
We clear them all in one go. Then we remove VGIC_V3_LPIS_DISCARD.

This way, we get all PRISTINE_LPI fast, except for the very first one
after a DISCARD or MAPD (!valid) command.

Does it make more sense now? What do you think?

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