diff mbox series

[2/7,v7] KVM: nSVM: Define an exit code to reflect consistency check failure

Message ID 20210412215611.110095-3-krish.sadhukhan@oracle.com (mailing list archive)
State New
Headers show
Series KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests | expand

Commit Message

Krish Sadhukhan April 12, 2021, 9:56 p.m. UTC
nested_svm_vmrun() returns SVM_EXIT_ERR both when consistency checks for
MSRPM fail and when merging the MSRPM of vmcb12 with that of KVM fails. These
two failures are different in that the first one happens during consistency
checking while the second happens after consistency checking passes and after
guest mode switch is done. In order to differentiate between the two types of
error conditions, define an exit code that can be used to denote consistency
check failures. This new exit code is similar to what nVMX uses to denote
consistency check failures. For nSVM, we will use the highest bit in the high
part of the EXIT_CODE field.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/include/uapi/asm/svm.h | 1 +
 arch/x86/kvm/svm/nested.c       | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini April 17, 2021, 2:17 p.m. UTC | #1
On 12/04/21 23:56, Krish Sadhukhan wrote:
> nested_svm_vmrun() returns SVM_EXIT_ERR both when consistency checks for
> MSRPM fail and when merging the MSRPM of vmcb12 with that of KVM fails. These
> two failures are different in that the first one happens during consistency
> checking while the second happens after consistency checking passes and after
> guest mode switch is done. In order to differentiate between the two types of
> error conditions, define an exit code that can be used to denote consistency
> check failures. This new exit code is similar to what nVMX uses to denote
> consistency check failures. For nSVM, we will use the highest bit in the high
> part of the EXIT_CODE field.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

The exit code is defined by AMD, we cannot change it.

Paolo

> ---
>   arch/x86/include/uapi/asm/svm.h | 1 +
>   arch/x86/kvm/svm/nested.c       | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
> index 554f75fe013c..b0a6550a23f5 100644
> --- a/arch/x86/include/uapi/asm/svm.h
> +++ b/arch/x86/include/uapi/asm/svm.h
> @@ -111,6 +111,7 @@
>   #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
>   
>   #define SVM_EXIT_ERR           -1
> +#define	SVM_CONSISTENCY_ERR    1 << 31
>   
>   #define SVM_EXIT_REASONS \
>   	{ SVM_EXIT_READ_CR0,    "read_cr0" }, \
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 8453c898b68b..ae53ae46ebca 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -606,7 +606,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>   	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
>   	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
>   		vmcb12->control.exit_code    = SVM_EXIT_ERR;
> -		vmcb12->control.exit_code_hi = 0;
> +		vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
>   		vmcb12->control.exit_info_1  = 0;
>   		vmcb12->control.exit_info_2  = 0;
>   		goto out;
>
Krish Sadhukhan April 19, 2021, 5:57 p.m. UTC | #2
On 4/17/21 7:17 AM, Paolo Bonzini wrote:
> On 12/04/21 23:56, Krish Sadhukhan wrote:
>> nested_svm_vmrun() returns SVM_EXIT_ERR both when consistency checks for
>> MSRPM fail and when merging the MSRPM of vmcb12 with that of KVM 
>> fails. These
>> two failures are different in that the first one happens during 
>> consistency
>> checking while the second happens after consistency checking passes 
>> and after
>> guest mode switch is done. In order to differentiate between the two 
>> types of
>> error conditions, define an exit code that can be used to denote 
>> consistency
>> check failures. This new exit code is similar to what nVMX uses to 
>> denote
>> consistency check failures. For nSVM, we will use the highest bit in 
>> the high
>> part of the EXIT_CODE field.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>
> The exit code is defined by AMD, we cannot change it.


The reason why I thought of this is that SVM implementation uses only 
the lower half, as all AMD-defined exit code are handled therein only. 
Is this still going to cause an issue ?

>
> Paolo
>
>> ---
>>   arch/x86/include/uapi/asm/svm.h | 1 +
>>   arch/x86/kvm/svm/nested.c       | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/svm.h 
>> b/arch/x86/include/uapi/asm/svm.h
>> index 554f75fe013c..b0a6550a23f5 100644
>> --- a/arch/x86/include/uapi/asm/svm.h
>> +++ b/arch/x86/include/uapi/asm/svm.h
>> @@ -111,6 +111,7 @@
>>   #define SVM_VMGEXIT_UNSUPPORTED_EVENT        0x8000ffff
>>     #define SVM_EXIT_ERR           -1
>> +#define    SVM_CONSISTENCY_ERR    1 << 31
>>     #define SVM_EXIT_REASONS \
>>       { SVM_EXIT_READ_CR0,    "read_cr0" }, \
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 8453c898b68b..ae53ae46ebca 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -606,7 +606,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>>       if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
>>           !nested_vmcb_check_controls(&svm->nested.ctl)) {
>>           vmcb12->control.exit_code    = SVM_EXIT_ERR;
>> -        vmcb12->control.exit_code_hi = 0;
>> +        vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
>>           vmcb12->control.exit_info_1  = 0;
>>           vmcb12->control.exit_info_2  = 0;
>>           goto out;
>>
>
Paolo Bonzini April 19, 2021, 6:28 p.m. UTC | #3
On 19/04/21 19:57, Krish Sadhukhan wrote:
> The reason why I thought of this is that SVM implementation uses only 
> the lower half, as all AMD-defined exit code are handled therein only. 
> Is this still going to cause an issue ?

I would have to check what happens on bare metal, but VMEXIT_INVALID is 
defined as "-1", not "FFFFFFFFh", so I think it should use the high 32 
bits (in which case KVM is wrong in not storing the high 32 bits).

Paolo
Jim Mattson April 19, 2021, 6:36 p.m. UTC | #4
On Mon, Apr 19, 2021 at 11:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 19/04/21 19:57, Krish Sadhukhan wrote:
> > The reason why I thought of this is that SVM implementation uses only
> > the lower half, as all AMD-defined exit code are handled therein only.
> > Is this still going to cause an issue ?
>
> I would have to check what happens on bare metal, but VMEXIT_INVALID is
> defined as "-1", not "FFFFFFFFh", so I think it should use the high 32
> bits (in which case KVM is wrong in not storing the high 32 bits).

And VMEXIT_BUSY is -2.
diff mbox series

Patch

diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
index 554f75fe013c..b0a6550a23f5 100644
--- a/arch/x86/include/uapi/asm/svm.h
+++ b/arch/x86/include/uapi/asm/svm.h
@@ -111,6 +111,7 @@ 
 #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
 
 #define SVM_EXIT_ERR           -1
+#define	SVM_CONSISTENCY_ERR    1 << 31
 
 #define SVM_EXIT_REASONS \
 	{ SVM_EXIT_READ_CR0,    "read_cr0" }, \
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 8453c898b68b..ae53ae46ebca 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -606,7 +606,7 @@  int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
 	    !nested_vmcb_check_controls(&svm->nested.ctl)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
-		vmcb12->control.exit_code_hi = 0;
+		vmcb12->control.exit_code_hi = SVM_CONSISTENCY_ERR;
 		vmcb12->control.exit_info_1  = 0;
 		vmcb12->control.exit_info_2  = 0;
 		goto out;