Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

On 11/01/2018 03:47, Tim Chen wrote:
> On 01/08/2018 10:08 AM, Paolo Bonzini wrote:
> 
>> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
>> *vcpu)
>>  
>>  pt_guest_enter(vmx);
>>  
>> +if (have_spec_ctrl && vmx->spec_ctrl != 0)
>> +wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
>> +
> 
> Say host uses IBRS (have_spec_ctrl==1), and have IBRS set to 1. 
> And say guest's vmx->spec_ctrl is 0 and not using IBRS.
> 
> We will be leaving IBRS msr as 1 and won't be
> switching it to 0 before entering guest.
> Guest will be running with incorrect IBRS value.
> 
> Seems like the correct logic is 
> 
> if (host_supports_ibrs)
>   /* switch IBRS if host and guest ibrs differs */
>   if ((host_uses_ibrs && vmx->spec_ctrl == 0) ||/* host IBRS 1, guest 
> IBRS 0 */
>   (!host_uses_ibrs && vmx->spec_ctrl == 1)) /* host IBRS 0, guest 
> IBRS 1 */
>   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> }
> 
> Your have_spec_ctrl logic specifies that IBRS is available.
> But that doesn't necessarily mean that we will use IBRS in
> host.

Of course.  But these patches are just an initial version for a host
that doesn't support IBRS.

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

On 11/01/2018 03:47, Tim Chen wrote:
> On 01/08/2018 10:08 AM, Paolo Bonzini wrote:
> 
>> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
>> *vcpu)
>>  
>>  pt_guest_enter(vmx);
>>  
>> +if (have_spec_ctrl && vmx->spec_ctrl != 0)
>> +wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
>> +
> 
> Say host uses IBRS (have_spec_ctrl==1), and have IBRS set to 1. 
> And say guest's vmx->spec_ctrl is 0 and not using IBRS.
> 
> We will be leaving IBRS msr as 1 and won't be
> switching it to 0 before entering guest.
> Guest will be running with incorrect IBRS value.
> 
> Seems like the correct logic is 
> 
> if (host_supports_ibrs)
>   /* switch IBRS if host and guest ibrs differs */
>   if ((host_uses_ibrs && vmx->spec_ctrl == 0) ||/* host IBRS 1, guest 
> IBRS 0 */
>   (!host_uses_ibrs && vmx->spec_ctrl == 1)) /* host IBRS 0, guest 
> IBRS 1 */
>   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> }
> 
> Your have_spec_ctrl logic specifies that IBRS is available.
> But that doesn't necessarily mean that we will use IBRS in
> host.

Of course.  But these patches are just an initial version for a host
that doesn't support IBRS.

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Tim Chen]

On 01/08/2018 10:08 AM, Paolo Bonzini wrote:

> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  
>   pt_guest_enter(vmx);
>  
> + if (have_spec_ctrl && vmx->spec_ctrl != 0)
> + wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Say host uses IBRS (have_spec_ctrl==1), and have IBRS set to 1. 
And say guest's vmx->spec_ctrl is 0 and not using IBRS.

We will be leaving IBRS msr as 1 and won't be
switching it to 0 before entering guest.
Guest will be running with incorrect IBRS value.

Seems like the correct logic is 

if (host_supports_ibrs)
/* switch IBRS if host and guest ibrs differs */
if ((host_uses_ibrs && vmx->spec_ctrl == 0) ||/* host IBRS 1, guest 
IBRS 0 */
(!host_uses_ibrs && vmx->spec_ctrl == 1)) /* host IBRS 0, guest 
IBRS 1 */
wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
}

Your have_spec_ctrl logic specifies that IBRS is available.
But that doesn't necessarily mean that we will use IBRS in
host.

Tim

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Tim Chen]

On 01/08/2018 10:08 AM, Paolo Bonzini wrote:

> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  
>   pt_guest_enter(vmx);
>  
> + if (have_spec_ctrl && vmx->spec_ctrl != 0)
> + wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Say host uses IBRS (have_spec_ctrl==1), and have IBRS set to 1. 
And say guest's vmx->spec_ctrl is 0 and not using IBRS.

We will be leaving IBRS msr as 1 and won't be
switching it to 0 before entering guest.
Guest will be running with incorrect IBRS value.

Seems like the correct logic is 

if (host_supports_ibrs)
/* switch IBRS if host and guest ibrs differs */
if ((host_uses_ibrs && vmx->spec_ctrl == 0) ||/* host IBRS 1, guest 
IBRS 0 */
(!host_uses_ibrs && vmx->spec_ctrl == 1)) /* host IBRS 0, guest 
IBRS 1 */
wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
}

Your have_spec_ctrl logic specifies that IBRS is available.
But that doesn't necessarily mean that we will use IBRS in
host.

Tim

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

On 09/01/2018 00:19, Jim Mattson wrote:
 +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
 +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>>> I have a lot of changes to MSR permission bitmap handling, but these
>>> intercepts should only be disabled when guest_cpuid_has(vcpu,
>>> X86_FEATURE_SPEC_CTRL).
>> That's harder to backport and not strictly necessary (just like
>> e.g. we don't check guest CPUID bits before emulation).  I agree that
>> your version is better, but I think the above is fine as a minimal
>> fix.
> 
> Due to the impacts that spec_ctrl has on the neighboring hyperthread,
> one may want to disable MSRs 0x48 and 0x49 for a particular VM. We do
> this by masking off CPUID.(EAX=7, ECX=0).EDX[26] and CPUID.(EAX=7,
> ECX=0).EDX[27] from the userspace agent. However, with your patch,
> *any* VCPU gets unrestricted access to these MSRs, without any
> mechanism for disabling them.

Yes, I agree that having the check is superior.  However, I also want to
get there step by step.

 +   if (have_spec_ctrl) {
 +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
 +   if (vmx->spec_ctrl)
 +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
 +   }
 +
>>>
>>> I know the VM-exit MSR load and store lists are probably slower, but
>>> I'm a little uncomfortable restoring the host's IA32_SPEC_CTRL MSR
>>> late if the guest has it clear and the host has it set.
>>
>> There is no indirect branch before though, isn't it?
>
> I guess that depends on how you define "before."

--verbose? :-/

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

On 09/01/2018 00:19, Jim Mattson wrote:
 +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
 +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>>> I have a lot of changes to MSR permission bitmap handling, but these
>>> intercepts should only be disabled when guest_cpuid_has(vcpu,
>>> X86_FEATURE_SPEC_CTRL).
>> That's harder to backport and not strictly necessary (just like
>> e.g. we don't check guest CPUID bits before emulation).  I agree that
>> your version is better, but I think the above is fine as a minimal
>> fix.
> 
> Due to the impacts that spec_ctrl has on the neighboring hyperthread,
> one may want to disable MSRs 0x48 and 0x49 for a particular VM. We do
> this by masking off CPUID.(EAX=7, ECX=0).EDX[26] and CPUID.(EAX=7,
> ECX=0).EDX[27] from the userspace agent. However, with your patch,
> *any* VCPU gets unrestricted access to these MSRs, without any
> mechanism for disabling them.

Yes, I agree that having the check is superior.  However, I also want to
get there step by step.

 +   if (have_spec_ctrl) {
 +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
 +   if (vmx->spec_ctrl)
 +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
 +   }
 +
>>>
>>> I know the VM-exit MSR load and store lists are probably slower, but
>>> I'm a little uncomfortable restoring the host's IA32_SPEC_CTRL MSR
>>> late if the guest has it clear and the host has it set.
>>
>> There is no indirect branch before though, isn't it?
>
> I guess that depends on how you define "before."

--verbose? :-/

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

