diff mbox series

[v6,24/25] KVM: nVMX: Introduce new VMX_BASIC bit for event error_code delivery to L1

Message ID 20230914063325.85503-25-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable CET Virtualization | expand

Commit Message

Yang, Weijiang Sept. 14, 2023, 6:33 a.m. UTC 
Per SDM description(Vol.3D, Appendix A.1):
"If bit 56 is read as 1, software can use VM entry to deliver a hardware
exception with or without an error code, regardless of vector"

Modify has_error_code check before inject events to nested guest. Only
enforce the check when guest is in real mode, the exception is not hard
exception and the platform doesn't enumerate bit56 in VMX_BASIC, in all
other case ignore the check to make the logic consistent with SDM.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++--------
 arch/x86/kvm/vmx/nested.h |  5 +++++
 2 files changed, 19 insertions(+), 8 deletions(-)

Comments

Maxim Levitsky Oct. 31, 2023, 5:57 p.m. UTC | #1 
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
> Per SDM description(Vol.3D, Appendix A.1):
> "If bit 56 is read as 1, software can use VM entry to deliver a hardware
> exception with or without an error code, regardless of vector"
> 
> Modify has_error_code check before inject events to nested guest. Only
> enforce the check when guest is in real mode, the exception is not hard
> exception and the platform doesn't enumerate bit56 in VMX_BASIC, in all
> other case ignore the check to make the logic consistent with SDM.
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++--------
>  arch/x86/kvm/vmx/nested.h |  5 +++++
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index c5ec0ef51ff7..78a3be394d00 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1205,9 +1205,9 @@ static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
>  {
>  	const u64 feature_and_reserved =
>  		/* feature (except bit 48; see below) */
> -		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
> +		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) | BIT_ULL(56) |
>  		/* reserved */
> -		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
> +		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 57);
>  	u64 vmx_basic = vmcs_config.nested.basic;
>  
>  	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
> @@ -2846,12 +2846,16 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>  		    CC(intr_type == INTR_TYPE_OTHER_EVENT && vector != 0))
>  			return -EINVAL;
>  
> -		/* VM-entry interruption-info field: deliver error code */
> -		should_have_error_code =
> -			intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
> -			x86_exception_has_error_code(vector);
> -		if (CC(has_error_code != should_have_error_code))
> -			return -EINVAL;
> +		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
> +		    !nested_cpu_has_no_hw_errcode_cc(vcpu)) {
> +			/* VM-entry interruption-info field: deliver error code */
> +			should_have_error_code =
> +				intr_type == INTR_TYPE_HARD_EXCEPTION &&
> +				prot_mode &&
> +				x86_exception_has_error_code(vector);
> +			if (CC(has_error_code != should_have_error_code))
> +				return -EINVAL;
> +		}
>  
>  		/* VM-entry exception error code */
>  		if (CC(has_error_code &&
> @@ -6968,6 +6972,8 @@ static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
>  
>  	if (cpu_has_vmx_basic_inout())
>  		msrs->basic |= VMX_BASIC_INOUT;
> +	if (cpu_has_vmx_basic_no_hw_errcode())
> +		msrs->basic |= VMX_BASIC_NO_HW_ERROR_CODE_CC;
>  }
>  
>  static void nested_vmx_setup_cr_fixed(struct nested_vmx_msrs *msrs)
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index b4b9d51438c6..26842da6857d 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -284,6 +284,11 @@ static inline bool nested_cr4_valid(struct kvm_vcpu *vcpu, unsigned long val)
>  	       __kvm_is_valid_cr4(vcpu, val);
>  }
>  
> +static inline bool nested_cpu_has_no_hw_errcode_cc(struct kvm_vcpu *vcpu)
> +{
> +	return to_vmx(vcpu)->nested.msrs.basic & VMX_BASIC_NO_HW_ERROR_CODE_CC;
> +}
> +
>  /* No difference in the restrictions on guest and host CR4 in VMX operation. */
>  #define nested_guest_cr4_valid	nested_cr4_valid
>  #define nested_host_cr4_valid	nested_cr4_valid

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky
Chao Gao Nov. 1, 2023, 4:21 a.m. UTC | #2 
On Thu, Sep 14, 2023 at 02:33:24AM -0400, Yang Weijiang wrote:
>Per SDM description(Vol.3D, Appendix A.1):
>"If bit 56 is read as 1, software can use VM entry to deliver a hardware
>exception with or without an error code, regardless of vector"
>
>Modify has_error_code check before inject events to nested guest. Only
>enforce the check when guest is in real mode, the exception is not hard
>exception and the platform doesn't enumerate bit56 in VMX_BASIC, in all
>other case ignore the check to make the logic consistent with SDM.
>
>Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>---
> arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++--------
> arch/x86/kvm/vmx/nested.h |  5 +++++
> 2 files changed, 19 insertions(+), 8 deletions(-)
>
>diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>index c5ec0ef51ff7..78a3be394d00 100644
>--- a/arch/x86/kvm/vmx/nested.c
>+++ b/arch/x86/kvm/vmx/nested.c
>@@ -1205,9 +1205,9 @@ static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
> {
> 	const u64 feature_and_reserved =
> 		/* feature (except bit 48; see below) */
>-		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
>+		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) | BIT_ULL(56) |
> 		/* reserved */
>-		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
>+		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 57);
> 	u64 vmx_basic = vmcs_config.nested.basic;
> 
> 	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
>@@ -2846,12 +2846,16 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
> 		    CC(intr_type == INTR_TYPE_OTHER_EVENT && vector != 0))
> 			return -EINVAL;
> 
>-		/* VM-entry interruption-info field: deliver error code */
>-		should_have_error_code =
>-			intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
>-			x86_exception_has_error_code(vector);
>-		if (CC(has_error_code != should_have_error_code))
>-			return -EINVAL;
>+		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
>+		    !nested_cpu_has_no_hw_errcode_cc(vcpu)) {
>+			/* VM-entry interruption-info field: deliver error code */
>+			should_have_error_code =
>+				intr_type == INTR_TYPE_HARD_EXCEPTION &&
>+				prot_mode &&
>+				x86_exception_has_error_code(vector);
>+			if (CC(has_error_code != should_have_error_code))
>+				return -EINVAL;
>+		}

prot_mode and intr_type are used twice, making the code a little hard to read.

how about:
		/*
		 * Cannot deliver error code in real mode or if the
		 * interruption type is not hardware exception. For other
		 * cases, do the consistency check only if the vCPU doesn't
		 * enumerate VMX_BASIC_NO_HW_ERROR_CODE_CC.
		 */
		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION) {
			if (CC(has_error_code))
				return -EINVAL;
		} else if (!nested_cpu_has_no_hw_errcode_cc(vcpu)) {
			if (CC(has_error_code != x86_exception_has_error_code(vector)))
				return -EINVAL;
		}

and drop should_have_error_code.
Yang, Weijiang Nov. 15, 2023, 8:31 a.m. UTC | #3 
On 11/1/2023 12:21 PM, Chao Gao wrote:
> On Thu, Sep 14, 2023 at 02:33:24AM -0400, Yang Weijiang wrote:
>> Per SDM description(Vol.3D, Appendix A.1):
>> "If bit 56 is read as 1, software can use VM entry to deliver a hardware
>> exception with or without an error code, regardless of vector"
>>
>> Modify has_error_code check before inject events to nested guest. Only
>> enforce the check when guest is in real mode, the exception is not hard
>> exception and the platform doesn't enumerate bit56 in VMX_BASIC, in all
>> other case ignore the check to make the logic consistent with SDM.
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>> arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++--------
>> arch/x86/kvm/vmx/nested.h |  5 +++++
>> 2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index c5ec0ef51ff7..78a3be394d00 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -1205,9 +1205,9 @@ static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
>> {
>> 	const u64 feature_and_reserved =
>> 		/* feature (except bit 48; see below) */
>> -		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
>> +		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) | BIT_ULL(56) |
>> 		/* reserved */
>> -		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
>> +		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 57);
>> 	u64 vmx_basic = vmcs_config.nested.basic;
>>
>> 	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
>> @@ -2846,12 +2846,16 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>> 		    CC(intr_type == INTR_TYPE_OTHER_EVENT && vector != 0))
>> 			return -EINVAL;
>>
>> -		/* VM-entry interruption-info field: deliver error code */
>> -		should_have_error_code =
>> -			intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
>> -			x86_exception_has_error_code(vector);
>> -		if (CC(has_error_code != should_have_error_code))
>> -			return -EINVAL;
>> +		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
>> +		    !nested_cpu_has_no_hw_errcode_cc(vcpu)) {
>> +			/* VM-entry interruption-info field: deliver error code */
>> +			should_have_error_code =
>> +				intr_type == INTR_TYPE_HARD_EXCEPTION &&
>> +				prot_mode &&
>> +				x86_exception_has_error_code(vector);
>> +			if (CC(has_error_code != should_have_error_code))
>> +				return -EINVAL;
>> +		}
> prot_mode and intr_type are used twice, making the code a little hard to read.
>
> how about:
> 		/*
> 		 * Cannot deliver error code in real mode or if the
> 		 * interruption type is not hardware exception. For other
> 		 * cases, do the consistency check only if the vCPU doesn't
> 		 * enumerate VMX_BASIC_NO_HW_ERROR_CODE_CC.
> 		 */
> 		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION) {
> 			if (CC(has_error_code))
> 				return -EINVAL;
> 		} else if (!nested_cpu_has_no_hw_errcode_cc(vcpu)) {
> 			if (CC(has_error_code != x86_exception_has_error_code(vector)))
> 				return -EINVAL;
> 		}
>
> and drop should_have_error_code.

