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

[Reiji Watanabe <reijiw@google.com>]

Hi Alex,

On Mon, Jan 10, 2022 at 2:48 AM Alexandru Elisei
<alexandru.elisei@arm.com> wrote:
>
> Hi Reiji,
>
> On Sun, Jan 09, 2022 at 09:40:41PM -0800, Reiji Watanabe wrote:
> > vcpu_allowed_register_width() checks if all the VCPUs are either
> > all 32bit or all 64bit.  Since the checking is done even for vCPUs
> > that are not initialized (KVM_ARM_VCPU_INIT has not been done) yet,
> > the non-initialized vCPUs are erroneously treated as 64bit vCPU,
> > which causes the function to incorrectly detect a mixed-width VM.
> >
> > Fix vcpu_allowed_register_width() to skip the check for vCPUs that
> > are not initialized yet.
> >
> > Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  arch/arm64/kvm/reset.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index 426bd7fbc3fd..ef78bbc7566a 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -180,8 +180,19 @@ static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> >       if (kvm_has_mte(vcpu->kvm) && is32bit)
> >               return false;
> >
> > +     /*
> > +      * Make sure vcpu->arch.target setting is visible from others so
> > +      * that the width consistency checking between two vCPUs is done
> > +      * by at least one of them at KVM_ARM_VCPU_INIT.
> > +      */
> > +     smp_mb();
>
> From ARM DDI 0487G.a, page B2-146 ("Data Memory Barrier (DMB)"):
>
> "The DMB instruction is a memory barrier instruction that ensures the relative
> order of memory accesses before the barrier with memory accesses after the
> barrier."
>
> I'm going to assume from the comment that you are referring to completion of
> memory accesses ("Make sure [..] is visible from others"). Please correct me if
> I am wrong. In this case, DMB ensures ordering of memory accesses with regards
> to writes and reads, not *completion*.  Have a look at
> tools/memory-model/litmus-tests/MP+fencewmbonceonce+fencermbonceonce.litmus for
> the classic message passing example as an example of memory ordering.
> Message passing and other patterns are also explained in ARM DDI 0487G.a, page
> K11-8363.
>
> I'm not saying that your approach is incorrect, but the commit message should
> explain what memory accesses are being ordered relative to each other and why.

Thank you so much for the review.
What I meant with the comment was:
---
  DMB is used to make sure that writing @vcpu->arch.target, which is done
  by kvm_vcpu_set_target() before getting here, is visible to other PEs
  before the following kvm_for_each_vcpu iteration reads the other vCPUs'
  target field.
---
Did the comment become more clear ?? (Or do I use DMB incorrectly ?)

> > +
> >       /* Check that the vcpus are either all 32bit or all 64bit */
> >       kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> > +             /* Skip if KVM_ARM_VCPU_INIT is not done for the vcpu yet */
> > +             if (tmp->arch.target == -1)
> > +                     continue;

I just noticed DMB(ishld) is needed here to assure ordering between
reading tmp->arch.target and reading vcpu->arch.features for this fix.
Similarly, kvm_vcpu_set_target() needs DMB(ishst) to assure ordering
between writing vcpu->arch.features and writing vcpu->arch.target...
I am going to fix them in the v2 series.

Thanks,
Reiji

> > +
> >               if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> >                       return false;
> >       }
> >
> > base-commit: df0cc57e057f18e44dac8e6c18aba47ab53202f9
> > --
> > 2.34.1.575.g55b058a8bb-goog
> >