On Thu, May 20, 2021 at 01:58:55PM +0100, Marc Zyngier wrote: > On Thu, 20 May 2021 13:44:34 +0100, > Mark Rutland <mark.rutl...@arm.com> wrote: > > > > On Thu, May 20, 2021 at 01:22:53PM +0100, Marc Zyngier wrote: > > > It looks like we have tolerated creating mixed-width VMs since... > > > forever. However, that was never the intention, and we'd rather > > > not have to support that pointless complexity. > > > > > > Forbid such a setup by making sure all the vcpus have the same > > > register width. > > > > > > Reported-by: Steven Price <steven.pr...@arm.com> > > > Signed-off-by: Marc Zyngier <m...@kernel.org> > > > Cc: sta...@vger.kernel.org > > > --- > > > arch/arm64/kvm/reset.c | 28 ++++++++++++++++++++++++---- > > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > > > index 956cdc240148..1cf308be6ef3 100644 > > > --- a/arch/arm64/kvm/reset.c > > > +++ b/arch/arm64/kvm/reset.c > > > @@ -166,6 +166,25 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu > > > *vcpu) > > > return 0; > > > } > > > > > > +static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu) > > > +{ > > > + struct kvm_vcpu *tmp; > > > + int i; > > > + > > > + /* Check that the vcpus are either all 32bit or all 64bit */ > > > + kvm_for_each_vcpu(i, tmp, vcpu->kvm) { > > > + bool w; > > > + > > > + w = test_bit(KVM_ARM_VCPU_EL1_32BIT, tmp->arch.features); > > > + w ^= test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features); > > > + > > > + if (w) > > > + return false; > > > + } > > > > I think this is wrong for a single-cpu VM. In that case, the loop will > > have a single iteration, and tmp == vcpu, so w must be 0 regardless of > > the value of arch.features. > > I don't immediately see what is wrong with a single-cpu VM. 'w' will > be zero indeed, and we'll return that this is allowed. After all, each > VM starts by being a single-CPU VM.
Sorry; I should have been clearer. I had assumed that this was trying to rely on a difference across vcpus implicitly providing an equivalent of the removed check for the KVM_ARM_VCPU_EL1_32BIT cap. I guess from the below that was not the case. :) Thanks, Mark. > But of course... > > > IIUC that doesn't prevent KVM_ARM_VCPU_EL1_32BIT being set when we don't > > have the ARM64_HAS_32BIT_EL1 cap, unless that's checked elsewhere? > > ... I mistakenly removed the check against ARM64_HAS_32BIT_EL1... > > > > > How about something like: > > > > | static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu) > > | { > > | bool is_32bit = vcpu_features_32bit(vcpu); > > | struct kvm_vcpu *tmp; > > | int i; > > | > > | if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is_32bit) > > | return false; > > | > > | kvm_for_each_vcpu(i, tmp, vcpu->kvm) { > > | if (is_32bit != vcpu_features_32bit(tmp)) > > | return false; > > | } > > | > > | return true; > > | } > > > > ... with a helper in <asm/kvm_emulate.h> like: > > > > | static bool vcpu_features_32bit(struct kvm_vcpu *vcpu) > > | { > > | return test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features); > > | } > > > > ... or > > > > | static inline bool vcpu_has_feature(struct kvm_vcpu *vcpu, int feature) > > | { > > | return test_bit(feature, vcpu->arch.features); > > | } > > > > ... so that we can avoid the line splitting required by the length of > > the test_bit() expression? > > Yup, looks OK to me (with a preference for the latter). > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm