Re: [PATCH v1 2/6] KVM: arm64: Consume pending SError as early as possible

[James Morse <james.morse@arm.com>]

Hi Julien,

On 05/06/2019 10:00, Julien Thierry wrote:
> On 04/06/2019 15:45, James Morse wrote:
>> On systems with v8.2 we switch the 'vaxorcism' of guest SError with an
>> alternative sequence that uses the ESB-instruction, then reads DISR_EL1.
>> This saves the unmasking and re-masking of asynchronous exceptions.
>>
>> We do this after we've saved the guest registers and restored the
>> host's. Any SError that becomes pending due to this will be accounted
>> to the guest, when it actually occurred during host-execution.
>>
>> Move the ESB-instruction as early as possible. Any guest SError
>> will become pending due to this ESB-instruction and then consumed to
>> DISR_EL1 before the host touches anything.

> Since you're moving the ESB from a HAS_RAS alternative location to a
> normal location, it might be worth noting in the commit message that the
> ESB is a NOP when RAS is not implemented, to clarify that we are not
> uselessly adding a barrier (or potentially undefined instruction).

Sure.


>> This lets us account for host/guest SError precisely on the guest
>> exit exception boundary.

>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>> index 914036e6b6d7..b8d37a987b34 100644
>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>> @@ -230,6 +230,7 @@ ENDPROC(\label)
>>  .macro valid_vect target
>>  	.align 7
>>  661:
>> +	esb
> 
> Having said the above, if the kernel is built without RAS support (you
> have to disable some of options enabled by default to get to that) but
> runs on a CPU that does have the RAS extention, should we execute a nop
> instead of an esb (so have an alternative here)?

That's an interesting corner! The esb-instruction would have consumed any guest-SError,
but we'd never read DISR_EL1 because that undefs, so it lives behind the RAS extension
support. The effect is guest-SError going missing.


> Also, when we have the smccc workaround installed we do two esb, is that
> intentional/necessary?

> Could we have only one esb at the start of hyp_ventry (and "only" 26
> nops after it) for KVM_INDIRECT_VECTORS? Or does this not affect
> performance that much to be of interest?

Ugh, because kvm_patch_vector_branch() that does the preamble work, and jumps over the
'real' vectors preamble depends on CONFIG_HARDEN_EL2_VECTORS, not
CONFIG_HARDEN_BRANCH_PREDICTOR. (I thought the vector tail was always generated...)

Is it harmless? aarch64/functions/ras/AArch64.ESBOperation says DISR_EL1 is overwritten if
a physical SError is pending. For this to be a problem, we'd need two, and the second one
would have to be caused by the smccc workaround (possibly by the firmware portion). This
would be accounted to the guest, which could be a problem. But either the stack has
uncorrected errors (so we aren't going to make it out of KVM alive), or firmware causes
SError. (I'm struggling to care...)

...

Would getting the unpatched kvm_patch_vector_branch() region to do the pre-amble work and
jump over it work?

We'd end up with ESB-instruction at the top of the unpatched-vector, which may be
overwritten with the smccc-workaround, which also contains an ESB-instruction.
kvm_patch_vector_branch() generates the other half of the preamble but the
unpatched-vector needs to do the same so support all the combinations.

I think this makes the addition to this Gordian-knot of code easier to reason about, which
is a good enough reason to do it!


Thanks,

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