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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>]

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


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


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


- 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 was brainstorming a way to re-enable trapping the idling
  instruction WFI temporarily so that the other vcpu would definitely
  trap into Xen again when it is out of the critical section. But it
  doesn't seem possible from the spec.)