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

[Reiji Watanabe <reijiw@google.com>]

Hi Marc,

On Fri, Mar 4, 2022 at 6:57 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 04 Mar 2022 08:00:20 +0000,
> Reiji Watanabe <reijiw@google.com> wrote:
> >
> > > > +{
> > > > +     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 (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
> > > > +             allowed = (is32bit ==
> > > > +                        test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
> > > > +     } else {
> > > > +             if (is32bit)
> > > > +                     set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > >
> > > nit: probably best written as:
> > >
> > >                 __assign_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags, is32bit);
> > >
> > > > +
> > > > +             set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
> > >
> > > Since this is only ever set whilst holding the lock, you can user the
> > > __set_bit() version.
> >
> > Thank you for the proposal. But since other CPUs could attempt
> > to set other bits without holding the lock, I don't think we
> > can use the non-atomic version here.
>
> Ah, good point. Keep the atomic accesses then.
>
> >
> > >
> > > > +     }
> > > > +
> > > > +     mutex_unlock(&kvm->lock);
> > > > +
> > > > +     return allowed ? 0 : -EINVAL;
> > > > +}
> > > > +
> > > >  static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> > > >                              const struct kvm_vcpu_init *init)
> > > >  {
> > > > @@ -1140,6 +1177,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);
> > >
> > > Why is that called *after* resetting the vcpu, which itself relies on
> > > KVM_ARM_VCPU_EL1_32BIT, which we agreed to get rid of as much as
> > > possible?
> >
> > That's because I didn't want to set EL1_32BIT/REG_WIDTH_CONFIGURED
> > for the guest based on the vCPU for which KVM_ARM_VCPU_INIT would fail.
> > The flags can be set in the kvm_reset_vcpu() and cleared in
> > case of failure.  But then that temporary value could lead
> > KVM_ARM_VCPU_INIT for other vCPUs to fail, which I don't think
> > is nice to do.
>
> But it also means that userspace is trying to create incompatible
> vcpus concurrently. Why should we care? We shouldn't even consider
> resetting the flags on failure, as userspace has already indicated its
> intention to create a 32 or 64bit VM.


Right, I understand it won't practically matter:)
I will fix the code to set the flags based on the first vCPU that calls
kvm_reset_vcpu() (and keep the flags even if kvm_reset_vcpu() fails).

Thank you!
Reiji
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm