From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from hsi-kbw-5-158-153-52.hsi19.kabel-badenwuerttemberg.de ([5.158.153.52] helo=nanos.tec.linutronix.de) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fJEXV-00040q-SV for speck@linutronix.de; Thu, 17 May 2018 10:45:34 +0200 Date: Thu, 17 May 2018 10:45:30 +0200 (CEST) From: Thomas Gleixner Subject: Re: [patch 13/15] SSB updates V17 13 In-Reply-To: <20180517020804.GJ10272@char.us.oracle.com> Message-ID: References: <20180516135132.687640705@linutronix.de> <20180516135210.411897243@linutronix.de> <20180517020804.GJ10272@char.us.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: 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 > > > > 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); } }