The change looks clearer, I'll take it, thanks!
Sean Christopherson Nov. 15, 2023, 1:27 p.m. UTC | #4 
On Wed, Nov 15, 2023, Weijiang Yang wrote:
> On 11/1/2023 12:21 PM, Chao Gao wrote:
> > On Thu, Sep 14, 2023 at 02:33:24AM -0400, Yang Weijiang wrote:
> > > @@ -2846,12 +2846,16 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
> > > 		    CC(intr_type == INTR_TYPE_OTHER_EVENT && vector != 0))
> > > 			return -EINVAL;
> > > 
> > > -		/* VM-entry interruption-info field: deliver error code */
> > > -		should_have_error_code =
> > > -			intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
> > > -			x86_exception_has_error_code(vector);
> > > -		if (CC(has_error_code != should_have_error_code))
> > > -			return -EINVAL;
> > > +		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
> > > +		    !nested_cpu_has_no_hw_errcode_cc(vcpu)) {
> > > +			/* VM-entry interruption-info field: deliver error code */
> > > +			should_have_error_code =
> > > +				intr_type == INTR_TYPE_HARD_EXCEPTION &&
> > > +				prot_mode &&
> > > +				x86_exception_has_error_code(vector);
> > > +			if (CC(has_error_code != should_have_error_code))
> > > +				return -EINVAL;
> > > +		}
> > prot_mode and intr_type are used twice, making the code a little hard to read.
> > 
> > how about:
> > 		/*
> > 		 * Cannot deliver error code in real mode or if the
> > 		 * interruption type is not hardware exception. For other
> > 		 * cases, do the consistency check only if the vCPU doesn't
> > 		 * enumerate VMX_BASIC_NO_HW_ERROR_CODE_CC.
> > 		 */
> > 		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION) {
> > 			if (CC(has_error_code))
> > 				return -EINVAL;
> > 		} else if (!nested_cpu_has_no_hw_errcode_cc(vcpu)) {
> > 			if (CC(has_error_code != x86_exception_has_error_code(vector)))
> > 				return -EINVAL;
> > 		}

Or maybe go one step further and put the nested_cpu_has...() check inside the CC()
macro so that it too will be captured on error.  It's a little uglier though, and
I doubt providing that extra information will matter in practice, so definitely
feel free to stick with Chao's version.

		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION) {
			if (CC(has_error_code))
				return -EINVAL;
		} else if (CC(!nested_cpu_has_no_hw_errcode_cc(vcpu) &&
			      has_error_code != x86_exception_has_error_code(vector))) {
			return -EINVAL;
		}
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index c5ec0ef51ff7..78a3be394d00 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1205,9 +1205,9 @@  static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx, u64 data)
 {
 	const u64 feature_and_reserved =
 		/* feature (except bit 48; see below) */
-		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
+		BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) | BIT_ULL(56) |
 		/* reserved */
-		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
+		BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 57);
 	u64 vmx_basic = vmcs_config.nested.basic;
 
 	if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
@@ -2846,12 +2846,16 @@  static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
 		    CC(intr_type == INTR_TYPE_OTHER_EVENT && vector != 0))
 			return -EINVAL;
 
-		/* VM-entry interruption-info field: deliver error code */
-		should_have_error_code =
-			intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
-			x86_exception_has_error_code(vector);
-		if (CC(has_error_code != should_have_error_code))
-			return -EINVAL;
+		if (!prot_mode || intr_type != INTR_TYPE_HARD_EXCEPTION ||
+		    !nested_cpu_has_no_hw_errcode_cc(vcpu)) {
+			/* VM-entry interruption-info field: deliver error code */
+			should_have_error_code =
+				intr_type == INTR_TYPE_HARD_EXCEPTION &&
+				prot_mode &&
+				x86_exception_has_error_code(vector);
+			if (CC(has_error_code != should_have_error_code))
+				return -EINVAL;
+		}
 
 		/* VM-entry exception error code */
 		if (CC(has_error_code &&
@@ -6968,6 +6972,8 @@  static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
 
 	if (cpu_has_vmx_basic_inout())
 		msrs->basic |= VMX_BASIC_INOUT;
+	if (cpu_has_vmx_basic_no_hw_errcode())
+		msrs->basic |= VMX_BASIC_NO_HW_ERROR_CODE_CC;
 }
 
 static void nested_vmx_setup_cr_fixed(struct nested_vmx_msrs *msrs)
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index b4b9d51438c6..26842da6857d 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -284,6 +284,11 @@  static inline bool nested_cr4_valid(struct kvm_vcpu *vcpu, unsigned long val)
 	       __kvm_is_valid_cr4(vcpu, val);
 }
 
+static inline bool nested_cpu_has_no_hw_errcode_cc(struct kvm_vcpu *vcpu)
+{
+	return to_vmx(vcpu)->nested.msrs.basic & VMX_BASIC_NO_HW_ERROR_CODE_CC;
+}
+
 /* No difference in the restrictions on guest and host CR4 in VMX operation. */
 #define nested_guest_cr4_valid	nested_cr4_valid
 #define nested_host_cr4_valid	nested_cr4_valid