On Tue, 18 Jan 2022 16:02:28 +0000,
"Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, Nov 29, 2021 at 08:00:55PM +0000, Marc Zyngier wrote:
> > From: Jintack Lim <jintack.lim@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx>
> > [maz: switch to common exception injection framework]
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> ...
> > +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.
