Re: [patch 13/15] SSB updates V17 13

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 16 May 2018, speck for Konrad Rzeszutek Wilk wrote:
> 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()"/ ?

Fixed.

> > 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/

Fixed.

> >  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?

Done.

> >  {
> > +	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?

Because I'm a moron. Delta fix below.

Thanks,

	tglx
8<-------------------------

--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -157,8 +157,8 @@ x86_virt_spec_ctrl(u64 guest_spec_ctrl,
 		if (static_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD))
 			hostval |= ssbd_tif_to_spec_ctrl(ti->flags);
 
-		if (hostval != guest_spec_ctrl) {
-			msrval = setguest ? guest_spec_ctrl : hostval;
+		if (hostval != guestval) {
+			msrval = setguest ? guestval : hostval;
 			wrmsrl(MSR_IA32_SPEC_CTRL, msrval);
 		}
 	}