On 09/01/2018 00:58, Liran Alon wrote:
> 
> - pbonz...@redhat.com wrote:
> 
>> - Original Message -
>>> From: "David Woodhouse" <dw...@infradead.org>
>>> To: "Paolo Bonzini" <pbonz...@redhat.com>,
>> linux-kernel@vger.kernel.org, k...@vger.kernel.org
>>> Cc: jmatt...@google.com, aligu...@amazon.com, "thomas lendacky"
>> <thomas.lenda...@amd.com>, b...@alien8.de
>>> Sent: Monday, January 8, 2018 8:41:07 PM
>>> Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and
>> MSR_IA32_PRED_CMD down to the guest
>>>
>>> On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
>>>>
>>>> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
>>>> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
>>>> +
>>>
>>> I think this one probably *is* safe even without an 'else lfence',
>>> which means that the CPU can speculate around it, but it wants a
>>> comment explaining that someone has properly analysed it and saying
>>> precisely why.
>>
>> This one is okay as long as there are no indirect jumps until
>> vmresume.  But the one on vmexit is only okay because right now
>> it's *disabling* IBRS.  Once IBRS is used by Linux, we'll need an
>> lfence there.  I'll add a comment.
>>
>> Paolo
> 
> That is true but from what I understand, there is an indirect branch from 
> this point until vmresume.
> That indirect branch resides in atomic_switch_perf_msrs() immediately called 
> after this WRMSR:
> atomic_switch_perf_msrs() -> perf_guest_get_msrs() -> 
> x86_pmu.guest_get_msrs().

Sure, it has to move later as pointed out by other reviewers.

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

On 09/01/2018 00:58, Liran Alon wrote:
> 
> - pbonz...@redhat.com wrote:
> 
>> - Original Message -
>>> From: "David Woodhouse" 
>>> To: "Paolo Bonzini" ,
>> linux-kernel@vger.kernel.org, k...@vger.kernel.org
>>> Cc: jmatt...@google.com, aligu...@amazon.com, "thomas lendacky"
>> , b...@alien8.de
>>> Sent: Monday, January 8, 2018 8:41:07 PM
>>> Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and
>> MSR_IA32_PRED_CMD down to the guest
>>>
>>> On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
>>>>
>>>> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
>>>> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
>>>> +
>>>
>>> I think this one probably *is* safe even without an 'else lfence',
>>> which means that the CPU can speculate around it, but it wants a
>>> comment explaining that someone has properly analysed it and saying
>>> precisely why.
>>
>> This one is okay as long as there are no indirect jumps until
>> vmresume.  But the one on vmexit is only okay because right now
>> it's *disabling* IBRS.  Once IBRS is used by Linux, we'll need an
>> lfence there.  I'll add a comment.
>>
>> Paolo
> 
> That is true but from what I understand, there is an indirect branch from 
> this point until vmresume.
> That indirect branch resides in atomic_switch_perf_msrs() immediately called 
> after this WRMSR:
> atomic_switch_perf_msrs() -> perf_guest_get_msrs() -> 
> x86_pmu.guest_get_msrs().

Sure, it has to move later as pointed out by other reviewers.

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Liran Alon]

- pbonz...@redhat.com wrote:

> - Original Message -
> > From: "David Woodhouse" <dw...@infradead.org>
> > To: "Paolo Bonzini" <pbonz...@redhat.com>,
> linux-kernel@vger.kernel.org, k...@vger.kernel.org
> > Cc: jmatt...@google.com, aligu...@amazon.com, "thomas lendacky"
> <thomas.lenda...@amd.com>, b...@alien8.de
> > Sent: Monday, January 8, 2018 8:41:07 PM
> > Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and
> MSR_IA32_PRED_CMD down to the guest
> > 
> > On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
> > > 
> > > +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> > > +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > > +
> > 
> > I think this one probably *is* safe even without an 'else lfence',
> > which means that the CPU can speculate around it, but it wants a
> > comment explaining that someone has properly analysed it and saying
> > precisely why.
> 
> This one is okay as long as there are no indirect jumps until
> vmresume.  But the one on vmexit is only okay because right now
> it's *disabling* IBRS.  Once IBRS is used by Linux, we'll need an
> lfence there.  I'll add a comment.
> 
> Paolo

That is true but from what I understand, there is an indirect branch from this 
point until vmresume.
That indirect branch resides in atomic_switch_perf_msrs() immediately called 
after this WRMSR:
atomic_switch_perf_msrs() -> perf_guest_get_msrs() -> x86_pmu.guest_get_msrs().

-Liran

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Liran Alon]

- pbonz...@redhat.com wrote:

> - Original Message -
> > From: "David Woodhouse" 
> > To: "Paolo Bonzini" ,
> linux-kernel@vger.kernel.org, k...@vger.kernel.org
> > Cc: jmatt...@google.com, aligu...@amazon.com, "thomas lendacky"
> , b...@alien8.de
> > Sent: Monday, January 8, 2018 8:41:07 PM
> > Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and
> MSR_IA32_PRED_CMD down to the guest
> > 
> > On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
> > > 
> > > +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> > > +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > > +
> > 
> > I think this one probably *is* safe even without an 'else lfence',
> > which means that the CPU can speculate around it, but it wants a
> > comment explaining that someone has properly analysed it and saying
> > precisely why.
> 
> This one is okay as long as there are no indirect jumps until
> vmresume.  But the one on vmexit is only okay because right now
> it's *disabling* IBRS.  Once IBRS is used by Linux, we'll need an
> lfence there.  I'll add a comment.
> 
> Paolo

That is true but from what I understand, there is an indirect branch from this 
point until vmresume.
That indirect branch resides in atomic_switch_perf_msrs() immediately called 
after this WRMSR:
atomic_switch_perf_msrs() -> perf_guest_get_msrs() -> x86_pmu.guest_get_msrs().

-Liran

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Jim Mattson]

On Mon, Jan 8, 2018 at 2:32 PM, Paolo Bonzini  wrote:
>
>> I have:
>>
>> if (!have_spec_ctrl ||
>> (!msr_info->host_initiated &&
>>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> return 1;
>> msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
>> break;
>
>> I have:
>>
>> if (!have_spec_ctrl ||
>> (!msr_info->host_initiated &&
>>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> return 1;
>> to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
>> break;
>
> Both better than mine.
>
>> > +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
>> > +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>>
>> I have a lot of changes to MSR permission bitmap handling, but these
>> intercepts should only be disabled when guest_cpuid_has(vcpu,
>> X86_FEATURE_SPEC_CTRL).
>
> That's harder to backport and not strictly necessary (just like
> e.g. we don't check guest CPUID bits before emulation).  I agree that
> your version is better, but I think the above is fine as a minimal
> fix.

Due to the impacts that spec_ctrl has on the neighboring hyperthread,
one may want to disable MSRs 0x48 and 0x49 for a particular VM. We do
this by masking off CPUID.(EAX=7, ECX=0).EDX[26] and CPUID.(EAX=7,
ECX=0).EDX[27] from the userspace agent. However, with your patch,
*any* VCPU gets unrestricted access to these MSRs, without any
mechanism for disabling them.

>> Here, I have:
>>
>> /*
>> * If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
>> * store it on VM-exit.
>> */
>> if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
>> add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
>> else
>> clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
>>
>> /*
>> * If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
>> * IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
>> * MSRs, so that the guest value will be loaded on VM-entry
>> * and the host value will be loaded on VM-exit.
>> */
>> if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
>> add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
>>   vmx->msr_ia32_spec_ctrl,
>>   spec_ctrl_enabled());
>> else
>> clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);
>>
>> > atomic_switch_perf_msrs(vmx);
>> >
>> > vmx_arm_hv_timer(vcpu);
>> > @@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu
>> > *vcpu)
>> >  #endif
>> >   );
>> >
>> > +   if (have_spec_ctrl) {
>> > +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
>> > +   if (vmx->spec_ctrl)
>> > +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
>> > +   }
>> > +
>>
>> I know the VM-exit MSR load and store lists are probably slower, but
>> I'm a little uncomfortable restoring the host's IA32_SPEC_CTRL MSR
>> late if the guest has it clear and the host has it set.
>
> There is no indirect branch before though, isn't it?

I guess that depends on how you define "before."

> Paolo
>
>> > /* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed
>> > */
>> > if (vmx->host_debugctlmsr)
>> > update_debugctlmsr(vmx->host_debugctlmsr);
>> > --
>> > 1.8.3.1
>> >
>> >
>>

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Jim Mattson]

