[MODERATED] Re: [patch 1/2] Command line and documentation 1

[Ingo Molnar <mingo@kernel.org>]

* speck for Thomas Gleixner <speck@linutronix.de> wrote:

> From: Jiri Kosina <jkosina@suse.cz>
> 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 <mingo@kernel.org>

Thanks,

	Ingo