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

[Julien Grall <julien@xxxxxxx>]

On 09/04/2020 13:56, Julien Grall wrote:
> 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?
To be clear, I am not saying I{S,C}ENABLER should not be a concern. But 
I would prefer if we focus on a generic solution if the problem is wider.
Cheers,

--
Julien Grall