On Mon, Jan 8, 2018 at 2:32 PM, Paolo Bonzini  wrote:
>
>> I have:
>>
>> if (!have_spec_ctrl ||
>> (!msr_info->host_initiated &&
>>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> return 1;
>> msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
>> break;
>
>> I have:
>>
>> if (!have_spec_ctrl ||
>> (!msr_info->host_initiated &&
>>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
>> return 1;
>> to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
>> break;
>
> Both better than mine.
>
>> > +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
>> > +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>>
>> I have a lot of changes to MSR permission bitmap handling, but these
>> intercepts should only be disabled when guest_cpuid_has(vcpu,
>> X86_FEATURE_SPEC_CTRL).
>
> That's harder to backport and not strictly necessary (just like
> e.g. we don't check guest CPUID bits before emulation).  I agree that
> your version is better, but I think the above is fine as a minimal
> fix.

Due to the impacts that spec_ctrl has on the neighboring hyperthread,
one may want to disable MSRs 0x48 and 0x49 for a particular VM. We do
this by masking off CPUID.(EAX=7, ECX=0).EDX[26] and CPUID.(EAX=7,
ECX=0).EDX[27] from the userspace agent. However, with your patch,
*any* VCPU gets unrestricted access to these MSRs, without any
mechanism for disabling them.

>> Here, I have:
>>
>> /*
>> * If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
>> * store it on VM-exit.
>> */
>> if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
>> add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
>> else
>> clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
>>
>> /*
>> * If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
>> * IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
>> * MSRs, so that the guest value will be loaded on VM-entry
>> * and the host value will be loaded on VM-exit.
>> */
>> if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
>> add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
>>   vmx->msr_ia32_spec_ctrl,
>>   spec_ctrl_enabled());
>> else
>> clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);
>>
>> > atomic_switch_perf_msrs(vmx);
>> >
>> > vmx_arm_hv_timer(vcpu);
>> > @@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu
>> > *vcpu)
>> >  #endif
>> >   );
>> >
>> > +   if (have_spec_ctrl) {
>> > +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
>> > +   if (vmx->spec_ctrl)
>> > +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
>> > +   }
>> > +
>>
>> I know the VM-exit MSR load and store lists are probably slower, but
>> I'm a little uncomfortable restoring the host's IA32_SPEC_CTRL MSR
>> late if the guest has it clear and the host has it set.
>
> There is no indirect branch before though, isn't it?

I guess that depends on how you define "before."

> Paolo
>
>> > /* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed
>> > */
>> > if (vmx->host_debugctlmsr)
>> > update_debugctlmsr(vmx->host_debugctlmsr);
>> > --
>> > 1.8.3.1
>> >
>> >
>>

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

- Original Message -
> From: "David Woodhouse" <dw...@infradead.org>
> To: "Paolo Bonzini" <pbonz...@redhat.com>, linux-kernel@vger.kernel.org, 
> k...@vger.kernel.org
> Cc: jmatt...@google.com, aligu...@amazon.com, "thomas lendacky" 
> <thomas.lenda...@amd.com>, b...@alien8.de
> Sent: Monday, January 8, 2018 8:41:07 PM
> Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and 
> MSR_IA32_PRED_CMD down to the guest
> 
> On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
> > 
> > +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> > +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > +
> 
> I think this one probably *is* safe even without an 'else lfence',
> which means that the CPU can speculate around it, but it wants a
> comment explaining that someone has properly analysed it and saying
> precisely why.

This one is okay as long as there are no indirect jumps until
vmresume.  But the one on vmexit is only okay because right now
it's *disabling* IBRS.  Once IBRS is used by Linux, we'll need an
lfence there.  I'll add a comment.

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

- Original Message -
> From: "David Woodhouse" 
> To: "Paolo Bonzini" , linux-kernel@vger.kernel.org, 
> k...@vger.kernel.org
> Cc: jmatt...@google.com, aligu...@amazon.com, "thomas lendacky" 
> , b...@alien8.de
> Sent: Monday, January 8, 2018 8:41:07 PM
> Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and 
> MSR_IA32_PRED_CMD down to the guest
> 
> On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
> > 
> > +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> > +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > +
> 
> I think this one probably *is* safe even without an 'else lfence',
> which means that the CPU can speculate around it, but it wants a
> comment explaining that someone has properly analysed it and saying
> precisely why.

This one is okay as long as there are no indirect jumps until
vmresume.  But the one on vmexit is only okay because right now
it's *disabling* IBRS.  Once IBRS is used by Linux, we'll need an
lfence there.  I'll add a comment.

Paolo

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

> I have:
> 
> if (!have_spec_ctrl ||
> (!msr_info->host_initiated &&
>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
> return 1;
> msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
> break;

> I have:
> 
> if (!have_spec_ctrl ||
> (!msr_info->host_initiated &&
>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
> return 1;
> to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
> break;

Both better than mine.

> > +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> > +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
> 
> I have a lot of changes to MSR permission bitmap handling, but these
> intercepts should only be disabled when guest_cpuid_has(vcpu,
> X86_FEATURE_SPEC_CTRL).

That's harder to backport and not strictly necessary (just like
e.g. we don't check guest CPUID bits before emulation).  I agree that
your version is better, but I think the above is fine as a minimal
fix.

> Here, I have:
> 
> /*
> * If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
> * store it on VM-exit.
> */
> if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
> add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
> else
> clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
> 
> /*
> * If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
> * IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
> * MSRs, so that the guest value will be loaded on VM-entry
> * and the host value will be loaded on VM-exit.
> */
> if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
> add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
>   vmx->msr_ia32_spec_ctrl,
>   spec_ctrl_enabled());
> else
> clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);
>
> > atomic_switch_perf_msrs(vmx);
> >
> > vmx_arm_hv_timer(vcpu);
> > @@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu
> > *vcpu)
> >  #endif
> >   );
> >
> > +   if (have_spec_ctrl) {
> > +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > +   if (vmx->spec_ctrl)
> > +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> > +   }
> > +
> 
> I know the VM-exit MSR load and store lists are probably slower, but
> I'm a little uncomfortable restoring the host's IA32_SPEC_CTRL MSR
> late if the guest has it clear and the host has it set.

There is no indirect branch before though, isn't it?

Paolo

> > /* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed
> > */
> > if (vmx->host_debugctlmsr)
> > update_debugctlmsr(vmx->host_debugctlmsr);
> > --
> > 1.8.3.1
> >
> >
> 

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

> I have:
> 
> if (!have_spec_ctrl ||
> (!msr_info->host_initiated &&
>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
> return 1;
> msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
> break;

> I have:
> 
> if (!have_spec_ctrl ||
> (!msr_info->host_initiated &&
>  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
> return 1;
> to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
> break;

Both better than mine.

> > +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> > +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
> 
> I have a lot of changes to MSR permission bitmap handling, but these
> intercepts should only be disabled when guest_cpuid_has(vcpu,
> X86_FEATURE_SPEC_CTRL).

That's harder to backport and not strictly necessary (just like
e.g. we don't check guest CPUID bits before emulation).  I agree that
your version is better, but I think the above is fine as a minimal
fix.

> Here, I have:
> 
> /*
> * If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
> * store it on VM-exit.
> */
> if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
> add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
> else
> clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
> 
> /*
> * If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
> * IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
> * MSRs, so that the guest value will be loaded on VM-entry
> * and the host value will be loaded on VM-exit.
> */
> if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
> add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
>   vmx->msr_ia32_spec_ctrl,
>   spec_ctrl_enabled());
> else
> clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);
>
> > atomic_switch_perf_msrs(vmx);
> >
> > vmx_arm_hv_timer(vcpu);
> > @@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu
> > *vcpu)
> >  #endif
> >   );
> >
> > +   if (have_spec_ctrl) {
> > +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> > +   if (vmx->spec_ctrl)
> > +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> > +   }
> > +
> 
> I know the VM-exit MSR load and store lists are probably slower, but
> I'm a little uncomfortable restoring the host's IA32_SPEC_CTRL MSR
> late if the guest has it clear and the host has it set.

There is no indirect branch before though, isn't it?

Paolo

> > /* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed
> > */
> > if (vmx->host_debugctlmsr)
> > update_debugctlmsr(vmx->host_debugctlmsr);
> > --
> > 1.8.3.1
> >
> >
> 

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

