From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 09 Jul 2018 10:26:57 -0000 Received: from mail-wm0-x233.google.com ([2a00:1450:400c:c09::233]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1fcTNf-0003vt-PL for speck@linutronix.de; Mon, 09 Jul 2018 12:26:56 +0200 Received: by mail-wm0-x233.google.com with SMTP id s12-v6so20638549wmc.1 for ; Mon, 09 Jul 2018 03:26:55 -0700 (PDT) Received: from gmail.com (2E8B0CD5.catv.pool.telekom.hu. [46.139.12.213]) by smtp.gmail.com with ESMTPSA id c7-v6sm28494617wre.73.2018.07.09.03.26.47 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 09 Jul 2018 03:26:47 -0700 (PDT) Sender: Ingo Molnar Date: Mon, 9 Jul 2018 12:26:45 +0200 From: Ingo Molnar Subject: [MODERATED] Re: [patch 1/2] Command line and documentation 1 Message-ID: <20180709102645.GA26055@gmail.com> References: <20180708125216.197406530@linutronix.de> <20180708125654.729119463@linutronix.de> MIME-Version: 1.0 In-Reply-To: <20180708125654.729119463@linutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: * speck for Thomas Gleixner wrote: > From: Jiri Kosina > Subject: [patch 1/2] x86/bugs, kvm: introduce boot-time control of L1TF mitigations Looks good to me, I only have minor nits: > Introduce 'l1tf=' kernel commad line option to allow for boot-time > switching of mitigation that is used on processors affected by L1TF. Beyond the typo there's also a missing article: s/Introduce 'l1tf=' kernel command line option /Introduce the 'l1tf=' kernel command line option > > The possible values are: > > full > Provide all available mitigations for the L1TF > vulnerability. Disable SMT and enable all mitigations > in the hypervisors. SMT control is still possible > after boot. Maybe: SMT control via /sys/devices/system/cpu/smt/control is still possible after boot. to make the text more helpful overall? > full,force > Same as 'full', but disables SMT control. Implies the > 'nosmt=force' command line option. Maybe add: full,force Same as 'full', but disables SMT control. Implies the 'nosmt=force' command line option. (I.e. sysfs control of SMT is disabled.) > novirt > Leaves SMT enabled and does not enable the hypervisor > mitigation. Hypervisors will issue a warning when the first VM is > being started in a potentially insecure configuration, i.e. SMT > enabled or L1D flush disabled. s/being started /started > novirt,nowarn > Same as 'novirt', but hypervisors will not warn when > a VM is started in a potentially insecure configuration. > > Default is 'novirt'. > > Let KVM adhere to these semantics, which means: > > - 'lt1f=full,force' : start performing L1D flushes > - 'l1tf=full' : start performing L1D flushes, > warn on VM start if SMT has > been runtime enabled > - 'l1tf=novirt' : warn on first VM start > - 'l1tf=novirt,nowarn': no extra action is taken > > This makes the KVM's private 'nosmt' option redundant, and as it is a bit > non-systematic anyway (this is something to control globally, not on > hypervisor level). Remove that option. s/the KVM's /KVM's > + l1tf= [X86] Control mitigation of the L1TF vulnerability on > + affected CPUs > + > + The kernel PTE inversion protection is unconditionally > + enabled and cannot be controlled. s/controlled /disabled ? > + > + full > + Provide all available mitigations for the > + L1TF vulnerability. Disable SMT and enable > + all mitigations in the hypervisors. SMT > + control is still possible after boot. > + > + full,force > + Same as 'full', but disables SMT > + control. Implies the 'nosmt=force' command > + line option. > + > + novirt > + Leaves SMT enabled and does not enable the > + hypervisor mitigation. Hypervisors will > + issue a warning when the first VM is being > + started in a potentially insecure > + configuration, i.e. SMT enabled or L1D > + flush disabled. > + > + novirt,nowarn > + Same as 'novirt', but hypervisors will not > + warn when a VM is started in a potentially > + insecure configuration. > + > + Default is 'novirt'. Also, to extent you agree to the tweaks of the text I gave above, they should propagate into the documentation here as well. > +/* Default mitigation for L1TF-affected CPUs */ > +enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_NOVIRT; > +#if IS_ENABLED(CONFIG_KVM_INTEL) Is there any difference to #ifdef CONFIG_KVM_INTEL? > + switch (l1tf_mitigation) { > + case L1TF_MITIGATION_FULL_FORCE: > + cpu_smt_disable(true); > + break; > + case L1TF_MITIGATION_FULL: > + cpu_smt_disable(false); > + break; > + case L1TF_MITIGATION_NOVIRT: > + case L1TF_MITIGATION_NOVIRT_NOWARN: > + break; > + } Nit: maybe put this into the same order as the definition order, i.e. weaker-to-stronger options? > +static int __init l1tf_cmdline(char *str) > +{ > + if (!boot_cpu_has_bug(X86_BUG_L1TF)) > + return 0; > + > + if (!str) > + return 0; > + > + if (!strcmp(str, "full")) > + l1tf_mitigation = L1TF_MITIGATION_FULL; > + else if (!strcmp(str, "full,force")) > + l1tf_mitigation = L1TF_MITIGATION_FULL_FORCE; > + else if (!strcmp(str, "novirt")) > + l1tf_mitigation = L1TF_MITIGATION_NOVIRT; > + else if (!strcmp(str, "novirt,nowarn")) > + l1tf_mitigation = L1TF_MITIGATION_NOVIRT_NOWARN; Ditto. > +static const char *l1tf_states[] = { > + [L1TF_MITIGATION_FULL] = "Mitigation: Full", > + [L1TF_MITIGATION_FULL_FORCE] = "Mitigation: Full [force]", > + [L1TF_MITIGATION_NOVIRT] = "Mitigation: Page Table Inversion", > + [L1TF_MITIGATION_NOVIRT_NOWARN] = "Mitigation: Page Table Inversion" > +}; Ditto. > + > +#if IS_ENABLED(CONFIG_KVM_INTEL) > +static const char *l1tf_vmx_states[] = { > + [VMENTER_L1D_FLUSH_NEVER] = "vulnerable", > + [VMENTER_L1D_FLUSH_COND] = "mostly protected", > + [VMENTER_L1D_FLUSH_ALWAYS] = "fully protected" > +}; (This has the right order.) > + if (boot_cpu_has(X86_BUG_L1TF)) { > + switch (l1tf_mitigation) { > + case L1TF_MITIGATION_NOVIRT_NOWARN: > + /* The 'I explicitly don't care' flag is set */ > + break; > + case L1TF_MITIGATION_NOVIRT: > + /* > + * Warn upon starting the first VM in a > + * potentially insecure environment. > + */ > + if (cpu_smt_control == CPU_SMT_ENABLED || > + vmentry_l1d_flush == VMENTER_L1D_FLUSH_NEVER) > + pr_warn_once(L1TF_MSG_NOVIRT); > + break; > + case L1TF_MITIGATION_FULL: > + /* > + * Warn if SMT has been runtime-enabled. We can't > + * really warn only once here, as SMT state is > + * not constant. > + */ > + if (cpu_smt_control == CPU_SMT_ENABLED) > + pr_warn(L1TF_MSG_SMT); > + break; > + case L1TF_MITIGATION_FULL_FORCE: > + /* All is good, proceed without hiccup */ > + break; This has the definition-order too. So I'd suggest harmonizing the order. > + /* Take the l1tf command line parameter into account */ Nit: s/the l1tf command line parameter /the l1tf= command line parameter Just so that people know which exact parameter this is. > extern enum cpuhp_smt_control cpu_smt_control; > +void cpu_smt_disable(bool force); Should perhaps be 'extern' too, like the other prototypes? LGTM otherwise: Reviewed-by: Ingo Molnar Thanks, Ingo