[MODERATED] Re: [patch V11 05/16] SSB 5

[Andi Kleen <ak@linux.intel.com>]

On Thu, May 10, 2018 at 02:30:58PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> On Thu, May 10, 2018 at 10:52:57AM -0700, speck for Andi Kleen wrote:
> > Hi,
> > 
> > I went over this patch again and I'm not sure i understand
> > how it works.
> > 
> > 
> > On Wed, May 02, 2018 at 11:51:07PM +0200, speck for Thomas Gleixner wrote:
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -9720,8 +9720,7 @@ static void __noclone vmx_vcpu_run(struc
> > >  	 * is no need to worry about the conditional branch over the wrmsr
> > >  	 * being speculatively taken.
> > >  	 */
> > > -	if (vmx->spec_ctrl)
> > > -		native_wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > > +	x86_spec_ctrl_set_guest(vmx->spec_ctrl);
> > >  
> > >  	vmx->__launched = vmx->loaded_vmcs->launched;
> > >  
> > > @@ -9869,8 +9868,7 @@ static void __noclone vmx_vcpu_run(struc
> > >  	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
> > >  		vmx->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
> > >  
> > > -	if (vmx->spec_ctrl)
> > > -		native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> > > +	x86_spec_ctrl_restore_host(vmx->spec_ctrl);
> > 
> > So we assume that vmx->spec_ctrl is always the correct value of the guest.
> > 
> > 
> > But vmx_set_msr does
> 
> .. only if we intercept it.
> > 
> > 	        vmx->spec_ctrl = data;
> > 
> >                 if (!data)
> >                         break;
> > 
> >                 /*
> >                  * For non-nested:
> >                  * When it's written (to non-zero) for the first time, pass
> >                  * it through.
> >                  *
> >                  * For nested:
> >                  * The handling of the MSR bitmap for L2 guests is done in
> >                  * nested_vmx_merge_msr_bitmap. We should not touch the
> >                  * vmcs02.msr_bitmap here since it gets completely overwritten
> >                  * in the merging. We update the vmcs01 here for L1 as well
> >                  * since it will end up touching the MSR anyway now.
> >                  */
> >                 vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap,
> >                                               MSR_IA32_SPEC_CTRL,
> >                                               MSR_TYPE_RW);
> > 
> > That means only the value of the first write in the guest is saved in vmx->spec_ctrl.
> > 
> > But what happens when a later write is different from the first write?
> 
> This code:
> 
>   	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
>   		vmx->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
> 
> Would read it on the VMEXITs.

Thanks. I missed that. Good that it works.

Why don't we use the msr save/restore lists for that? That would likely
be faster than a manual msr accesses.

-Andi