- Original Message -
> From: "Ashok Raj" <lkml.a...@gmail.com>
> To: "Paolo Bonzini" <pbonz...@redhat.com>
> Cc: linux-kernel@vger.kernel.org, k...@vger.kernel.org, jmatt...@google.com, 
> aligu...@amazon.com, "thomas lendacky"
> <thomas.lenda...@amd.com>, d...@amazon.co.uk, b...@alien8.de, "Ashok Raj" 
> <ashok....@linux.intel.com>
> Sent: Monday, January 8, 2018 11:09:53 PM
> Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and 
> MSR_IA32_PRED_CMD down to the guest
> 
> Hi Paolo
> 
> Do you assume that host isn't using IBRS and only guest uses it?

For now, yes.

Patches to add the IBRS and IBPB cpufeatures will have to adjust the
MSR writes from this patch.

Paolo

> 
> 
> 
> On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini <pbonz...@redhat.com> wrote:
> > Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> > for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> > IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> > it yet).
> >
> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> > ---
> >  arch/x86/kvm/vmx.c | 32 
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 669f5f74857d..d00bcad7336e 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -120,6 +120,8 @@
> >  module_param_named(preemption_timer, enable_preemption_timer, bool,
> >  S_IRUGO);
> >  #endif
> >
> > +static bool __read_mostly have_spec_ctrl;
> > +
> >  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
> >  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
> >  #define KVM_VM_CR0_ALWAYS_ON   \
> > @@ -609,6 +611,8 @@ struct vcpu_vmx {
> > u64   msr_host_kernel_gs_base;
> > u64   msr_guest_kernel_gs_base;
> >  #endif
> > +   u64   spec_ctrl;
> > +
> > u32 vm_entry_controls_shadow;
> > u32 vm_exit_controls_shadow;
> > u32 secondary_exec_control;
> > @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct
> > msr_data *msr_info)
> > case MSR_IA32_TSC:
> > msr_info->data = guest_read_tsc(vcpu);
> > break;
> > +   case MSR_IA32_SPEC_CTRL:
> > +   msr_info->data = to_vmx(vcpu)->spec_ctrl;
> > +   break;
> > case MSR_IA32_SYSENTER_CS:
> > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
> > break;
> > @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct
> > msr_data *msr_info)
> > case MSR_IA32_TSC:
> > kvm_write_tsc(vcpu, msr_info);
> > break;
> > +   case MSR_IA32_SPEC_CTRL:
> > +   to_vmx(vcpu)->spec_ctrl = msr_info->data;
> > +   break;
> > case MSR_IA32_CR_PAT:
> > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> > @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
> > goto out;
> > }
> >
> > +   /*
> > +* FIXME: this is only needed until SPEC_CTRL is supported
> > +* by upstream Linux in cpufeatures, then it can be replaced
> > +* with static_cpu_has.
> > +*/
> > +   have_spec_ctrl = cpu_has_spec_ctrl();
> > +   if (have_spec_ctrl)
> > +   pr_info("kvm: SPEC_CTRL available\n");
> > +   else
> > +   pr_info("kvm: SPEC_CTRL not available\n");
> > +
> > if (boot_cpu_has(X86_FEATURE_NX))
> > kvm_enable_efer_bits(EFER_NX);
> >
> > @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
> > vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
> > vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> > vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> > +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> > +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
> >
> > memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
> > vmx_msr_bitmap_legacy, PAGE_SIZE);
> > @@ -9597,6 +962

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

- Original Message -
> From: "Ashok Raj" 
> To: "Paolo Bonzini" 
> Cc: linux-kernel@vger.kernel.org, k...@vger.kernel.org, jmatt...@google.com, 
> aligu...@amazon.com, "thomas lendacky"
> , d...@amazon.co.uk, b...@alien8.de, "Ashok Raj" 
> 
> Sent: Monday, January 8, 2018 11:09:53 PM
> Subject: Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and 
> MSR_IA32_PRED_CMD down to the guest
> 
> Hi Paolo
> 
> Do you assume that host isn't using IBRS and only guest uses it?

For now, yes.

Patches to add the IBRS and IBPB cpufeatures will have to adjust the
MSR writes from this patch.

Paolo

> 
> 
> 
> On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:
> > Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> > for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> > IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> > it yet).
> >
> > Signed-off-by: Paolo Bonzini 
> > ---
> >  arch/x86/kvm/vmx.c | 32 
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 669f5f74857d..d00bcad7336e 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -120,6 +120,8 @@
> >  module_param_named(preemption_timer, enable_preemption_timer, bool,
> >  S_IRUGO);
> >  #endif
> >
> > +static bool __read_mostly have_spec_ctrl;
> > +
> >  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
> >  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
> >  #define KVM_VM_CR0_ALWAYS_ON   \
> > @@ -609,6 +611,8 @@ struct vcpu_vmx {
> > u64   msr_host_kernel_gs_base;
> > u64   msr_guest_kernel_gs_base;
> >  #endif
> > +   u64   spec_ctrl;
> > +
> > u32 vm_entry_controls_shadow;
> > u32 vm_exit_controls_shadow;
> > u32 secondary_exec_control;
> > @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct
> > msr_data *msr_info)
> > case MSR_IA32_TSC:
> > msr_info->data = guest_read_tsc(vcpu);
> > break;
> > +   case MSR_IA32_SPEC_CTRL:
> > +   msr_info->data = to_vmx(vcpu)->spec_ctrl;
> > +   break;
> > case MSR_IA32_SYSENTER_CS:
> > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
> > break;
> > @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct
> > msr_data *msr_info)
> > case MSR_IA32_TSC:
> > kvm_write_tsc(vcpu, msr_info);
> > break;
> > +   case MSR_IA32_SPEC_CTRL:
> > +   to_vmx(vcpu)->spec_ctrl = msr_info->data;
> > +   break;
> > case MSR_IA32_CR_PAT:
> > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> > @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
> > goto out;
> > }
> >
> > +   /*
> > +* FIXME: this is only needed until SPEC_CTRL is supported
> > +* by upstream Linux in cpufeatures, then it can be replaced
> > +* with static_cpu_has.
> > +*/
> > +   have_spec_ctrl = cpu_has_spec_ctrl();
> > +   if (have_spec_ctrl)
> > +   pr_info("kvm: SPEC_CTRL available\n");
> > +   else
> > +   pr_info("kvm: SPEC_CTRL not available\n");
> > +
> > if (boot_cpu_has(X86_FEATURE_NX))
> > kvm_enable_efer_bits(EFER_NX);
> >
> > @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
> > vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
> > vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> > vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> > +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> > +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
> >
> > memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
> > vmx_msr_bitmap_legacy, PAGE_SIZE);
> > @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu
> > *vcpu)
> >
> > pt_guest_enter(vmx);
> >
> > +   if (have_spec_ctrl &&a

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Ashok Raj]

Hi Paolo

Do you assume that host isn't using IBRS and only guest uses it?



On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:
> Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> it yet).
>
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/kvm/vmx.c | 32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 669f5f74857d..d00bcad7336e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -120,6 +120,8 @@
>  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
>  #endif
>
> +static bool __read_mostly have_spec_ctrl;
> +
>  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
>  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
>  #define KVM_VM_CR0_ALWAYS_ON   \
> @@ -609,6 +611,8 @@ struct vcpu_vmx {
> u64   msr_host_kernel_gs_base;
> u64   msr_guest_kernel_gs_base;
>  #endif
> +   u64   spec_ctrl;
> +
> u32 vm_entry_controls_shadow;
> u32 vm_exit_controls_shadow;
> u32 secondary_exec_control;
> @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> msr_info->data = guest_read_tsc(vcpu);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   msr_info->data = to_vmx(vcpu)->spec_ctrl;
> +   break;
> case MSR_IA32_SYSENTER_CS:
> msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
> break;
> @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> kvm_write_tsc(vcpu, msr_info);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   to_vmx(vcpu)->spec_ctrl = msr_info->data;
> +   break;
> case MSR_IA32_CR_PAT:
> if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
> goto out;
> }
>
> +   /*
> +* FIXME: this is only needed until SPEC_CTRL is supported
> +* by upstream Linux in cpufeatures, then it can be replaced
> +* with static_cpu_has.
> +*/
> +   have_spec_ctrl = cpu_has_spec_ctrl();
> +   if (have_spec_ctrl)
> +   pr_info("kvm: SPEC_CTRL available\n");
> +   else
> +   pr_info("kvm: SPEC_CTRL not available\n");
> +
> if (boot_cpu_has(X86_FEATURE_NX))
> kvm_enable_efer_bits(EFER_NX);
>
> @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>
> memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
> vmx_msr_bitmap_legacy, PAGE_SIZE);
> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>
> pt_guest_enter(vmx);
>
> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Do we even need to optimize this? what if host Linux enabled IBRS, but
guest has it turned off?
Thought it might be simpler to blindly update it with what
vmx->spec_ctrl value is?

