On Tue, 18 Jan 2022 04:19:22 +0000,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> KVM allows userspace to configure either all 32bit or 64bit vCPUs
> for a guest. At vCPU reset, vcpu_allowed_register_width() checks
> if the vcpu's register width is consistent with all other vCPUs'.
> Since the checking is done even against vCPUs that are not initialized
> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
> are erroneously treated as 64bit vCPU, which causes the function to
> incorrectly detect a mixed-width VM.
>
> Introduce a new flag (el1_reg_width) in kvm_arch to indicates that
> the guest needs to be configured with all 32bit or 64bit vCPUs,
> and initialize it at the first KVM_ARM_VCPU_INIT for the guest.
> Check vcpu's register width against the flag at the vcpu's
> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
>
> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/kvm_host.h | 13 +++++++++++++
> arch/arm64/kvm/arm.c | 30 ++++++++++++++++++++++++++++++
> arch/arm64/kvm/reset.c | 8 --------
> 3 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2a5f7f38006f..c02b7caf2c82 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -102,6 +102,12 @@ struct kvm_s2_mmu {
> struct kvm_arch_memory_slot {
> };
>
> +enum kvm_el1_reg_width {
> + EL1_WIDTH_UNINITIALIZED = 0,
> + EL1_32BIT,
> + EL1_64BIT,
> +};
> +
> struct kvm_arch {
> struct kvm_s2_mmu mmu;
>
> @@ -137,6 +143,13 @@ struct kvm_arch {
>
> /* Memory Tagging Extension enabled for the guest */
> bool mte_enabled;
> +
> + /*
> + * EL1 register width for the guest.
> + * This is set at the first KVM_ARM_VCPU_INIT for the guest based
> + * on whether the vcpu has KVM_ARM_VCPU_EL1_32BIT or not.
> + */
> + enum kvm_el1_reg_width el1_reg_width;
I really don't like that we need to keep track of yet another bit of
state on top of the existing one. Duplicating state is a source of
bugs, because you always end up checking the wrong one at the wrong
time (and I have scars to prove it).
> };
>
> struct kvm_vcpu_fault_info {
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e4727dc771bf..54ae8bf9d187 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1058,6 +1058,32 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
> return -EINVAL;
> }
>
> +/*
> + * A guest can have either all 32bit or 64bit vcpus only.
That's not strictly true. All we are enforcing is that EL1 is either
32 or 64bit.
> + * Either one the guest has is indicated in kvm->arch.el1_reg_width.
> + * Check if the vcpu's register width is consistent with
> + * kvm->arch.el1_reg_width. If kvm->arch.el1_reg_width is not set yet,
> + * set it based on the vcpu's KVM_ARM_VCPU_EL1_32BIT configuration.
> + */
> +static int kvm_register_width_check_or_init(struct kvm_vcpu *vcpu)
> +{
> + bool is32bit;
> + bool allowed = true;
> + struct kvm *kvm = vcpu->kvm;
> +
> + is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> +
> + mutex_lock(&kvm->lock);
> +
> + if (kvm->arch.el1_reg_width == EL1_WIDTH_UNINITIALIZED)
> + kvm->arch.el1_reg_width = is32bit ? EL1_32BIT : EL1_64BIT;
> + else
> + allowed = (is32bit == (kvm->arch.el1_reg_width == EL1_32BIT));
> +
> + mutex_unlock(&kvm->lock);
> + return allowed ? 0 : -EINVAL;
> +}
> +
> static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> const struct kvm_vcpu_init *init)
> {
> @@ -1097,6 +1123,10 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>
> /* Now we know what it is, we can reset it. */
> ret = kvm_reset_vcpu(vcpu);
> +
> + if (!ret)
> + ret = kvm_register_width_check_or_init(vcpu);
> +
> if (ret) {
> vcpu->arch.target = -1;
> bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 426bd7fbc3fd..dbf2939a6a96 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -168,9 +168,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
>
> static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> {
> - struct kvm_vcpu *tmp;
> bool is32bit;
> - int i;
>
> is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> @@ -180,12 +178,6 @@ static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> if (kvm_has_mte(vcpu->kvm) && is32bit)
> return false;
>
> - /* Check that the vcpus are either all 32bit or all 64bit */
> - kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> - if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> - return false;
> - }
> -
In [1], I suggested another approach that didn't require extra state,
and moved the existing checks under the kvm lock. What was wrong with
that approach?
Thanks,
M.
[1] https://lore.kernel.org/r/875yqqtn5q.wl-maz@xxxxxxxxxx
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
