[PATCH v5 14/69] KVM: arm64: nv: Support virtual EL2 exceptions

[Marc Zyngier]

On Tue, 18 Jan 2022 16:02:28 +0000,
"Russell King (Oracle)" <linux at armlinux.org.uk> wrote:
> 
> On Mon, Nov 29, 2021 at 08:00:55PM +0000, Marc Zyngier wrote:
> > From: Jintack Lim <jintack.lim at linaro.org>
> > 
> > Support injecting exceptions and performing exception returns to and
> > from virtual EL2.  This must be done entirely in software except when
> > taking an exception from vEL0 to vEL2 when the virtual HCR_EL2.{E2H,TGE}
> > == {1,1}  (a VHE guest hypervisor).
> > 
> > Signed-off-by: Jintack Lim <jintack.lim at linaro.org>
> > Signed-off-by: Christoffer Dall <christoffer.dall at arm.com>
> > [maz: switch to common exception injection framework]
> > Signed-off-by: Marc Zyngier <maz at kernel.org>
> > ---
> ...
> > +void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu)
> > +{
> > +	u64 spsr, elr, mode;
> > +	bool direct_eret;
> > +
> > +	/*
> > +	 * Going through the whole put/load motions is a waste of time
> > +	 * if this is a VHE guest hypervisor returning to its own
> > +	 * userspace, or the hypervisor performing a local exception
> > +	 * return. No need to save/restore registers, no need to
> > +	 * switch S2 MMU. Just do the canonical ERET.
> > +	 */
> > +	spsr = vcpu_read_sys_reg(vcpu, SPSR_EL2);
> > +	mode = spsr & (PSR_MODE_MASK | PSR_MODE32_BIT);
> > +
> > +	direct_eret  = (mode == PSR_MODE_EL0t &&
> > +			vcpu_el2_e2h_is_set(vcpu) &&
> > +			vcpu_el2_tge_is_set(vcpu));
> > +	direct_eret |= (mode == PSR_MODE_EL2h || mode == PSR_MODE_EL2t);
> 
> There are excessive parens on the RHS of the above two.

I guess this is my personal taste, and this is the kind of cosmetic
things that help me reason about the code. Some people use syntax
highlighting, I use bracketing. I don't think this really matters in
the grand scheme of things.

[...]

> > +/*
> > + * Emulate taking an exception to EL2.
> > + * See ARM ARM J8.1.2 AArch64.TakeException()
> > + */
> > +static int kvm_inject_nested(struct kvm_vcpu *vcpu, u64 esr_el2,
> > +			     enum exception_type type)
> > +{
> > +	u64 pstate, mode;
> > +	bool direct_inject;
> > +
> > +	if (!nested_virt_in_use(vcpu)) {
> > +		kvm_err("Unexpected call to %s for the non-nesting configuration\n",
> > +				__func__);
> 
> Too much indentation. I'm guessing this "unexpected" condition isn't
> something that can be caused by a rogue guest? If it can, doesn't this
> need to be rate limited?

If we end-up here, this is very much a hypervisor logic bug.

[...]

> > +
> > +	/* If not nesting, EL1 is the only possible exception target */
> > +	if (likely(!nested_virt_in_use(vcpu))) {
> > +		vcpu->arch.flags |= KVM_ARM64_EXCEPT_AA64_EL1;
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * With NV, we need to pick between EL1 and EL2. Note that we
> > +	 * never deal with a nesting exception here, hence never
> > +	 * changing context, and the exception itself can be delayed
> > +	 * until the next entry.
> > +	 */
> > +	switch(*vcpu_cpsr(vcpu) & PSR_MODE_MASK) {
> > +	case PSR_MODE_EL2h:
> > +	case PSR_MODE_EL2t:
> > +		vcpu->arch.flags |= KVM_ARM64_EXCEPT_AA64_EL2;
> > +		break;
> > +	case PSR_MODE_EL1h:
> > +	case PSR_MODE_EL1t:
> > +		vcpu->arch.flags |= KVM_ARM64_EXCEPT_AA64_EL1;
> > +		break;
> > +	case PSR_MODE_EL0t:
> > +		if (vcpu_el2_tge_is_set(vcpu) & HCR_TGE)
> > +			vcpu->arch.flags |= KVM_ARM64_EXCEPT_AA64_EL2;
> > +		else
> > +			vcpu->arch.flags |= KVM_ARM64_EXCEPT_AA64_EL1;
> > +		break;
> > +	default:
> > +		BUG();
> 
> Is taking out the host really appropriate here? Is this something a
> rogue guest could trigger?

This switch is supposed to cover all the NS exception levels, in
either stack configuration. If we suddenly find ourselves with a
non-architectural state, we have horribly messed up. And no, a guest
shouldn't be able to affect this. If it can, that's even more of a
reason to take everything down ASAP.

Thanks,

	M.

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