> atomic_switch_perf_msrs(vmx);
>
> vmx_arm_hv_timer(vcpu);
> @@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  #endif
>   );
>
> +   if (have_spec_ctrl) {
> +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +   if (vmx->spec_ctrl)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> +   }
> +

Same thing here.. if the host OS has enabled IBRS wouldn't you want to
keep the same value?

> /* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
> if (vmx->host_debugctlmsr)
> update_debugctlmsr(vmx->host_debugctlmsr);
> --
> 1.8.3.1
>
>

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Ashok Raj]

Hi Paolo

Do you assume that host isn't using IBRS and only guest uses it?



On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:
> Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> it yet).
>
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/kvm/vmx.c | 32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 669f5f74857d..d00bcad7336e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -120,6 +120,8 @@
>  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
>  #endif
>
> +static bool __read_mostly have_spec_ctrl;
> +
>  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
>  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
>  #define KVM_VM_CR0_ALWAYS_ON   \
> @@ -609,6 +611,8 @@ struct vcpu_vmx {
> u64   msr_host_kernel_gs_base;
> u64   msr_guest_kernel_gs_base;
>  #endif
> +   u64   spec_ctrl;
> +
> u32 vm_entry_controls_shadow;
> u32 vm_exit_controls_shadow;
> u32 secondary_exec_control;
> @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> msr_info->data = guest_read_tsc(vcpu);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   msr_info->data = to_vmx(vcpu)->spec_ctrl;
> +   break;
> case MSR_IA32_SYSENTER_CS:
> msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
> break;
> @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> kvm_write_tsc(vcpu, msr_info);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   to_vmx(vcpu)->spec_ctrl = msr_info->data;
> +   break;
> case MSR_IA32_CR_PAT:
> if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
> goto out;
> }
>
> +   /*
> +* FIXME: this is only needed until SPEC_CTRL is supported
> +* by upstream Linux in cpufeatures, then it can be replaced
> +* with static_cpu_has.
> +*/
> +   have_spec_ctrl = cpu_has_spec_ctrl();
> +   if (have_spec_ctrl)
> +   pr_info("kvm: SPEC_CTRL available\n");
> +   else
> +   pr_info("kvm: SPEC_CTRL not available\n");
> +
> if (boot_cpu_has(X86_FEATURE_NX))
> kvm_enable_efer_bits(EFER_NX);
>
> @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>
> memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
> vmx_msr_bitmap_legacy, PAGE_SIZE);
> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>
> pt_guest_enter(vmx);
>
> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Do we even need to optimize this? what if host Linux enabled IBRS, but
guest has it turned off?
Thought it might be simpler to blindly update it with what
vmx->spec_ctrl value is?

> atomic_switch_perf_msrs(vmx);
>
> vmx_arm_hv_timer(vcpu);
> @@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  #endif
>   );
>
> +   if (have_spec_ctrl) {
> +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +   if (vmx->spec_ctrl)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> +   }
> +

Same thing here.. if the host OS has enabled IBRS wouldn't you want to
keep the same value?

> /* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
> if (vmx->host_debugctlmsr)
> update_debugctlmsr(vmx->host_debugctlmsr);
> --
> 1.8.3.1
>
>

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Liran Alon]

On 08/01/18 21:18, Jim Mattson wrote:

Guest usage of MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD should be
predicated on guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL).

On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:

Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
it yet).

Signed-off-by: Paolo Bonzini 
---
  arch/x86/kvm/vmx.c | 32 
  1 file changed, 32 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 669f5f74857d..d00bcad7336e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -120,6 +120,8 @@
  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
  #endif

+static bool __read_mostly have_spec_ctrl;
+
  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
  #define KVM_VM_CR0_ALWAYS_ON   \
@@ -609,6 +611,8 @@ struct vcpu_vmx {
 u64   msr_host_kernel_gs_base;
 u64   msr_guest_kernel_gs_base;
  #endif
+   u64   spec_ctrl;
+
 u32 vm_entry_controls_shadow;
 u32 vm_exit_controls_shadow;
 u32 secondary_exec_control;
@@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 case MSR_IA32_TSC:
 msr_info->data = guest_read_tsc(vcpu);
 break;
+   case MSR_IA32_SPEC_CTRL:
+   msr_info->data = to_vmx(vcpu)->spec_ctrl;
+   break;


I have:

if (!have_spec_ctrl ||
 (!msr_info->host_initiated &&
  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
break;


 case MSR_IA32_SYSENTER_CS:
 msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
 break;
@@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 case MSR_IA32_TSC:
 kvm_write_tsc(vcpu, msr_info);
 break;
+   case MSR_IA32_SPEC_CTRL:
+   to_vmx(vcpu)->spec_ctrl = msr_info->data;
+   break;


I have:

if (!have_spec_ctrl ||
 (!msr_info->host_initiated &&
  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
break;


 case MSR_IA32_CR_PAT:
 if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
 if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
 goto out;
 }

+   /*
+* FIXME: this is only needed until SPEC_CTRL is supported
+* by upstream Linux in cpufeatures, then it can be replaced
+* with static_cpu_has.
+*/
+   have_spec_ctrl = cpu_has_spec_ctrl();
+   if (have_spec_ctrl)
+   pr_info("kvm: SPEC_CTRL available\n");
+   else
+   pr_info("kvm: SPEC_CTRL not available\n");
+
 if (boot_cpu_has(X86_FEATURE_NX))
 kvm_enable_efer_bits(EFER_NX);

@@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
 vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
 vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
 vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);


I have a lot of changes to MSR permission bitmap handling, but these
intercepts should only be disabled when guest_cpuid_has(vcpu,
X86_FEATURE_SPEC_CTRL).


 memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
 vmx_msr_bitmap_legacy, PAGE_SIZE);
@@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)

 pt_guest_enter(vmx);

+   if (have_spec_ctrl && vmx->spec_ctrl != 0)
+   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+


Here, I have:

/*
* If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
* store it on VM-exit.
*/
if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
else
clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);

/*
* If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
* IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
* MSRs, so that the guest value will be loaded on VM-entry
* and the host value will be loaded on VM-exit.
*/
if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
   vmx->msr_ia32_spec_ctrl,
   spec_ctrl_enabled());
else
clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);



I totally agree with this.

This exactly solves the issue I 

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Liran Alon]

On 08/01/18 21:18, Jim Mattson wrote:

Guest usage of MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD should be
predicated on guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL).

On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:

Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
it yet).

Signed-off-by: Paolo Bonzini 
---
  arch/x86/kvm/vmx.c | 32 
  1 file changed, 32 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 669f5f74857d..d00bcad7336e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -120,6 +120,8 @@
  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
  #endif

+static bool __read_mostly have_spec_ctrl;
+
  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
  #define KVM_VM_CR0_ALWAYS_ON   \
