Hi Marc,
On Tue, Feb 8, 2022 at 6:41 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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.
I will fix the comment.
>
> > + * 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?
With that approach, even for a vcpu that has a broken set of features,
which leads kvm_reset_vcpu() to fail for the vcpu, the vcpu->arch.features
are checked by other vCPUs' vcpu_allowed_register_width() until the
vcpu->arch.target is set to -1.
Due to this, I would think some or possibly all vCPUs' kvm_reset_vcpu()
may or may not fail (e.g. if userspace tries to configure vCPU#0 with
32bit EL1, and vCPU#1 and #2 with 64 bit EL1, KVM_ARM_VCPU_INIT
for either vCPU#0, or both vCPU#1 and #2 should fail. But, with that
approach, it doesn't always work that way. Instead, KVM_ARM_VCPU_INIT
for all vCPUs could fail or KVM_ARM_VCPU_INIT for vCPU#0 and #1 could
fail while the one for CPU#2 works).
Also, even after the first KVM_RUN for vCPUs are already done,
(the first) KVM_ARM_VCPU_INIT for another vCPU could cause the
kvm_reset_vcpu() for those vCPUs to fail.
I would think those behaviors are odd, and I wanted to avoid them.
Thanks,
Reiji
>
> 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
