Date: Wed, 16 May 2018 22:08:04 -0400
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: [MODERATED] Re: [patch 13/15] SSB updates V17 13
Message-ID: <20180517020804.GJ10272@char.us.oracle.com>
References: <20180516135132.687640705@linutronix.de>
 <20180516135210.411897243@linutronix.de>
MIME-Version: 1.0
In-Reply-To: <20180516135210.411897243@linutronix.de>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
To: speck@linutronix.de

On Wed, May 16, 2018 at 03:51:45PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch 13/15] x86/bugs: Rework spec_ctrl base and mask logic
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> x86_spec_ctrL_mask is intended to mask out bits from a MSR_SPEC_CTRL value
> which are not to be modified. However the implementation is not really used
> and the bitmask is inverted for no real reason. Aside of that it is missing

It is inverted to make the check easier. That is the code (but removed in the prior
patch) had:

	if (val & x86_spec_ctrl_mask)
		complain..

Perhaps: s/for no real reason/to make the check easier - but removed in
"x86/bugs: Remove x86_spec_ctrl_set()"/ ?

> the STIBP bit if it is supported by the platform, so if the mask would be
> used in x86_virt_spec_ctrl() then it would prevent a guest from setting
> STIBP.
> 
> Add the STIBP bit if supported and use the mask in x86_spec_ctrl_set_guest()

s/x86_spec_ctrl_set_guest/x86_virt_spec_ctrl/

> to sanitize the value which is supplied by the guest.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> ---
> V2: Moved the stray x86_spec_ctrl_base inversion hunk from patch 11/15
>     Renamed some variables so it's more clear that they are MSR values
>     and not MSR indices.
> ---
>  arch/x86/kernel/cpu/bugs.c |   28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -42,7 +42,7 @@ EXPORT_SYMBOL_GPL(x86_spec_ctrl_base);
>   * The vendor and possibly platform specific bits which can be modified in
>   * x86_spec_ctrl_base.
>   */
> -static u64 __ro_after_init x86_spec_ctrl_mask = ~SPEC_CTRL_IBRS;
> +static u64 __ro_after_init x86_spec_ctrl_mask = SPEC_CTRL_IBRS;
>  
>  /*
>   * AMD specific MSR info for Speculative Store Bypass control.
> @@ -68,6 +68,10 @@ void __init check_bugs(void)
>  	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
>  		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>  
> +	/* Allow STIBP in MSR_SPEC_CTRL if supported */
> +	if (boot_cpu_has(X86_FEATURE_STIBP))
> +		x86_spec_ctrl_mask |= SPEC_CTRL_STIBP;
> +
>  	/* Select the proper spectre mitigation before patching alternatives */
>  	spectre_v2_select_mitigation();
>  
> @@ -134,20 +138,28 @@ static enum spectre_v2_mitigation spectr
>  	SPECTRE_V2_NONE;
>  
>  void
> -x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool guest)
> +x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool setguest)

Would it make sense to roll in the s/guest/setguest/ in the
"x86/bugs: Unify x86_spec_ctrl_{set_guest,restore_host}" patch?

>  {
> +	u64 msrval, guestval, hostval = x86_spec_ctrl_base;
>  	struct thread_info *ti = current_thread_info();
> -	u64 msr, host = x86_spec_ctrl_base;
>  
>  	/* Is MSR_SPEC_CTRL implemented ? */
>  	if (static_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
> +		/*
> +		 * Restrict guest_spec_ctrl to supported values. Clear the
> +		 * modifiable bits in the host base value and or the
> +		 * modifiable bits from the guest value.
> +		 */
> +		guestval = hostval & ~x86_spec_ctrl_mask;
> +		guestval |= guest_spec_ctrl & x86_spec_ctrl_mask;

How come the guestval is not used at all in this patch?

> +
>  		/* SSBD controlled in MSR_SPEC_CTRL */
>  		if (static_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD))
> -			host |= ssbd_tif_to_spec_ctrl(ti->flags);
> +			hostval |= ssbd_tif_to_spec_ctrl(ti->flags);
>  
> -		if (host != guest_spec_ctrl) {
> -			msr = guest ? guest_spec_ctrl : host;
> -			wrmsrl(MSR_IA32_SPEC_CTRL, msr);
> +		if (hostval != guest_spec_ctrl) {
> +			msrval = setguest ? guest_spec_ctrl : hostval;
> +			wrmsrl(MSR_IA32_SPEC_CTRL, msrval);
>  		}
>  	}
>  }
> @@ -493,7 +505,7 @@ static enum ssb_mitigation __init __ssb_
>  		switch (boot_cpu_data.x86_vendor) {
>  		case X86_VENDOR_INTEL:
>  			x86_spec_ctrl_base |= SPEC_CTRL_SSBD;
> -			x86_spec_ctrl_mask &= ~SPEC_CTRL_SSBD;
> +			x86_spec_ctrl_mask |= SPEC_CTRL_SSBD;
>  			wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>  			break;
>  		case X86_VENDOR_AMD:
>