@@ -609,6 +611,8 @@ struct vcpu_vmx {
 u64   msr_host_kernel_gs_base;
 u64   msr_guest_kernel_gs_base;
  #endif
+   u64   spec_ctrl;
+
 u32 vm_entry_controls_shadow;
 u32 vm_exit_controls_shadow;
 u32 secondary_exec_control;
@@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 case MSR_IA32_TSC:
 msr_info->data = guest_read_tsc(vcpu);
 break;
+   case MSR_IA32_SPEC_CTRL:
+   msr_info->data = to_vmx(vcpu)->spec_ctrl;
+   break;


I have:

if (!have_spec_ctrl ||
 (!msr_info->host_initiated &&
  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
break;


 case MSR_IA32_SYSENTER_CS:
 msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
 break;
@@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 case MSR_IA32_TSC:
 kvm_write_tsc(vcpu, msr_info);
 break;
+   case MSR_IA32_SPEC_CTRL:
+   to_vmx(vcpu)->spec_ctrl = msr_info->data;
+   break;


I have:

if (!have_spec_ctrl ||
 (!msr_info->host_initiated &&
  !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
break;


 case MSR_IA32_CR_PAT:
 if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
 if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
 goto out;
 }

+   /*
+* FIXME: this is only needed until SPEC_CTRL is supported
+* by upstream Linux in cpufeatures, then it can be replaced
+* with static_cpu_has.
+*/
+   have_spec_ctrl = cpu_has_spec_ctrl();
+   if (have_spec_ctrl)
+   pr_info("kvm: SPEC_CTRL available\n");
+   else
+   pr_info("kvm: SPEC_CTRL not available\n");
+
 if (boot_cpu_has(X86_FEATURE_NX))
 kvm_enable_efer_bits(EFER_NX);

@@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
 vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
 vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
 vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);


I have a lot of changes to MSR permission bitmap handling, but these
intercepts should only be disabled when guest_cpuid_has(vcpu,
X86_FEATURE_SPEC_CTRL).


 memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
 vmx_msr_bitmap_legacy, PAGE_SIZE);
@@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)

 pt_guest_enter(vmx);

+   if (have_spec_ctrl && vmx->spec_ctrl != 0)
+   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+


Here, I have:

/*
* If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
* store it on VM-exit.
*/
if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
else
clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);

/*
* If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
* IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
* MSRs, so that the guest value will be loaded on VM-entry
* and the host value will be loaded on VM-exit.
*/
if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
   vmx->msr_ia32_spec_ctrl,
   spec_ctrl_enabled());
else
clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);



I totally agree with this.

This exactly solves the issue I mentioned before of restoring the guest 

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[David Woodhouse]

On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
> 
> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

I think this one probably *is* safe even without an 'else lfence',
which means that the CPU can speculate around it, but it wants a
comment explaining that someone has properly analysed it and saying
precisely why.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[David Woodhouse]

On Mon, 2018-01-08 at 19:08 +0100, Paolo Bonzini wrote:
> 
> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

I think this one probably *is* safe even without an 'else lfence',
which means that the CPU can speculate around it, but it wants a
comment explaining that someone has properly analysed it and saying
precisely why.

smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Liran Alon]

On 08/01/18 20:08, Paolo Bonzini wrote:

Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
it yet).

Signed-off-by: Paolo Bonzini 
---
  arch/x86/kvm/vmx.c | 32 
  1 file changed, 32 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 669f5f74857d..d00bcad7336e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -120,6 +120,8 @@
  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
  #endif

+static bool __read_mostly have_spec_ctrl;
+
  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
  #define KVM_VM_CR0_ALWAYS_ON  \
@@ -609,6 +611,8 @@ struct vcpu_vmx {
u64   msr_host_kernel_gs_base;
u64   msr_guest_kernel_gs_base;
  #endif
+   u64   spec_ctrl;
+
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
u32 secondary_exec_control;
@@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
msr_info->data = guest_read_tsc(vcpu);
break;
+   case MSR_IA32_SPEC_CTRL:
+   msr_info->data = to_vmx(vcpu)->spec_ctrl;
+   break;
case MSR_IA32_SYSENTER_CS:
msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
break;
@@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
kvm_write_tsc(vcpu, msr_info);
break;
+   case MSR_IA32_SPEC_CTRL:
+   to_vmx(vcpu)->spec_ctrl = msr_info->data;
+   break;
case MSR_IA32_CR_PAT:
if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
goto out;
}

+   /*
+* FIXME: this is only needed until SPEC_CTRL is supported
+* by upstream Linux in cpufeatures, then it can be replaced
+* with static_cpu_has.
+*/
+   have_spec_ctrl = cpu_has_spec_ctrl();
+   if (have_spec_ctrl)
+   pr_info("kvm: SPEC_CTRL available\n");
+   else
+   pr_info("kvm: SPEC_CTRL not available\n");
+
if (boot_cpu_has(X86_FEATURE_NX))
kvm_enable_efer_bits(EFER_NX);

@@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);

memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
vmx_msr_bitmap_legacy, PAGE_SIZE);
@@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)

pt_guest_enter(vmx);

+   if (have_spec_ctrl && vmx->spec_ctrl != 0)
+   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+


Intel specifies that the restore of MSR_IA32_SPEC_CTRL to guest value 
using WRMSR should be done after the last indirect branch before VMEntry.
However, atomic_switch_perf_msrs() calls perf_guest_get_msrs() which 
calls x86_pmu.guest_get_msrs() which is an indirect branch.


Therefore, it seems that this block should be done after the call to 
vmx_arm_hv_timer().



atomic_switch_perf_msrs(vmx);

vmx_arm_hv_timer(vcpu);
@@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
  #endif
  );

+   if (have_spec_ctrl) {
+   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+   if (vmx->spec_ctrl)
+   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+   }
+
/* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
if (vmx->host_debugctlmsr)
update_debugctlmsr(vmx->host_debugctlmsr);

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Liran Alon]

On 08/01/18 20:08, Paolo Bonzini wrote:

Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
it yet).

Signed-off-by: Paolo Bonzini 
---
  arch/x86/kvm/vmx.c | 32 
  1 file changed, 32 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 669f5f74857d..d00bcad7336e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -120,6 +120,8 @@
  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
  #endif

+static bool __read_mostly have_spec_ctrl;
+
  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
  #define KVM_VM_CR0_ALWAYS_ON  \
@@ -609,6 +611,8 @@ struct vcpu_vmx {
u64   msr_host_kernel_gs_base;
u64   msr_guest_kernel_gs_base;
  #endif
+   u64   spec_ctrl;
+
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
u32 secondary_exec_control;
@@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
msr_info->data = guest_read_tsc(vcpu);
break;
+   case MSR_IA32_SPEC_CTRL:
+   msr_info->data = to_vmx(vcpu)->spec_ctrl;
+   break;
case MSR_IA32_SYSENTER_CS:
msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
break;
@@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
kvm_write_tsc(vcpu, msr_info);
break;
+   case MSR_IA32_SPEC_CTRL:
+   to_vmx(vcpu)->spec_ctrl = msr_info->data;
+   break;
case MSR_IA32_CR_PAT:
if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
goto out;
}

+   /*
+* FIXME: this is only needed until SPEC_CTRL is supported
+* by upstream Linux in cpufeatures, then it can be replaced
+* with static_cpu_has.
+*/
+   have_spec_ctrl = cpu_has_spec_ctrl();
+   if (have_spec_ctrl)
+   pr_info("kvm: SPEC_CTRL available\n");
+   else
+   pr_info("kvm: SPEC_CTRL not available\n");
+
if (boot_cpu_has(X86_FEATURE_NX))
kvm_enable_efer_bits(EFER_NX);

@@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);

memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
vmx_msr_bitmap_legacy, PAGE_SIZE);
@@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)

pt_guest_enter(vmx);

+   if (have_spec_ctrl && vmx->spec_ctrl != 0)
+   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+


Intel specifies that the restore of MSR_IA32_SPEC_CTRL to guest value 
using WRMSR should be done after the last indirect branch before VMEntry.
However, atomic_switch_perf_msrs() calls perf_guest_get_msrs() which 
calls x86_pmu.guest_get_msrs() which is an indirect branch.


Therefore, it seems that this block should be done after the call to 
vmx_arm_hv_timer().



atomic_switch_perf_msrs(vmx);

vmx_arm_hv_timer(vcpu);
@@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
  #endif
  );

+   if (have_spec_ctrl) {
+   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+   if (vmx->spec_ctrl)
+   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+   }
+
/* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
if (vmx->host_debugctlmsr)
update_debugctlmsr(vmx->host_debugctlmsr);

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Jim Mattson]

Guest usage of MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD should be
predicated on guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL).

On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:
> Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> it yet).
>
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/kvm/vmx.c | 32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 669f5f74857d..d00bcad7336e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -120,6 +120,8 @@
>  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
>  #endif
>
> +static bool __read_mostly have_spec_ctrl;
> +
>  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
>  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
>  #define KVM_VM_CR0_ALWAYS_ON   \
> @@ -609,6 +611,8 @@ struct vcpu_vmx {
> u64   msr_host_kernel_gs_base;
> u64   msr_guest_kernel_gs_base;
>  #endif
> +   u64   spec_ctrl;
> +
> u32 vm_entry_controls_shadow;
> u32 vm_exit_controls_shadow;
> u32 secondary_exec_control;
> @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> msr_info->data = guest_read_tsc(vcpu);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   msr_info->data = to_vmx(vcpu)->spec_ctrl;
> +   break;

I have:

if (!have_spec_ctrl ||
(!msr_info->host_initiated &&
 !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
break;

> case MSR_IA32_SYSENTER_CS:
> msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
> break;
> @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> kvm_write_tsc(vcpu, msr_info);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   to_vmx(vcpu)->spec_ctrl = msr_info->data;
> +   break;

I have:

if (!have_spec_ctrl ||
(!msr_info->host_initiated &&
 !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
break;

> case MSR_IA32_CR_PAT:
> if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
> goto out;
> }
>
> +   /*
> +* FIXME: this is only needed until SPEC_CTRL is supported
> +* by upstream Linux in cpufeatures, then it can be replaced
> +* with static_cpu_has.
> +*/
> +   have_spec_ctrl = cpu_has_spec_ctrl();
> +   if (have_spec_ctrl)
> +   pr_info("kvm: SPEC_CTRL available\n");
> +   else
> +   pr_info("kvm: SPEC_CTRL not available\n");
> +
> if (boot_cpu_has(X86_FEATURE_NX))
> kvm_enable_efer_bits(EFER_NX);
>
> @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);

I have a lot of changes to MSR permission bitmap handling, but these
intercepts should only be disabled when guest_cpuid_has(vcpu,
X86_FEATURE_SPEC_CTRL).

> memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
> vmx_msr_bitmap_legacy, PAGE_SIZE);
> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>
> pt_guest_enter(vmx);
>
> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Here, I have:

/*
* If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
* store it on VM-exit.
*/
if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
else
clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);

/*
* If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
* IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
* MSRs, so that the guest value will be loaded on VM-entry
* and the host value will be loaded on VM-exit.
*/
if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
  vmx->msr_ia32_spec_ctrl,
  spec_ctrl_enabled());
else
clear_atomic_switch_msr(vmx, 

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Jim Mattson]

Guest usage of MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD should be
predicated on guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL).

On Mon, Jan 8, 2018 at 10:08 AM, Paolo Bonzini  wrote:
> Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> it yet).
>
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/kvm/vmx.c | 32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 669f5f74857d..d00bcad7336e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -120,6 +120,8 @@
>  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
>  #endif
>
> +static bool __read_mostly have_spec_ctrl;
> +
>  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
>  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
>  #define KVM_VM_CR0_ALWAYS_ON   \
> @@ -609,6 +611,8 @@ struct vcpu_vmx {
> u64   msr_host_kernel_gs_base;
> u64   msr_guest_kernel_gs_base;
>  #endif
> +   u64   spec_ctrl;
> +
> u32 vm_entry_controls_shadow;
> u32 vm_exit_controls_shadow;
> u32 secondary_exec_control;
> @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> msr_info->data = guest_read_tsc(vcpu);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   msr_info->data = to_vmx(vcpu)->spec_ctrl;
> +   break;

I have:

if (!have_spec_ctrl ||
(!msr_info->host_initiated &&
 !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
msr_info->data = to_vmx(vcpu)->msr_ia32_spec_ctrl;
break;

> case MSR_IA32_SYSENTER_CS:
> msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
> break;
> @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_TSC:
> kvm_write_tsc(vcpu, msr_info);
> break;
> +   case MSR_IA32_SPEC_CTRL:
> +   to_vmx(vcpu)->spec_ctrl = msr_info->data;
> +   break;

I have:

if (!have_spec_ctrl ||
(!msr_info->host_initiated &&
 !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)))
return 1;
to_vmx(vcpu)->msr_ia32_spec_ctrl = data;
break;

> case MSR_IA32_CR_PAT:
> if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
> goto out;
> }
>
> +   /*
> +* FIXME: this is only needed until SPEC_CTRL is supported
> +* by upstream Linux in cpufeatures, then it can be replaced
> +* with static_cpu_has.
> +*/
> +   have_spec_ctrl = cpu_has_spec_ctrl();
> +   if (have_spec_ctrl)
> +   pr_info("kvm: SPEC_CTRL available\n");
> +   else
> +   pr_info("kvm: SPEC_CTRL not available\n");
> +
> if (boot_cpu_has(X86_FEATURE_NX))
> kvm_enable_efer_bits(EFER_NX);
>
> @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> +   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);

I have a lot of changes to MSR permission bitmap handling, but these
intercepts should only be disabled when guest_cpuid_has(vcpu,
X86_FEATURE_SPEC_CTRL).

> memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
> vmx_msr_bitmap_legacy, PAGE_SIZE);
> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>
> pt_guest_enter(vmx);
>
> +   if (have_spec_ctrl && vmx->spec_ctrl != 0)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Here, I have:

/*
* If the guest is allowed to write to MSR_IA32_SPEC_CTRL,
* store it on VM-exit.
*/
if (guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
add_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);
else
clear_autostore_msr(vmx, MSR_IA32_SPEC_CTRL);

/*
* If the guest's IA32_SPEC_CTRL MSR doesn't match the host's
* IA32_SPEC_CTRL MSR, then add the MSR to the atomic switch
* MSRs, so that the guest value will be loaded on VM-entry
* and the host value will be loaded on VM-exit.
*/
if (vmx->msr_ia32_spec_ctrl != spec_ctrl_enabled())
add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL,
  vmx->msr_ia32_spec_ctrl,
  spec_ctrl_enabled());
else
clear_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL);

> 

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Konrad Rzeszutek Wilk]

On Mon, Jan 08, 2018 at 07:08:41PM +0100, Paolo Bonzini wrote:
> Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> it yet).
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/kvm/vmx.c | 32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 669f5f74857d..d00bcad7336e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -120,6 +120,8 @@
>  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
>  #endif
>  
> +static bool __read_mostly have_spec_ctrl;
> +
>  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
>  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
>  #define KVM_VM_CR0_ALWAYS_ON \
> @@ -609,6 +611,8 @@ struct vcpu_vmx {
>   u64   msr_host_kernel_gs_base;
>   u64   msr_guest_kernel_gs_base;
>  #endif
> + u64   spec_ctrl;
> +
>   u32 vm_entry_controls_shadow;
>   u32 vm_exit_controls_shadow;
>   u32 secondary_exec_control;
> @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   case MSR_IA32_TSC:
>   msr_info->data = guest_read_tsc(vcpu);
>   break;
> + case MSR_IA32_SPEC_CTRL:
> + msr_info->data = to_vmx(vcpu)->spec_ctrl;
> + break;
>   case MSR_IA32_SYSENTER_CS:
>   msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
>   break;
> @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   case MSR_IA32_TSC:
>   kvm_write_tsc(vcpu, msr_info);
>   break;
> + case MSR_IA32_SPEC_CTRL:
> + to_vmx(vcpu)->spec_ctrl = msr_info->data;
> + break;
>   case MSR_IA32_CR_PAT:
>   if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
>   if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
>   goto out;
>   }
>  
> + /*
> +  * FIXME: this is only needed until SPEC_CTRL is supported
> +  * by upstream Linux in cpufeatures, then it can be replaced
> +  * with static_cpu_has.
> +  */
> + have_spec_ctrl = cpu_has_spec_ctrl();
> + if (have_spec_ctrl)
> + pr_info("kvm: SPEC_CTRL available\n");
> + else
> + pr_info("kvm: SPEC_CTRL not available\n");
> +
>   if (boot_cpu_has(X86_FEATURE_NX))
>   kvm_enable_efer_bits(EFER_NX);
>  
> @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
>   vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
>   vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
>   vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> + vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> + vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>  
>   memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
>   vmx_msr_bitmap_legacy, PAGE_SIZE);
> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  
>   pt_guest_enter(vmx);
>  
> + if (have_spec_ctrl && vmx->spec_ctrl)
> + wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Shouldn't this be as close to the assembler code as possible?

>   atomic_switch_perf_msrs(vmx);
>  
>   vmx_arm_hv_timer(vcpu);

Re: [PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Konrad Rzeszutek Wilk]

