KVM: SVM: Add a dedicated INVD intercept routine

Commit Message

Tom Lendacky Sept. 23, 2020, 8:27 p.m. UTC

From: Tom Lendacky <[e-mail]>

The INVD instruction intercept performs emulation. Emulation can't be done
on an SEV guest because the guest memory is encrypted.

Provide a dedicated intercept routine for the INVD intercept. Within this
intercept routine just skip the instruction for an SEV guest, since it is
emulated as a NOP anyway.

Fixes: 1654efcbc431 ("KVM: SVM: Add KVM_SEV_INIT command")
Signed-off-by: Tom Lendacky <[e-mail]>
---
 arch/x86/kvm/svm/svm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Sept. 23, 2020, 8:32 p.m. UTC | #1

On Wed, Sep 23, 2020 at 03:27:39PM -0500, Tom Lendacky wrote:
> From: Tom Lendacky <[e-mail]>
> 
> The INVD instruction intercept performs emulation. Emulation can't be done
> on an SEV guest because the guest memory is encrypted.
> 
> Provide a dedicated intercept routine for the INVD intercept. Within this
> intercept routine just skip the instruction for an SEV guest, since it is
> emulated as a NOP anyway.
> 
> Fixes: 1654efcbc431 ("KVM: SVM: Add KVM_SEV_INIT command")
> Signed-off-by: Tom Lendacky <[e-mail]>
> ---
>  arch/x86/kvm/svm/svm.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c91acabf18d0..332ec4425d89 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2183,6 +2183,17 @@ static int iret_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> +static int invd_interception(struct vcpu_svm *svm)
> +{
> +	/*
> +	 * Can't do emulation on an SEV guest and INVD is emulated
> +	 * as a NOP, so just skip the instruction.
> +	 */
> +	return (sev_guest(svm->vcpu.kvm))
> +		? kvm_skip_emulated_instruction(&svm->vcpu)
> +		: kvm_emulate_instruction(&svm->vcpu, 0);

Is there any reason not to do kvm_skip_emulated_instruction() for both SEV
and legacy?  VMX has the same odd kvm_emulate_instruction() call, but AFAICT
that's completely unecessary, i.e. VMX can also convert to a straight skip.

> +}
> +
>  static int invlpg_interception(struct vcpu_svm *svm)
>  {
>  	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
> @@ -2774,7 +2785,7 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>  	[SVM_EXIT_RDPMC]			= rdpmc_interception,
>  	[SVM_EXIT_CPUID]			= cpuid_interception,
>  	[SVM_EXIT_IRET]                         = iret_interception,
> -	[SVM_EXIT_INVD]                         = emulate_on_interception,
> +	[SVM_EXIT_INVD]                         = invd_interception,
>  	[SVM_EXIT_PAUSE]			= pause_interception,
>  	[SVM_EXIT_HLT]				= halt_interception,
>  	[SVM_EXIT_INVLPG]			= invlpg_interception,
> -- 
> 2.28.0
>

Tom Lendacky Sept. 23, 2020, 8:40 p.m. UTC | #2

On 9/23/20 3:32 PM, Sean Christopherson wrote:
> On Wed, Sep 23, 2020 at 03:27:39PM -0500, Tom Lendacky wrote:
>> From: Tom Lendacky <[e-mail]>
>>
>> The INVD instruction intercept performs emulation. Emulation can't be done
>> on an SEV guest because the guest memory is encrypted.
>>
>> Provide a dedicated intercept routine for the INVD intercept. Within this
>> intercept routine just skip the instruction for an SEV guest, since it is
>> emulated as a NOP anyway.
>>
>> Fixes: 1654efcbc431 ("KVM: SVM: Add KVM_SEV_INIT command")
>> Signed-off-by: Tom Lendacky <[e-mail]>
>> ---
>>  arch/x86/kvm/svm/svm.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index c91acabf18d0..332ec4425d89 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -2183,6 +2183,17 @@ static int iret_interception(struct vcpu_svm *svm)
>>  	return 1;
>>  }
>>  
>> +static int invd_interception(struct vcpu_svm *svm)
>> +{
>> +	/*
>> +	 * Can't do emulation on an SEV guest and INVD is emulated
>> +	 * as a NOP, so just skip the instruction.
>> +	 */
>> +	return (sev_guest(svm->vcpu.kvm))
>> +		? kvm_skip_emulated_instruction(&svm->vcpu)
>> +		: kvm_emulate_instruction(&svm->vcpu, 0);
> 
> Is there any reason not to do kvm_skip_emulated_instruction() for both SEV
> and legacy?  VMX has the same odd kvm_emulate_instruction() call, but AFAICT
> that's completely unecessary, i.e. VMX can also convert to a straight skip.

You could, I just figured I'd leave the legacy behavior just in case. Not
that I can think of a reason that behavior would ever change.

Thanks,
Tom

> 
>> +}
>> +
>>  static int invlpg_interception(struct vcpu_svm *svm)
>>  {
>>  	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
>> @@ -2774,7 +2785,7 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>>  	[SVM_EXIT_RDPMC]			= rdpmc_interception,
>>  	[SVM_EXIT_CPUID]			= cpuid_interception,
>>  	[SVM_EXIT_IRET]                         = iret_interception,
>> -	[SVM_EXIT_INVD]                         = emulate_on_interception,
>> +	[SVM_EXIT_INVD]                         = invd_interception,
>>  	[SVM_EXIT_PAUSE]			= pause_interception,
>>  	[SVM_EXIT_HLT]				= halt_interception,
>>  	[SVM_EXIT_INVLPG]			= invlpg_interception,
>> -- 
>> 2.28.0
>>

Paolo Bonzini Sept. 24, 2020, 6:51 a.m. UTC | #3

On 23/09/20 22:40, Tom Lendacky wrote:
>>> +static int invd_interception(struct vcpu_svm *svm)
>>> +{
>>> +	/*
>>> +	 * Can't do emulation on an SEV guest and INVD is emulated
>>> +	 * as a NOP, so just skip the instruction.
>>> +	 */
>>> +	return (sev_guest(svm->vcpu.kvm))
>>> +		? kvm_skip_emulated_instruction(&svm->vcpu)
>>> +		: kvm_emulate_instruction(&svm->vcpu, 0);
>>
>> Is there any reason not to do kvm_skip_emulated_instruction() for both SEV
>> and legacy?  VMX has the same odd kvm_emulate_instruction() call, but AFAICT
>> that's completely unecessary, i.e. VMX can also convert to a straight skip.
> 
> You could, I just figured I'd leave the legacy behavior just in case. Not
> that I can think of a reason that behavior would ever change.

Yeah, let's do skip for both SVM and VMX.

Paolo

Tom Lendacky Sept. 24, 2020, 1:33 p.m. UTC | #4

On 9/24/20 1:51 AM, Paolo Bonzini wrote:
> On 23/09/20 22:40, Tom Lendacky wrote:
>>>> +static int invd_interception(struct vcpu_svm *svm)
>>>> +{
>>>> +	/*
>>>> +	 * Can't do emulation on an SEV guest and INVD is emulated
>>>> +	 * as a NOP, so just skip the instruction.
>>>> +	 */
>>>> +	return (sev_guest(svm->vcpu.kvm))
>>>> +		? kvm_skip_emulated_instruction(&svm->vcpu)
>>>> +		: kvm_emulate_instruction(&svm->vcpu, 0);
>>>
>>> Is there any reason not to do kvm_skip_emulated_instruction() for both SEV
>>> and legacy?  VMX has the same odd kvm_emulate_instruction() call, but AFAICT
>>> that's completely unecessary, i.e. VMX can also convert to a straight skip.
>>
>> You could, I just figured I'd leave the legacy behavior just in case. Not
>> that I can think of a reason that behavior would ever change.
> 
> Yeah, let's do skip for both SVM and VMX.

Ok, I'll submit a two patch series to change SVM and VMX. I'll do two 
patches because of the fixes tag to get the SVM fix back to stable. But, 
if you would prefer a single patch, let me know.

Thanks,
Tom

> 
> Paolo
>

Vitaly Kuznetsov Sept. 24, 2020, 1:58 p.m. UTC | #5

Tom Lendacky <[e-mail]> writes:

> From: Tom Lendacky <[e-mail]>
>
> The INVD instruction intercept performs emulation. Emulation can't be done
> on an SEV guest because the guest memory is encrypted.
>
> Provide a dedicated intercept routine for the INVD intercept. Within this
> intercept routine just skip the instruction for an SEV guest, since it is
> emulated as a NOP anyway.
>
> Fixes: 1654efcbc431 ("KVM: SVM: Add KVM_SEV_INIT command")
> Signed-off-by: Tom Lendacky <[e-mail]>
> ---
>  arch/x86/kvm/svm/svm.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c91acabf18d0..332ec4425d89 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2183,6 +2183,17 @@ static int iret_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> +static int invd_interception(struct vcpu_svm *svm)
> +{
> +	/*
> +	 * Can't do emulation on an SEV guest and INVD is emulated
> +	 * as a NOP, so just skip the instruction.
> +	 */
> +	return (sev_guest(svm->vcpu.kvm))
> +		? kvm_skip_emulated_instruction(&svm->vcpu)
> +		: kvm_emulate_instruction(&svm->vcpu, 0);
> +}
> +
>  static int invlpg_interception(struct vcpu_svm *svm)
>  {
>  	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
> @@ -2774,7 +2785,7 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>  	[SVM_EXIT_RDPMC]			= rdpmc_interception,
>  	[SVM_EXIT_CPUID]			= cpuid_interception,
>  	[SVM_EXIT_IRET]                         = iret_interception,
> -	[SVM_EXIT_INVD]                         = emulate_on_interception,
> +	[SVM_EXIT_INVD]                         = invd_interception,
>  	[SVM_EXIT_PAUSE]			= pause_interception,
>  	[SVM_EXIT_HLT]				= halt_interception,
>  	[SVM_EXIT_INVLPG]			= invlpg_interception,

Out of pure curiosity,

does it sill make sense to intercept INVD when we just skip it? Would it
rather make sense to disable INVD intercept for SEV guests completely?

Paolo Bonzini Sept. 24, 2020, 2:06 p.m. UTC | #6

On 24/09/20 15:58, Vitaly Kuznetsov wrote:
> does it sill make sense to intercept INVD when we just skip it? Would it
> rather make sense to disable INVD intercept for SEV guests completely?

If we don't intercept the processor would really invalidate the cache,
that is certainly not what we want.

Paolo

Patch

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c91acabf18d0..332ec4425d89 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2183,6 +2183,17 @@  static int iret_interception(struct vcpu_svm *svm)
 	return 1;
 }
 
+static int invd_interception(struct vcpu_svm *svm)
+{
+	/*
+	 * Can't do emulation on an SEV guest and INVD is emulated
+	 * as a NOP, so just skip the instruction.
+	 */
+	return (sev_guest(svm->vcpu.kvm))
+		? kvm_skip_emulated_instruction(&svm->vcpu)
+		: kvm_emulate_instruction(&svm->vcpu, 0);
+}
+
 static int invlpg_interception(struct vcpu_svm *svm)
 {
 	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
@@ -2774,7 +2785,7 @@  static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_RDPMC]			= rdpmc_interception,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
-	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_INVD]                         = invd_interception,
 	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,