| Message ID | 20211103062520.1445832-5-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
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@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. > > 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/ -- Thanks Oliver
Hi Oliver, On Thu, Nov 4, 2021 at 9:34 AM Oliver Upton <oupton@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@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
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; + } 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; +} + static void id_reg_info_init_all(void) { int i;
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@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(+)