On Mon, Jan 08, 2018 at 07:08:41PM +0100, Paolo Bonzini wrote:
> Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
> for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
> IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
> it yet).
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/kvm/vmx.c | 32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 669f5f74857d..d00bcad7336e 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -120,6 +120,8 @@
>  module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
>  #endif
>  
> +static bool __read_mostly have_spec_ctrl;
> +
>  #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
>  #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
>  #define KVM_VM_CR0_ALWAYS_ON \
> @@ -609,6 +611,8 @@ struct vcpu_vmx {
>   u64   msr_host_kernel_gs_base;
>   u64   msr_guest_kernel_gs_base;
>  #endif
> + u64   spec_ctrl;
> +
>   u32 vm_entry_controls_shadow;
>   u32 vm_exit_controls_shadow;
>   u32 secondary_exec_control;
> @@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   case MSR_IA32_TSC:
>   msr_info->data = guest_read_tsc(vcpu);
>   break;
> + case MSR_IA32_SPEC_CTRL:
> + msr_info->data = to_vmx(vcpu)->spec_ctrl;
> + break;
>   case MSR_IA32_SYSENTER_CS:
>   msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
>   break;
> @@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
>   case MSR_IA32_TSC:
>   kvm_write_tsc(vcpu, msr_info);
>   break;
> + case MSR_IA32_SPEC_CTRL:
> + to_vmx(vcpu)->spec_ctrl = msr_info->data;
> + break;
>   case MSR_IA32_CR_PAT:
>   if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
>   if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
> @@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
>   goto out;
>   }
>  
> + /*
> +  * FIXME: this is only needed until SPEC_CTRL is supported
> +  * by upstream Linux in cpufeatures, then it can be replaced
> +  * with static_cpu_has.
> +  */
> + have_spec_ctrl = cpu_has_spec_ctrl();
> + if (have_spec_ctrl)
> + pr_info("kvm: SPEC_CTRL available\n");
> + else
> + pr_info("kvm: SPEC_CTRL not available\n");
> +
>   if (boot_cpu_has(X86_FEATURE_NX))
>   kvm_enable_efer_bits(EFER_NX);
>  
> @@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
>   vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
>   vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
>   vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
> + vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
> + vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
>  
>   memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
>   vmx_msr_bitmap_legacy, PAGE_SIZE);
> @@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  
>   pt_guest_enter(vmx);
>  
> + if (have_spec_ctrl && vmx->spec_ctrl)
> + wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Shouldn't this be as close to the assembler code as possible?

>   atomic_switch_perf_msrs(vmx);
>  
>   vmx_arm_hv_timer(vcpu);

[PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
it yet).

Signed-off-by: Paolo Bonzini 
---
 arch/x86/kvm/vmx.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 669f5f74857d..d00bcad7336e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -120,6 +120,8 @@
 module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
 #endif
 
+static bool __read_mostly have_spec_ctrl;
+
 #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
 #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
 #define KVM_VM_CR0_ALWAYS_ON   \
@@ -609,6 +611,8 @@ struct vcpu_vmx {
u64   msr_host_kernel_gs_base;
u64   msr_guest_kernel_gs_base;
 #endif
+   u64   spec_ctrl;
+
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
u32 secondary_exec_control;
@@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
msr_info->data = guest_read_tsc(vcpu);
break;
+   case MSR_IA32_SPEC_CTRL:
+   msr_info->data = to_vmx(vcpu)->spec_ctrl;
+   break;
case MSR_IA32_SYSENTER_CS:
msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
break;
@@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
kvm_write_tsc(vcpu, msr_info);
break;
+   case MSR_IA32_SPEC_CTRL:
+   to_vmx(vcpu)->spec_ctrl = msr_info->data;
+   break;
case MSR_IA32_CR_PAT:
if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
goto out;
}
 
+   /*
+* FIXME: this is only needed until SPEC_CTRL is supported
+* by upstream Linux in cpufeatures, then it can be replaced
+* with static_cpu_has.
+*/
+   have_spec_ctrl = cpu_has_spec_ctrl();
+   if (have_spec_ctrl)
+   pr_info("kvm: SPEC_CTRL available\n");
+   else
+   pr_info("kvm: SPEC_CTRL not available\n");
+
if (boot_cpu_has(X86_FEATURE_NX))
kvm_enable_efer_bits(EFER_NX);
 
@@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
 
memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
vmx_msr_bitmap_legacy, PAGE_SIZE);
@@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
pt_guest_enter(vmx);
 
+   if (have_spec_ctrl && vmx->spec_ctrl != 0)
+   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+
atomic_switch_perf_msrs(vmx);
 
vmx_arm_hv_timer(vcpu);
@@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 #endif
  );
 
+   if (have_spec_ctrl) {
+   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+   if (vmx->spec_ctrl)
+   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+   }
+
/* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
if (vmx->host_debugctlmsr)
update_debugctlmsr(vmx->host_debugctlmsr);
-- 
1.8.3.1

[PATCH 3/7] kvm: vmx: pass MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD down to the guest

[Paolo Bonzini]

Direct access to MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD is important
for performance.  Allow load/store of MSR_IA32_SPEC_CTRL, restore guest
IBRS on VM entry and set it to 0 on VM exit (because Linux does not use
it yet).

Signed-off-by: Paolo Bonzini 
---
 arch/x86/kvm/vmx.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 669f5f74857d..d00bcad7336e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -120,6 +120,8 @@
 module_param_named(preemption_timer, enable_preemption_timer, bool, S_IRUGO);
 #endif
 
+static bool __read_mostly have_spec_ctrl;
+
 #define KVM_GUEST_CR0_MASK (X86_CR0_NW | X86_CR0_CD)
 #define KVM_VM_CR0_ALWAYS_ON_UNRESTRICTED_GUEST (X86_CR0_WP | X86_CR0_NE)
 #define KVM_VM_CR0_ALWAYS_ON   \
@@ -609,6 +611,8 @@ struct vcpu_vmx {
u64   msr_host_kernel_gs_base;
u64   msr_guest_kernel_gs_base;
 #endif
+   u64   spec_ctrl;
+
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
u32 secondary_exec_control;
@@ -3361,6 +3365,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
msr_info->data = guest_read_tsc(vcpu);
break;
+   case MSR_IA32_SPEC_CTRL:
+   msr_info->data = to_vmx(vcpu)->spec_ctrl;
+   break;
case MSR_IA32_SYSENTER_CS:
msr_info->data = vmcs_read32(GUEST_SYSENTER_CS);
break;
@@ -3500,6 +3507,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_IA32_TSC:
kvm_write_tsc(vcpu, msr_info);
break;
+   case MSR_IA32_SPEC_CTRL:
+   to_vmx(vcpu)->spec_ctrl = msr_info->data;
+   break;
case MSR_IA32_CR_PAT:
if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))
@@ -7062,6 +7072,17 @@ static __init int hardware_setup(void)
goto out;
}
 
+   /*
+* FIXME: this is only needed until SPEC_CTRL is supported
+* by upstream Linux in cpufeatures, then it can be replaced
+* with static_cpu_has.
+*/
+   have_spec_ctrl = cpu_has_spec_ctrl();
+   if (have_spec_ctrl)
+   pr_info("kvm: SPEC_CTRL available\n");
+   else
+   pr_info("kvm: SPEC_CTRL not available\n");
+
if (boot_cpu_has(X86_FEATURE_NX))
kvm_enable_efer_bits(EFER_NX);
 
@@ -7131,6 +7152,8 @@ static __init int hardware_setup(void)
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_SPEC_CTRL, false);
+   vmx_disable_intercept_for_msr(MSR_IA32_PRED_CMD, false);
 
memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
vmx_msr_bitmap_legacy, PAGE_SIZE);
@@ -9597,6 +9620,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
pt_guest_enter(vmx);
 
+   if (have_spec_ctrl && vmx->spec_ctrl != 0)
+   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+
atomic_switch_perf_msrs(vmx);
 
vmx_arm_hv_timer(vcpu);
@@ -9707,6 +9733,12 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 #endif
  );
 
+   if (have_spec_ctrl) {
+   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+   if (vmx->spec_ctrl)
+   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
+   }
+
/* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
if (vmx->host_debugctlmsr)
update_debugctlmsr(vmx->host_debugctlmsr);
-- 
1.8.3.1