| Message ID | 20210305150638.2675513-35-npiggin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: PPC: Book3S: C-ify the P9 entry/exit code | expand |
Nicholas Piggin <npiggin@gmail.com> writes: > Radix guest support will be removed from the P7/8 path, so disallow > dependent threads mode on P9. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/include/asm/kvm_host.h | 1 - > arch/powerpc/kvm/book3s_hv.c | 27 +++++---------------------- > 2 files changed, 5 insertions(+), 23 deletions(-) > > diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h > index 05fb00d37609..dd017dfa4e65 100644 > --- a/arch/powerpc/include/asm/kvm_host.h > +++ b/arch/powerpc/include/asm/kvm_host.h > @@ -304,7 +304,6 @@ struct kvm_arch { > u8 fwnmi_enabled; > u8 secure_guest; > u8 svm_enabled; > - bool threads_indep; > bool nested_enable; > bool dawr1_enabled; > pgd_t *pgtable; > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index cb428e2f7140..928ed8180d9d 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -103,13 +103,9 @@ static int target_smt_mode; > module_param(target_smt_mode, int, 0644); > MODULE_PARM_DESC(target_smt_mode, "Target threads per core (0 = max)"); > > -static bool indep_threads_mode = true; > -module_param(indep_threads_mode, bool, S_IRUGO | S_IWUSR); > -MODULE_PARM_DESC(indep_threads_mode, "Independent-threads mode (only on POWER9)"); > - > static bool one_vm_per_core; > module_param(one_vm_per_core, bool, S_IRUGO | S_IWUSR); > -MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires indep_threads_mode=N)"); > +MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires POWER8 or older)"); Isn't this also a security feature, where there was an ask to make sure threads/vCPU from other VM won't run on this core? In that context isn't this applicable also for P9? > > #ifdef CONFIG_KVM_XICS > static const struct kernel_param_ops module_param_ops = { > @@ -2227,7 +2223,7 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, > */ > static int threads_per_vcore(struct kvm *kvm) > { > - if (kvm->arch.threads_indep) > + if (cpu_has_feature(CPU_FTR_ARCH_300)) > return 1; > return threads_per_subcore; > } > @@ -4319,7 +4315,7 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu) > vcpu->arch.state = KVMPPC_VCPU_BUSY_IN_HOST; > > do { > - if (kvm->arch.threads_indep && kvm_is_radix(kvm)) > + if (kvm_is_radix(kvm)) > r = kvmhv_run_single_vcpu(vcpu, ~(u64)0, > vcpu->arch.vcore->lpcr); > else > @@ -4934,21 +4930,8 @@ static int kvmppc_core_init_vm_hv(struct kvm *kvm) > /* > * Track that we now have a HV mode VM active. This blocks secondary > * CPU threads from coming online. > - * On POWER9, we only need to do this if the "indep_threads_mode" > - * module parameter has been set to N. > */ > - if (cpu_has_feature(CPU_FTR_ARCH_300)) { > - if (!indep_threads_mode && !cpu_has_feature(CPU_FTR_HVMODE)) { > - pr_warn("KVM: Ignoring indep_threads_mode=N in nested hypervisor\n"); > - kvm->arch.threads_indep = true; > - } else if (!indep_threads_mode && cpu_has_feature(CPU_FTR_P9_RADIX_PREFETCH_BUG)) { > - pr_warn("KVM: Ignoring indep_threads_mode=N on pre-DD2.2 POWER9\n"); > - kvm->arch.threads_indep = true; > - } else { > - kvm->arch.threads_indep = indep_threads_mode; > - } > - } > - if (!kvm->arch.threads_indep) > + if (!cpu_has_feature(CPU_FTR_ARCH_300)) > kvm_hv_vm_activated(); > > /* > @@ -4989,7 +4972,7 @@ static void kvmppc_core_destroy_vm_hv(struct kvm *kvm) > { > debugfs_remove_recursive(kvm->arch.debugfs_dir); > > - if (!kvm->arch.threads_indep) > + if (!cpu_has_feature(CPU_FTR_ARCH_300)) > kvm_hv_vm_deactivated(); > > kvmppc_free_vcores(kvm); > -- > 2.23.0
Excerpts from Aneesh Kumar K.V's message of March 18, 2021 1:11 am: > Nicholas Piggin <npiggin@gmail.com> writes: > >> Radix guest support will be removed from the P7/8 path, so disallow >> dependent threads mode on P9. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> arch/powerpc/include/asm/kvm_host.h | 1 - >> arch/powerpc/kvm/book3s_hv.c | 27 +++++---------------------- >> 2 files changed, 5 insertions(+), 23 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h >> index 05fb00d37609..dd017dfa4e65 100644 >> --- a/arch/powerpc/include/asm/kvm_host.h >> +++ b/arch/powerpc/include/asm/kvm_host.h >> @@ -304,7 +304,6 @@ struct kvm_arch { >> u8 fwnmi_enabled; >> u8 secure_guest; >> u8 svm_enabled; >> - bool threads_indep; >> bool nested_enable; >> bool dawr1_enabled; >> pgd_t *pgtable; >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index cb428e2f7140..928ed8180d9d 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -103,13 +103,9 @@ static int target_smt_mode; >> module_param(target_smt_mode, int, 0644); >> MODULE_PARM_DESC(target_smt_mode, "Target threads per core (0 = max)"); >> >> -static bool indep_threads_mode = true; >> -module_param(indep_threads_mode, bool, S_IRUGO | S_IWUSR); >> -MODULE_PARM_DESC(indep_threads_mode, "Independent-threads mode (only on POWER9)"); >> - >> static bool one_vm_per_core; >> module_param(one_vm_per_core, bool, S_IRUGO | S_IWUSR); >> -MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires indep_threads_mode=N)"); >> +MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires POWER8 or older)"); > > Isn't this also a security feature, where there was an ask to make sure > threads/vCPU from other VM won't run on this core? In that context isn't > this applicable also for P9? I'm not sure about an ask, but it is a possible security feature that would be relevant to all SMT CPUs running KVM guests. It doesn't make much sense to plumb P9 support all through the P8 path just for that though, in my opinion? Is it tested? Who uses it? It's lacking features of the P9 path. It would be better added to KVM/QEMU in general (or until that is available, disable SMT, or use CPU pinning and isolcpus to prevent host code running on secondaries, and isolating VMs from one another, etc). I think it's quite possible to rendezvous threads in kernel, move them onto the threads of a core, and then have them all running in KVM code before they enter the guest, without disabling SMT in the host. You could do it with kernel threads on the secondaries even, but I wouldn't like to have to plumb the vcore concept entirely through everywhere so I would actually prefer to see QEMU grow an understanding of it so it would know it has to call the ioctl on every guest SMT thread. Thanks, Nick
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h index 05fb00d37609..dd017dfa4e65 100644 --- a/arch/powerpc/include/asm/kvm_host.h +++ b/arch/powerpc/include/asm/kvm_host.h @@ -304,7 +304,6 @@ struct kvm_arch { u8 fwnmi_enabled; u8 secure_guest; u8 svm_enabled; - bool threads_indep; bool nested_enable; bool dawr1_enabled; pgd_t *pgtable; diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index cb428e2f7140..928ed8180d9d 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -103,13 +103,9 @@ static int target_smt_mode; module_param(target_smt_mode, int, 0644); MODULE_PARM_DESC(target_smt_mode, "Target threads per core (0 = max)"); -static bool indep_threads_mode = true; -module_param(indep_threads_mode, bool, S_IRUGO | S_IWUSR); -MODULE_PARM_DESC(indep_threads_mode, "Independent-threads mode (only on POWER9)"); - static bool one_vm_per_core; module_param(one_vm_per_core, bool, S_IRUGO | S_IWUSR); -MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires indep_threads_mode=N)"); +MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires POWER8 or older)"); #ifdef CONFIG_KVM_XICS static const struct kernel_param_ops module_param_ops = { @@ -2227,7 +2223,7 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id, */ static int threads_per_vcore(struct kvm *kvm) { - if (kvm->arch.threads_indep) + if (cpu_has_feature(CPU_FTR_ARCH_300)) return 1; return threads_per_subcore; } @@ -4319,7 +4315,7 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu) vcpu->arch.state = KVMPPC_VCPU_BUSY_IN_HOST; do { - if (kvm->arch.threads_indep && kvm_is_radix(kvm)) + if (kvm_is_radix(kvm)) r = kvmhv_run_single_vcpu(vcpu, ~(u64)0, vcpu->arch.vcore->lpcr); else @@ -4934,21 +4930,8 @@ static int kvmppc_core_init_vm_hv(struct kvm *kvm) /* * Track that we now have a HV mode VM active. This blocks secondary * CPU threads from coming online. - * On POWER9, we only need to do this if the "indep_threads_mode" - * module parameter has been set to N. */ - if (cpu_has_feature(CPU_FTR_ARCH_300)) { - if (!indep_threads_mode && !cpu_has_feature(CPU_FTR_HVMODE)) { - pr_warn("KVM: Ignoring indep_threads_mode=N in nested hypervisor\n"); - kvm->arch.threads_indep = true; - } else if (!indep_threads_mode && cpu_has_feature(CPU_FTR_P9_RADIX_PREFETCH_BUG)) { - pr_warn("KVM: Ignoring indep_threads_mode=N on pre-DD2.2 POWER9\n"); - kvm->arch.threads_indep = true; - } else { - kvm->arch.threads_indep = indep_threads_mode; - } - } - if (!kvm->arch.threads_indep) + if (!cpu_has_feature(CPU_FTR_ARCH_300)) kvm_hv_vm_activated(); /* @@ -4989,7 +4972,7 @@ static void kvmppc_core_destroy_vm_hv(struct kvm *kvm) { debugfs_remove_recursive(kvm->arch.debugfs_dir); - if (!kvm->arch.threads_indep) + if (!cpu_has_feature(CPU_FTR_ARCH_300)) kvm_hv_vm_deactivated(); kvmppc_free_vcores(kvm);
Radix guest support will be removed from the P7/8 path, so disallow dependent threads mode on P9. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/include/asm/kvm_host.h | 1 - arch/powerpc/kvm/book3s_hv.c | 27 +++++---------------------- 2 files changed, 5 insertions(+), 23 deletions(-)