Re: [PATCH v2 4/5] arm64: KVM: Prevent speculative S1 PTW when restoring vcpu context

From: Marc Zyngier <maz@kernel.org>
To: James Morse <james.morse@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>
Cc: kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v2 4/5] arm64: KVM: Prevent speculative S1 PTW when restoring vcpu context
Date: Sat, 26 Oct 2019 11:20:35 +0100	[thread overview]
Message-ID: <8636ffzu30.wl-maz@kernel.org> (raw)
In-Reply-To: <151fc868-6709-3017-e34d-649ec0e1812c@arm.com>

On Thu, 24 Oct 2019 17:10:44 +0100,
James Morse <james.morse@arm.com> wrote:

Hi James,

> Hi Marc,
> 
> On 19/10/2019 10:55, Marc Zyngier wrote:
> > When handling erratum 1319367, we must ensure that the page table
> > walker cannot parse the S1 page tables while the guest is in an
> > inconsistent state. This is done as follows:
> > 
> > On guest entry:
> > - TCR_EL1.EPD{0,1} are set, ensuring that no PTW can occur
> > - all system registers are restored, except for TCR_EL1 and SCTLR_EL1
> > - stage-2 is restored
> > - SCTLR_EL1 and TCR_EL1 are restored
> > 
> > On guest exit:
> > - SCTLR_EL1.M and TCR_EL1.EPD{0,1} are set, ensuring that no PTW can occur
> > - stage-2 is disabled
> > - All host system registers are restored
> 
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> (whitespace nit below)
> 
> 
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index 69e10b29cbd0..5765b17c38c7 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -118,6 +118,20 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> >  	}
> >  
> >  	write_sysreg(val, cptr_el2);
> > +
> > +	if (cpus_have_const_cap(ARM64_WORKAROUND_1319367)) {
> > +		struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt;
> > +
> > +		isb();
> > +		/*
> > +		 * At this stage, and thanks to the above isb(), S2 is
> > +		 * configured and enabled. We can now restore the guest's S1
> > +		 * configuration: SCTLR, and only then TCR.
> > +		 */
> 
> (note for my future self: because the guest may have had M=0 and rubbish in the TTBRs)
> 
> > +		write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1],	SYS_SCTLR);
> > +		isb();
> > +		write_sysreg_el1(ctxt->sys_regs[TCR_EL1],	SYS_TCR);
> > +	}
> >  }
> >  
> 
> 
> > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> > index 7ddbc849b580..fb97547bfa79 100644
> > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> > @@ -117,12 +117,26 @@ static void __hyp_text __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
> >  {
> >  	write_sysreg(ctxt->sys_regs[MPIDR_EL1],		vmpidr_el2);
> >  	write_sysreg(ctxt->sys_regs[CSSELR_EL1],	csselr_el1);
> > -	write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1],	SYS_SCTLR);
> > +
> > +	if (!cpus_have_const_cap(ARM64_WORKAROUND_1319367)) {
> > +		write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1],	SYS_SCTLR);
> > +		write_sysreg_el1(ctxt->sys_regs[TCR_EL1],	SYS_TCR);
> > +	} else	if (!ctxt->__hyp_running_vcpu) {
> > +		/*
> > +		 * Must only be done for guest registers, hence the context
> > +		 * test. We'recoming from the host, so SCTLR.M is already
> 
> (Nit: We'recoming?)

Well spotted, now fixed. And thanks for the reviewing, much appreciated.

Catalin, Will: given that this series conflicts with the workaround for
erratum 1542419, do you mind taking it via the arm64 tree?

To make things a bit simpler, I've updated the series with James' tags at
[1], and pushed out a resolution of the merge with arm64/for-next/core [2].

Thanks,

	M.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/erratum-1319367
[2] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/erratum-1319367-resolved

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm