From mboxrd@z Thu Jan  1 00:00:00 1970
Return-path: <ak@linux.intel.com>
Received: from mga01.intel.com ([192.55.52.88])
	by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256)
	(Exim 4.80)
	(envelope-from <ak@linux.intel.com>)
	id 1fGqvt-00025n-Jx
	for speck@linutronix.de; Thu, 10 May 2018 21:08:54 +0200
Date: Thu, 10 May 2018 12:08:50 -0700
From: Andi Kleen <ak@linux.intel.com>
Subject: [MODERATED] Re: [patch V11 05/16] SSB 5
Message-ID: <20180510190850.GE13616@tassilo.jf.intel.com>
References: <20180502215102.192655950@linutronix.de>
 <20180502215416.459915781@linutronix.de>
 <20180510175257.GD13616@tassilo.jf.intel.com>
 <20180510183058.GJ27358@char.us.oracle.com>
MIME-Version: 1.0
In-Reply-To: <20180510183058.GJ27358@char.us.oracle.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
To: speck@linutronix.de
List-ID: <speck.linutronix.de>

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