diff mbox series

[2/6] nVMX x86: Re-organize the code in nested_check_vmentry_prereqs(), related to Guest State Area

Message ID 20181113061209.6843-3-krish.sadhukhan@oracle.com (mailing list archive)
State New, archived
Headers show
Series [1/6] nVMX x86: Re-name check_vmentry_pre[post]reqs to nested_check_vmentry_pre[post] | expand

Commit Message

Krish Sadhukhan Nov. 13, 2018, 6:12 a.m. UTC 
Separate out the checks in nested_check_vmentry_prereqs(), that are related
to Guest State Area, to a separate function. The re-organized code is easier
for readability, enhancement and maintenance.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
---
 arch/x86/kvm/vmx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Sean Christopherson Nov. 13, 2018, 4:48 p.m. UTC | #1 
On Tue, Nov 13, 2018 at 01:12:05AM -0500, Krish Sadhukhan wrote:
> Separate out the checks in nested_check_vmentry_prereqs(), that are related
> to Guest State Area, to a separate function. The re-organized code is easier
> for readability, enhancement and maintenance.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 0f6c38f..d9f3bc7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -12313,12 +12313,23 @@ static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
>  	return 0;
>  }
>  
> +/*
> + * Checks related to Guest State Area
> + */
> +static int nested_check_guest_state_area(struct vmcs12 *vmcs12)
> +{
> +	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> +            vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  
> -	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> -	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> +	if (nested_check_guest_state_area(vmcs12))
>  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;

Hmm, guest state is a bit of a mess since many of the checks result in
VMExit.  The basic rule of thumb is that "Guest Non-Register State"
checks cause a failed VMEntry while "Guest Register State" checks cause
VMExit, though sadly even that delineation doesn't always hold true.

Maybe the best approach is to go one level deeper in the SDM, e.g.
nested_check_guest_non_register_state(), and add a comment calling out
that some "Guest Non-Register State" checks are deliberately omitted
because they cause VMExit.

>  
>  	if (nested_vmx_check_io_bitmap_controls(vcpu, vmcs12))
> -- 
> 2.9.5
>
Sean Christopherson Nov. 13, 2018, 5:06 p.m. UTC | #2 
On Tue, Nov 13, 2018 at 08:48:53AM -0800, Sean Christopherson wrote:
> On Tue, Nov 13, 2018 at 01:12:05AM -0500, Krish Sadhukhan wrote:
> > Separate out the checks in nested_check_vmentry_prereqs(), that are related
> > to Guest State Area, to a separate function. The re-organized code is easier
> > for readability, enhancement and maintenance.
> > 
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> > Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
> > ---
> >  arch/x86/kvm/vmx.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 0f6c38f..d9f3bc7 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -12313,12 +12313,23 @@ static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Checks related to Guest State Area
> > + */
> > +static int nested_check_guest_state_area(struct vmcs12 *vmcs12)
> > +{
> > +	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> > +            vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> >  {
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> >  
> > -	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
> > -	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
> > +	if (nested_check_guest_state_area(vmcs12))
> >  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> 
> Hmm, guest state is a bit of a mess since many of the checks result in
> VMExit.  The basic rule of thumb is that "Guest Non-Register State"
> checks cause a failed VMEntry while "Guest Register State" checks cause
> VMExit, though sadly even that delineation doesn't always hold true.
> 
> Maybe the best approach is to go one level deeper in the SDM, e.g.
> nested_check_guest_non_register_state(), and add a comment calling out
> that some "Guest Non-Register State" checks are deliberately omitted
> because they cause VMExit.

Actually, this is a KVM bug, or maybe KVM intentionally diverges from
the SDM.  Guest activity state checks cause VMExits, not VMFail.

> >  
> >  	if (nested_vmx_check_io_bitmap_controls(vcpu, vmcs12))
> > -- 
> > 2.9.5
> >
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 0f6c38f..d9f3bc7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -12313,12 +12313,23 @@  static int nested_vmx_check_nmi_controls(struct vmcs12 *vmcs12)
 	return 0;
 }
 
+/*
+ * Checks related to Guest State Area
+ */
+static int nested_check_guest_state_area(struct vmcs12 *vmcs12)
+{
+	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
+            vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int nested_check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
-	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
-	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
+	if (nested_check_guest_state_area(vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
 
 	if (nested_vmx_check_io_bitmap_controls(vcpu, vmcs12))