[PATCH v1 00/18] arm64/nmi: Support for FEAT_NMI

[Marc Zyngier]

On Wed, 09 Nov 2022 15:35:25 +0000,
Mark Brown <broonie at kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Sun, Nov 06, 2022 at 11:38:51AM +0000, Marc Zyngier wrote:
> > Mark Brown <broonie at kernel.org> wrote:
> 
> > > interrupts of any kind to add masking for NMIs.  This masking is not
> > > added to our standard interrupt masking operations since that would
> > > result in widespread masking of NMIs which would undermine their value.
> > > Given that there's a large amount of kernel code a good proportion of
> > > which I'm not terribly familar with it is likely that this area of the
> > > series needs attention in review as there may be be be areas that have
> > > been missed or misunderstood.
> 
> > Can you at least clarify why the no-quite-NMI cannot follow the
> > pattern established by the pseudo-NMI support? I would have expected
> > the critical sections to strictly map between the two so-called-NMI
> > implementations.
> 
> That's fair, I'll expand on this topic in the cover for next version.
> It is basically similar, there's a few differences where I couldn't
> convince myself that we were intentionally masking NMIs like
> el0_svc_common() and it did seem like we wanted to control things
> separately during entry given the separate report path we have with the
> architected version.  It's possible I've completely misunderstood this
> one way or another.
> 
> I did look at a couple of alernatives that had more overlap.  I looked
> at pulling the pNMI code out of the DAIF handling but given the
> implementation the benefits seemed unclear for the complexity given that
> it is actually working with DAIF.  I did also strongly consider putting
> the NMI handling into the DAIF handling but that was making it difficult
> to distinguish the handling of architected NMIs and the implicit
> coverage didn't seem ideal, I could have another spin through that
> approach though - I was on the edge with the approach there.  It sounds
> like you'd really prefer that one.

My gripe with the current approach is that it took us a *very* long
while to make pNMI work correctly in all cases (I'm pretty sure that
Mark Rutland still has nightmares about it). But now that we have the
framework for it and the mental model to match, I'd really want both
NMI methods to be merely two implementations of the same concept.

The only obvious difference should be that we can identify NMIs early
and that acking a NMI cannot result in acking an IRQ. I'd expect
everything else to be otherwise similar.

> 
> > > Using this feature in KVM guests will require the implementation of vGIC
> > > support which is not present in this series, and there is also no usage
> > > of the feature at EL2.  While FEAT_NMI does add a new writable register
> > > ALLINT the value is already context switched for EL1 via SPSR_EL2.ALLINT 
> > > and we can't trap read access to the register so we don't manage the
> > > write trap that is available in HCRX_EL2.TALLINT.  Guests can read from
> > > the register anyway and should only be able to affect their own state.
> 
> > ... and create some architectural state that is impossible to manage
> > and migrate. Great.
> 
> We can't already manage and migrate SPSR_EL2 (or SPSR_EL1 for that
> matter)?

Of course we can. SPSR_EL2 is nothing but the guest's PSTATE, and
thankfully we manage it. Same thing for SPSR_EL1.

> I might have missed part of how that's handled or used,
> especially given that I've not actively worked through this yet, but if
> we're not doing so then we've got other problems with for example
> PSTATE.BTYPE or PSTATE.SSBS.  It looks like it's available and I'm not
> seeing any filtering of the values or anything.

Imagine you run a VM on some FEAT_NMI-enabled HW. The guest uses the
PSTATE.ALLINT bit because it can. Now we migrate the VM somewhere else
that doesn't have ALLINT, and the VM is dead.

This is a general problem with the architecture, but we definitely
should take care of it as we're enabling these features.

> 
> Note that ALLINT.ALLINT is similar to DAIF or SBSS, it's directly
> mirroring the state of PSTATE.ALLINT.  The only access EL2 has to the
> EL1 state for ALLINT is via SPSR, on entry to EL2 ALLINT is reset as per
> the configuration in SCTLR_EL2.{SPINTMASK,NMI} and then the EL1 value is
> restored as part of exception return.
> 
> > Frankly, we are accumulating a bunch of half implemented features
> > (SME, SPE) for which there is no KVM support, and I don't think this
> > is the correct direction of travel.
> 
> Yeah, it's not great.  For NMIs Lorenzo is looking at the GIC side, I'll
> let him clarify his schedule.  Here it's more the case that implementing
> the architecture side without an interrupt controller isn't useful (and
> certainly not usefully testable) than anything else - my expectation is
> that it should work fine, though from your comments above it sounds like
> there will be issues with the handling of PSTATE that I've not realised.

The vgic support should be rather trivial. It is only a matter of
adding the required interrupt state, reworking the priority sorting
and LR stuffing, handling the additional MMIO range and dealing with
the GIC versioning. Trapping of ALLINT should be done at the same
time.

If people cannot be bothered, I'll end-up doing it myself.

	M.

-- 
Without deviation from the norm, progress is not possible.