Re: [PATCH 2/2] KVM: arm64: nVHE: Don't consume host SErrors with RAS

[Andrew Scull <ascull@google.com>]

On Wed, Aug 05, 2020 at 03:37:27PM +0100, James Morse wrote:
> Hi Andrew,
> 
> On 31/07/2020 11:20, Andrew Scull wrote:
> > On Fri, Jul 31, 2020 at 09:00:03AM +0100, Marc Zyngier wrote:
> >> On 2020-07-30 23:31, Andrew Scull wrote:
> >>> On Thu, Jul 30, 2020 at 04:18:23PM +0100, Andrew Scull wrote:
> >>>> The ESB at the start of the vectors causes any SErrors to be
> >>>> consumed to
> >>>> DISR_EL1. If the exception came from the host and the ESB caught an
> >>>> SError, it would not be noticed until a guest exits and DISR_EL1 is
> >>>> checked. Further, the SError would be attributed to the guest and not
> >>>> the host.
> >>>>
> >>>> To avoid these problems, use a different exception vector for the host
> >>>> that does not use an ESB but instead leaves any host SError pending. A
> >>>> guest will not be entered if an SError is pending so it will always be
> >>>> the host that will receive and handle it.
> >>>
> >>> Thinking further, I'm not sure this actually solves all of the problem.
> >>> It does prevent hyp from causing a host SError to be consumed but, IIUC,
> >>> there could be an SError already deferred by the host and logged in
> >>> DISR_EL1 that hyp would not preserve if a guest is run.
> >>>
> >>> I think the host's DISR_EL1 would need to be saved and restored in the
> >>> vcpu context switch which, from a cursory read of the ARM, is possible
> >>> without having to virtualize SErrors for the host.
> >>
> >> The question is what do you if you have something pending in DISR_EL1
> >> at the point where you enter EL2? Context switching it is not going to
> >> help. One problem is that you'd need to do an ESB, corrupting DISR_EL1,
> >> before any memory access (I'm assuming you can get traps where all
> >> registers are live). I can't see how we square this circle.
> 
> > I'll expand on what I understand (or think I do) about RAS at the
> > moment. It should hopefully highlight anything wrong with my reasoning
> > for your questions.
> > 
> > DISR_EL1.A being set means a pending physical SError has been
> > consumed/cleared. The host has already deferred an SError so saving and
> > restoring (i.e. preserving) DISR_EL1 for the host would mean the
> > deferred SError is as it was on return to the host.
> 
> [..]
> 
> > If there is a pending physical SError, we'd have to keep it pending so
> > the host can consume it.
> 
> I agree!
> 
> 
> > __guest_enter has the dsb-isb for RAS so
> > SErrors will become pending, but it doesn't consume them.
> 
> 
> > I can't
> > remember now whether this was reliable or not; I assumed it was as it is
> > gated on the RAS config.
> 
> It should be reliable for 'asynchronous external abort', but not 'SError Interrupt', both
> of which are signalled via the SError vector.

Uh oh, more terms with differences that I don't understand yet.

I assume that there aren't SError Interrupts that we would be concerned
about being deliered to the guest that should have been delivered to the
host after checing for asynchronous external aborts? At least, that seem
to be the current behaviour if I'm reading things right.

> The DSB completes previous memory accesses, so that the components they land in can return
> an abort if they need to. The ISB ensures the abort is seen as pending in ISR_EL1 before
> the CPU reads it.
> 
> The ESB-instruction is potentially cheaper, (as cheap as a nop if all errors are going to
> be reported as uncontained anyway), but it has the nasty side effect of consuming the
> error, which we don't want here.
>
>
> > The above didn't need an ESB for the host but incorrect assumptions
> > might change that.
> > 
> >> Furthermore, assuming you find a way to do it, what do you do with it?
> >>
> >> (a) Either there was something pending already and it is still pending,
> 
> > If a physical SError is pending, you leave it pending and go back to the
> > host so it can consume it.
> 
> Great idea!
> If the CPU has IESB, you can just check ISR_EL1 on entry from the host.
> If not, you'll need some extra barriers.

I'm not sure it would be easy to check ISR_EL1 and bail to the host in
the general case as it would require a meaningful return code for each
context, something like EAGAIN that doesn't exist in the hyp interfaces.

I was instead thinking that EL2 would continue as usual except it won't
consume any SErrors with the ESB. Care is needed when a vCPU is being
run and the DSB-ISB + ISR_EL1 check in __guest_enter is be there to
prevent a guest being entered if the host needs to handle an SError.

> >> (b) Or there was nothing pending and you now have an error that you
> >>     don't know how to report (the host EL1 never issued an ESB)
> > 
> > If there isn't a physical SError pending, either there is no SError at
> > all (easy) or the SError has already been consumed to DISR_EL1 by a host
> > ESB and we'd preserve DISR_EL1 for the host to handle however it chooses.
> 
> (I think this is a host bug)
> 
> 
> >> We could just error out on hypercalls if DISR_EL1 is non-zero, but
> >> I don't see how we do that for traps, as it would just confuse the host
> >> EL1.
> > 
> > Traps would need to be left pending. Detected but not consumed with the
> > dsb-isb in __guest_enter.
> 
> You're trapping stuff from the host?

Not yet, but it might happen e.g. to context switch FPSIMD under
protected KVM.

> If you might trap something between the ESB-instruction and the host reading DISR_EL1 then
> you can't use the ESB-instruction in your trap handling path, as it would over-write it.
> This sucks as presumably you want to make it common with the really-a-guest trap handling
> path.
> 
> The best thing to do would be to go the whole-hog and route SError to EL2 with
> HCR_EL2.AMO. SError taken out of the host can be re-injected with HCR_EL2.VSE and
> VSESR_EL2, which will be consumed to VDISR_EL2 if the 'host' executes an ESB-instruction.
> 
> DISR_EL1 then belongs to EL2, meaning its free to use the ESB-instruction as it likes.

Does a save and restore of DISR_EL1 (see other thread on this series)
address the concerns here or would you still think it better to start
virtualizing for the host?

> Thanks,
> 
> James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm