Hi Oliver, On Thu, Nov 4, 2021 at 9:34 AM Oliver Upton <oupton@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx/ Yes, the same applies to this, as well. Thank you so much for your review ! Regards, Reiji _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm