Re: [PATCH 1/2] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Reiji Watanabe <reijiw@google.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Will Deacon <will@kernel.org>, Peter Shier <pshier@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Oliver Upton <oupton@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>
Subject: Re: [PATCH 1/2] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs
Date: Mon, 10 Jan 2022 10:48:41 +0000	[thread overview]
Message-ID: <YdwPCcZWD8Uc1eej@monolith.localdoman> (raw)
In-Reply-To: <20220110054042.1079932-1-reijiw@google.com>

Hi Reiji,

On Sun, Jan 09, 2022 at 09:40:41PM -0800, Reiji Watanabe wrote:
> vcpu_allowed_register_width() checks if all the VCPUs are either
> all 32bit or all 64bit.  Since the checking is done even for vCPUs
> that are not initialized (KVM_ARM_VCPU_INIT has not been done) yet,
> the non-initialized vCPUs are erroneously treated as 64bit vCPU,
> which causes the function to incorrectly detect a mixed-width VM.
> 
> Fix vcpu_allowed_register_width() to skip the check for vCPUs that
> are not initialized yet.
> 
> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  arch/arm64/kvm/reset.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 426bd7fbc3fd..ef78bbc7566a 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -180,8 +180,19 @@ static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
>  	if (kvm_has_mte(vcpu->kvm) && is32bit)
>  		return false;
>  
> +	/*
> +	 * Make sure vcpu->arch.target setting is visible from others so
> +	 * that the width consistency checking between two vCPUs is done
> +	 * by at least one of them at KVM_ARM_VCPU_INIT.
> +	 */
> +	smp_mb();

From ARM DDI 0487G.a, page B2-146 ("Data Memory Barrier (DMB)"):

"The DMB instruction is a memory barrier instruction that ensures the relative
order of memory accesses before the barrier with memory accesses after the
barrier."

I'm going to assume from the comment that you are referring to completion of
memory accesses ("Make sure [..] is visible from others"). Please correct me if
I am wrong. In this case, DMB ensures ordering of memory accesses with regards
to writes and reads, not *completion*.  Have a look at
tools/memory-model/litmus-tests/MP+fencewmbonceonce+fencermbonceonce.litmus for
the classic message passing example as an example of memory ordering.
Message passing and other patterns are also explained in ARM DDI 0487G.a, page
K11-8363.

I'm not saying that your approach is incorrect, but the commit message should
explain what memory accesses are being ordered relative to each other and why.

Thanks,
Alex

> +
>  	/* Check that the vcpus are either all 32bit or all 64bit */
>  	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> +		/* Skip if KVM_ARM_VCPU_INIT is not done for the vcpu yet */
> +		if (tmp->arch.target == -1)
> +			continue;
> +
>  		if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
>  			return false;
>  	}
> 
> base-commit: df0cc57e057f18e44dac8e6c18aba47ab53202f9
> -- 
> 2.34.1.575.g55b058a8bb-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel