Re: [PATCH 06/16] KVM: arm64: Force a full unmap on vpcu reinit

From: Marc Zyngier <maz@kernel.org>
To: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, qperret@google.com,
	dbrazdil@google.com, Srivatsa Vaddagiri <vatsa@codeaurora.org>,
	Shanker R Donthineni <sdonthineni@nvidia.com>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	kernel-team@android.com
Subject: Re: [PATCH 06/16] KVM: arm64: Force a full unmap on vpcu reinit
Date: Wed, 28 Jul 2021 11:38:35 +0100	[thread overview]
Message-ID: <87wnpad8pg.wl-maz@kernel.org> (raw)
In-Reply-To: <20210727181132.GE19173@willie-the-truck>

On Tue, 27 Jul 2021 19:11:33 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> On Thu, Jul 15, 2021 at 05:31:49PM +0100, Marc Zyngier wrote:
> > As we now keep information in the S2PT, we must be careful not
> > to keep it across a VM reboot, which could otherwise lead to
> > interesting problems.
> > 
> > Make sure that the S2 is completely discarded on reset of
> > a vcpu, and remove the flag that enforces the MMIO check.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/arm.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 97ab1512c44f..b0d2225190d2 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -1096,12 +1096,18 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
> >  	 * ensuring that the data side is always coherent. We still
> >  	 * need to invalidate the I-cache though, as FWB does *not*
> >  	 * imply CTR_EL0.DIC.
> > +	 *
> > +	 * If the MMIO guard was enabled, we pay the price of a full
> > +	 * unmap to get back to a sane state (and clear the flag).
> >  	 */
> >  	if (vcpu->arch.has_run_once) {
> > -		if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB))
> > +		if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB) ||
> > +		    test_bit(KVM_ARCH_FLAG_MMIO_GUARD, &vcpu->kvm->arch.flags))
> >  			stage2_unmap_vm(vcpu->kvm);
> >  		else
> >  			icache_inval_all_pou();
> > +
> > +		clear_bit(KVM_ARCH_FLAG_MMIO_GUARD, &vcpu->kvm->arch.flags);
> 
> What prevents this racing with another vCPU trying to set the bit?

Not much. We could take the kvm lock on both ends to serialize it, but
that's pretty ugly. And should we care? What is the semantic of
resetting a vcpu while another is still running?

If we want to support this sort of behaviour, then our tracking is
totally bogus, because it is VM-wide. And you don't even have to play
with that bit from another vcpu: all the information is lost at the
point where we unmap the S2 PTs.

Maybe an alternative is to move this to the halt/reboot PSCI handlers,
making it clearer what we expect?

	M.

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