Re: [PATCH v2] xen/arm: implement GICD_I[S/C]ACTIVER reads

[Julien Grall <julien@xxxxxxx>]

On 09/04/2020 02:26, Stefano Stabellini wrote:
> On Tue, 7 Apr 2020, Julien Grall wrote:
> > > > > I don’t know what maintenance IRQs are, but if they only happen
> > > > > intermittently, it’s possible that you’d never get more than a single
> > > > > one in a latency-critical IRQ routine; and as such, the variatibility in
> > > > > execution time (jitter) wouldn’t be an issue in practice.  But every
> > > > > time you add a new unblockable IPI, you increase this jitter;
> > > > > particularly if this unblockable IPI might be repeated an arbitrary
> > > > > number of times.
> > > > > (Stefano, let me know if I’ve misunderstood something.)
> > > > > So stepping back a moment, here’s all the possible ideas that I think
> > > > > have been discussed (or are there implicitly) so far.
> > > > > 1. [Default] Do nothing; guests using this register continue crashing
> > > > > 2. Make the I?ACTIVER registers RZWI.
> > > > > 3. Make I?ACTIVER return the most recent known value; i.e. KVM’s current
> > > > > behavior (as far as we understand it)
> > > > > 4. Use a simple IPI with do_noop to update I?ACTIVER
> > > > > 4a.  Use an IPI, but come up with clever tricks to avoid interrupting
> > > > > guests handling IRQs.
> > > > > 5. Trap to Xen on guest EOI, so that we know when the
> > > > > 6. Some clever paravirtualized option
> > > > 7. Use an IPI if we are confident the interrupts may be active.
> > > I don’t understand this one.  How is it different than 4 or 4a?  And in
> > > particular, how does it evaluate on the “how much additional design work
> > > would it take” criteria?
> > Let me start with, if we want to have a vGIC to rule them all, then I am
> > afraid you are going to have to compromise. We can discuss about tailoring the
> > vGIC but I think before that we need a vGIC that is faithfull with the spec
> > (e.g differentiating level vs edge interrupts, handling activer...). Note that
> > Arm spent some effort to get a new vGIC merged but this was never made a first
> > class citizen.
> >
> > However, even if you tailor the vGIC, you are not going to be able to avoid
> > IPI in some occasions. This would happen when using event channels, in-guest
> > IPI... Another example is when enabling an interrupt, although I realize that
> > our vGIC does not do it today meaning that a pending interrupt while disabled
> > will not be forwarded until the vCPU exit.
> >
> > Furthermore, implementing a write to I{C,S}ACTIVER (to activate/de-activate)
> > is going to be very difficult (to not say impossible) to do without IPI.
> >
> > If you are worry about a vCPU been interrupted in critical section, then I
> > think you should tailor your guest to avoid using those registers.
> Let's call it option 8 "tell the user that she needs to modify her
> kernel."
>
>
> > An alternative would be to use spinlock/mutex within the code to prevent a
> > VCPU to access the vGIC registers while another vCPU don't want to be
> > interrupted.
> >
> > Regarding, 4a. The only way I could think of would be to trap the instructions
> > that mask/unmask interrupts. If I read correctly the Armv8 spec, it is not
> > possible to do it.
> >
> > 7. is basically 4.a the goal would be to avoid interrupts the vCPU has much as
> > possible but you may be wrong sometimes. Obviously we want to be correct most
> > of the times.
> >
> > I understand this may not be the ideal solution, but this is probably the best
> > we could come with and does not involve a lot of work because we have already
> > all the information in place (we know when an interrupt was injected to a
> > vCPU).
> >
> > The next best solution is to prevent in your guest to modify some registers of
> > the vGIC at the same time as another vCPU is in a critical section.
> Let's call this option 9.
>
>
> I am just thinking out loud here :)
Thank you for thinking out loud. Sadly, as I pointed out before, there 
are other part of the vGIC facing the same problems (e.g I{S,C}ENABLER, 
sending SGIs, sending event channels).
So can you enlighten me why I{S,C}ENABLER is that much a concern over 
the other?
> - 2 "Make the I?ACTIVER registers RZWI"
>
>    As far as I can tell it should prevent the livelock because it would
>    never return an ACTIVE state. It could be improved by returning the
>    latest ACTIVE information for local interrupts and returning zero for
>    interrupts routed to other pcpus. Not perfect but an option.
How a partial implementation will help? Wouldn't this make more 
difficult for a developper?
Bear in mind that on GICv3 you can read the information all the 
re-distributors information (not only yours).
> - 5 "maintenance interrupt"
>
>    This is good for jitter sensitive guests but not the best for the
>    others. We could enable it conditionally: enable maintenance
>    interrupts only for certain vcpus/pcpus but it is not great to have to
>    make this kind of difference in the implementation. However, it is
>    possible. Let's see if we can come up with something better.
>
>
> - 7 "optimized IPI"
>   
>    A tiny chance of causing issues. Let's see if we can come up with
>    anything better.
>
>
> - 8 "tell the user to fix modify the kernel"
>
>    We could do it in addition to 7. The issue is really how we do it.
>    A warning message if DEBUG && if sched==null? That doesn't sound
>    right. We could introduce a new nojitter=true command line option for
>    Xen? It wouldn't really change the behavior of Xen, but it would
>    enable this warning. Or, it could enable the warning and also switch
>    the implementation of I?ACTIVER to option 2.
This is not a sched=null specific problem. The problem would exactly be 
the same when you are dedicating a pCPU to a vCPU on credit and credit2.
> - 9 "prevent I?ACTIVER during critical sections"
>
>    This could be good but I have a difficult time thinking of how we
>    could implement it. How do we tell that the other vcpu is in or out of
>    the critical section?
I believe you misread what I wrote. I didn't suggest Xen would do it but 
the guest will do it. As the vCPUs belongs to the same guest.
Cheers,

--
Julien Grall