Hi Marc,
On Fri, Mar 4, 2022 at 6:57 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
On Fri, 04 Mar 2022 08:00:20 +0000,
Reiji Watanabe <reijiw@xxxxxxxxxx> 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
