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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 24 May 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 05/23/2017 07:23 PM, Stefano Stabellini wrote:
> > 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?
> 
> I don't think so.
> 
> > 
> > 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.
> 
> The problem we are trying to solve here is not because of NULL pending_irq
> structs. It is because of the previous interrupt may still be in LRs so we
> would end-up to have twice the LPIs in it. This will result to unpredictable
> behavior.
> 
> This could happen if you do:
> 
>     vCPU A               |   vCPU  B
>                          |
>     DISCARD vLPI1        |
>     MAPTI   vLPI1        |
> 
>                 interrupt injected on vCPU B
> 
>                          |   entering in the hyp
>                          |   clear_lrs
>                          |   vgic_vcpu_inject_irq
> 
> 
> clear_lrs will not remove the interrupt from LRs if it was already pending
> because pending_irq is not NULL anymore.
> 
> This is causing issue because we are trying to be clever with the LRs by not
> regenerating them at every entry/exit. This is causing trouble in many more
> place in the vGIC. IHMO we should attempt to regenerate them and see how this
> will affect the performance.

Yes but if pending_irq is not NULL, then it will be marked as PRISTINE,
so it is still recognizable. We can still "clean it up".


> > 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?
> 
> To be honest, I think we are trying to think about premature optimization
> without any number. We should first look at the vGIC rework and then see if
> this code will stay in place (I have the feeling it will disappear).
 
OK, no problem. Let's revisit this in the future.

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