[RFC PATCH v2 04/28] KVM: arm64: Keep consistency of ID registers between vCPUs

[Reiji Watanabe]

Hi Oliver,

On Thu, Nov 4, 2021 at 9:34 AM Oliver Upton <oupton at google.com> wrote:
>
> On Tue, Nov 02, 2021 at 11:24:56PM -0700, Reiji Watanabe wrote:
> > All vCPUs that are owned by a VM must have the same values of ID
> > registers.
> >
> > Return an error at the very first KVM_RUN for a vCPU if the vCPU has
> > different values in any ID registers from any other vCPUs that have
> > already started KVM_RUN once.  Also, return an error if userspace
> > tries to change a value of ID register for a vCPU that already
> > started KVM_RUN once.
> >
> > Changing ID register is still not allowed at present though.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  2 ++
> >  arch/arm64/kvm/arm.c              |  4 ++++
> >  arch/arm64/kvm/sys_regs.c         | 31 +++++++++++++++++++++++++++++++
> >  3 files changed, 37 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 0cd351099adf..69af669308b0 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -745,6 +745,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> >  long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> >                               struct kvm_arm_copy_mte_tags *copy_tags);
> >
> > +int kvm_id_regs_consistency_check(const struct kvm_vcpu *vcpu);
> > +
> >  /* Guest/host FPSIMD coordination helpers */
> >  int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> >  void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index fe102cd2e518..83cedd74de73 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -595,6 +595,10 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> >               return -EPERM;
> >
> >       vcpu->arch.has_run_once = true;
> > +     if (kvm_id_regs_consistency_check(vcpu)) {
> > +             vcpu->arch.has_run_once = false;
> > +             return -EPERM;
> > +     }
>
> It might be nice to return an error to userspace synchronously (i.e. on
> the register write). Of course, there is still the issue where userspace
> writes to some (but not all) of the vCPU feature ID registers, which
> can't be known until the first KVM_RUN.

Yes, I agree that it would be better.  As I mentioned for patch-02,
I will remove the consistency checking amongst vCPUs anyway though.


> >       kvm_arm_vcpu_init_debug(vcpu);
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 64d51aa3aee3..e34351fdc66c 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1436,6 +1436,10 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
> >       if (val != read_id_reg(vcpu, rd, raz) && !GET_ID_REG_INFO(encoding))
> >               return -EINVAL;
> >
> > +     /* Don't allow to change the reg after the first KVM_RUN. */
> > +     if (vcpu->arch.has_run_once)
> > +             return -EINVAL;
> > +
> >       if (raz)
> >               return 0;
> >
> > @@ -3004,6 +3008,33 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> >       return write_demux_regids(uindices);
> >  }
> >
> > +int kvm_id_regs_consistency_check(const struct kvm_vcpu *vcpu)
> > +{
> > +     int i;
> > +     const struct kvm_vcpu *t_vcpu;
> > +
> > +     /*
> > +      * Make sure vcpu->arch.has_run_once is visible for others so that
> > +      * ID regs' consistency between two vCPUs is checked by either one
> > +      * at least.
> > +      */
> > +     smp_mb();
> > +     WARN_ON(!vcpu->arch.has_run_once);
> > +
> > +     kvm_for_each_vcpu(i, t_vcpu, vcpu->kvm) {
> > +             if (!t_vcpu->arch.has_run_once)
> > +                     /* ID regs still could be updated. */
> > +                     continue;
> > +
> > +             if (memcmp(&__vcpu_sys_reg(vcpu, ID_REG_BASE),
> > +                        &__vcpu_sys_reg(t_vcpu, ID_REG_BASE),
> > +                        sizeof(__vcpu_sys_reg(vcpu, ID_REG_BASE)) *
> > +                                     KVM_ARM_ID_REG_MAX_NUM))
> > +                     return -EINVAL;
> > +     }
> > +     return 0;
> > +}
> > +
>
> Couldn't we do the consistency check exactly once per VM? I had alluded
> to this when reviewing Raghu's patches, but I think the same applies
> here too: an abstraction for detecting the first vCPU to run in a VM.
>
> https://lore.kernel.org/all/YYMKphExkqttn2w0@google.com/

Yes, the same applies to this, as well.

Thank you so much for your review !

Regards,
Reiji