diff mbox series

[v6,38/48] KVM: PPC: Book3S HV: Remove support for dependent threads mode on P9

Message ID 20210405011948.675354-39-npiggin@gmail.com
State New
Headers show
Series KVM: PPC: Book3S: C-ify the P9 entry/exit code | expand

Commit Message

Nicholas Piggin April 5, 2021, 1:19 a.m. UTC
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(-)

Comments

Paul Mackerras April 7, 2021, 6:51 a.m. UTC | #1
On Mon, Apr 05, 2021 at 11:19:38AM +1000, Nicholas Piggin wrote:
> Radix guest support will be removed from the P7/8 path, so disallow
> dependent threads mode on P9.

Dependent threads mode on P9 was added in order to support the mode
where for security reasons you want to restrict the vcpus that run on
a core to all be from the same VM, without requiring all guests to run
single-threaded.  This was (at least at one stage) thought to be a
useful mode for users that are worried about side-channel data leaks.

Now it's possible that that mode is not practically useful for some
reason, or that no-one actually wants it, but its removal should be
discussed.  Also, the fact that we are losing that mode should be
explicit in the commit message.

Paul.
Nicholas Piggin April 7, 2021, 7:44 a.m. UTC | #2
Excerpts from Paul Mackerras's message of April 7, 2021 4:51 pm:
> On Mon, Apr 05, 2021 at 11:19:38AM +1000, Nicholas Piggin wrote:
>> Radix guest support will be removed from the P7/8 path, so disallow
>> dependent threads mode on P9.
> 
> Dependent threads mode on P9 was added in order to support the mode
> where for security reasons you want to restrict the vcpus that run on
> a core to all be from the same VM, without requiring all guests to run
> single-threaded.  This was (at least at one stage) thought to be a
> useful mode for users that are worried about side-channel data leaks.

Right.

> 
> Now it's possible that that mode is not practically useful for some
> reason, or that no-one actually wants it, but its removal should be
> discussed.  Also, the fact that we are losing that mode should be
> explicit in the commit message.

Let's discuss. Did / does anyone really use it or ask for it that you
know of? What do other archs do? Compared with using standard options
that would achive this kind of security (disable SMT, I guess?) how
much is this worth keeping?

It was pretty simple to support when the P8 dependent theads code had
to support P9 anyway. After this series, now all that code is only for
that one feature, so it would be pretty nice to be able to remove it.
How do we reach a point where you'd be okay to remove this and tell 
people to just turn off SMT?

Thanks,
Nick
Nicholas Piggin April 7, 2021, 9:35 a.m. UTC | #3
Excerpts from Nicholas Piggin's message of April 7, 2021 5:44 pm:
> Excerpts from Paul Mackerras's message of April 7, 2021 4:51 pm:
>> On Mon, Apr 05, 2021 at 11:19:38AM +1000, Nicholas Piggin wrote:
>>> Radix guest support will be removed from the P7/8 path, so disallow
>>> dependent threads mode on P9.
>> 
>> Dependent threads mode on P9 was added in order to support the mode
>> where for security reasons you want to restrict the vcpus that run on
>> a core to all be from the same VM, without requiring all guests to run
>> single-threaded.  This was (at least at one stage) thought to be a
>> useful mode for users that are worried about side-channel data leaks.
> 
> Right.
> 
>> 
>> Now it's possible that that mode is not practically useful for some
>> reason, or that no-one actually wants it, but its removal should be
>> discussed.  Also, the fact that we are losing that mode should be
>> explicit in the commit message.
> 
> Let's discuss. Did / does anyone really use it or ask for it that you
> know of? What do other archs do? Compared with using standard options
> that would achive this kind of security (disable SMT, I guess?) how
> much is this worth keeping?
> 
> It was pretty simple to support when the P8 dependent theads code had
> to support P9 anyway. After this series, now all that code is only for
> that one feature, so it would be pretty nice to be able to remove it.
> How do we reach a point where you'd be okay to remove this and tell 
> people to just turn off SMT?

Assuming we do decide to remove it, how's about something like this for 
a changelog. Does a bit more justice to the feature and its removal.

Thanks,
Nick

    Dependent-threads mode is the normal KVM mode for pre-POWER9 SMT
    processors, where all threads in a core (or subcore) would run the same
    partition at the same time, or they would run the host.
    
    This design was mandated by MMU state that is shared between threads in
    a processor, so the synchronisation point is in hypervisor real-mode
    that has essentially no shared state, so it's safe for multiple threads
    to gather and switch to the correct mode.
    
    It is implemented by having the host unplug all secondary threads and
    always run in SMT1 mode, and host QEMU threads essentially represent
    virtual cores that wake these secondary threads out of unplug when the
    ioctl is called to run the guest. This happens via a side-path that is
    mostly invisible to the rest of the Linux host and the secondary threads
    still appear to be unplugged.
    
    POWER9 / ISA v3.0 has a more flexible MMU design that is independent
    per-thread and allows a much simpler KVM implementation. Before the new
    "P9 fast path" was added that began to take advantage of this, POWER9
    support was implemented in the existing path which has support to run
    in the dependent threads mode. So it was not much work to add support to
    run POWER9 in this dependent threads mode.
    
    The mode is not required by the POWER9 MMU (although "mixed-mode" hash /
    radix MMU limitations of early processors were worked around using this
    mode). But it is one way to run SMT guests without running different
    guests or guest and host on different threads of the same core, so it
    could avoid or reduce some SMT attack surfaces without turning off SMT
    entirely.
    
    This security feature has some real, if indeterminate, value. However
    the old path is lagging in features (nested HV), and with this series
    the new P9 path adds remaining missing features (radix prefetch bug
    and hash support, in later patches), so POWER9 dependent threads mode
    support would be the only remaining reason to keep that code in and keep
    supporting POWER9/POWER10 in the old path. So here we make the call to
    drop this feature.
    
    Remove dependent threads mode support for POWER9 and above processors.
    Systems can still achieve this security by disabling SMT entirely, but
    that would generally come at a larger performance cost for guests.
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index fa0083345b11..102928811da9 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 21e13f93235a..20ced6c5edfd 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 = {
@@ -2264,7 +2260,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;
 }
@@ -4360,7 +4356,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
@@ -4984,21 +4980,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();
 
 	/*
@@ -5039,7 +5022,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);