RE: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Thomas Gleixner]

On Tue, 31 Jul 2018, Prakhya, Sai Praneeth wrote:
> > > +#define X86_FEATURE_IBRS_ENHANCED		( 7*32+29) /*
> > "ibrs_enhanced" Use Enhanced IBRS in kernel */
> > 
> > That "ibrs_enhanced" part is not needed.
> 
> Just wanted to confirm with you before removing it, 
> Presently, on platforms that support features like arch_capabilities, stibp, ibrs and ibpb 
> /proc/cpuinfo does show them. Do you think we should really skip showing 
> Enhanced IBRS capability?

The feature strings are automatically generated from the define. The
comment can be used to supress them by an empty "" string or to modify them
by a "override" string at the beginning of the comment.

> > > +	/*
> > > +	 * As we don't use IBRS in kernel, nobody should have set
> > > +	 * SPEC_CTRL_IBRS until now. Shout loud if somebody did enable
> > > +	 * SPEC_CTRL_IBRS before us.
> > > +	 */
> > 
> > This comment does not make sense. What prevents the BIOS/bootloader or the
> > hypervisor from setting it?
> >
> 
> Makes sense. Removed it from V3.
> 
> > > +	WARN_ON_ONCE(x86_spec_ctrl_base & SPEC_CTRL_IBRS);

Please remove the warnon as well.

> > > +	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
> > > +	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> > 
> > And what exactly writes the MSR?
> >
> 
> While booting, x86_spec_ctrl_setup_ap() does that and after VMEXIT 
> x86_spec_ctrl_restore_host().
> 
> As x86_spec_ctrl_setup_ap() does wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base), 
> I thought writing here would be redundant.

x86_spec_ctrl_setup_ap() is only called on the AP but not on the BP. So the
boot processor will not have it set, unless something else writes the
MSR. So you really want to have an explicit write there.

Thanks,

	tglx