diff mbox series

[v3,5/7] KVM: x86: nSVM: implement nested vGIF

Message ID 20220301143650.143749-6-mlevitsk@redhat.com (mailing list archive)
State New, archived
Headers show
Series nSVM/SVM features | expand

Commit Message

Maxim Levitsky March 1, 2022, 2:36 p.m. UTC 
In case L1 enables vGIF for L2, the L2 cannot affect L1's GIF, regardless
of STGI/CLGI intercepts, and since VM entry enables GIF, this means
that L1's GIF is always 1 while L2 is running.

Thus in this case leave L1's vGIF in vmcb01, while letting L2
control the vGIF thus implementing nested vGIF.

Also allow KVM to toggle L1's GIF during nested entry/exit
by always using vmcb01.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 17 +++++++++++++----
 arch/x86/kvm/svm/svm.c    |  5 +++++
 arch/x86/kvm/svm/svm.h    | 25 +++++++++++++++++++++----
 3 files changed, 39 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini March 9, 2022, 1:40 p.m. UTC | #1 
The patch is good but I think it's possibly to rewrite some parts in an 
easier way.

On 3/1/22 15:36, Maxim Levitsky wrote:
> 
> +	if (svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
> +		int_ctl_vmcb12_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
> +	else
> +		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);

To remember for later: svm->vmcb's V_GIF_ENABLE_MASK is always the same 
as vgif:

- if it comes from vmcb12, it must be 1 (and then vgif is also 1)

- if it comes from vmcb01, it must be equal to vgif (because 
V_GIF_ENABLE_MASK is set in init_vmcb and never touched again).

>   
> +static bool nested_vgif_enabled(struct vcpu_svm *svm)
> +{
> +	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
> +		return false;
> +	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
> +}
> +
>   static inline bool vgif_enabled(struct vcpu_svm *svm)
>   {
> -	return !!(svm->vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
> +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> +
> +	return !!(vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
>   }
>   

Slight simplification:

- before the patch, vgif_enabled() is just "vgif", because 
V_GIF_ENABLE_MASK is set in init_vmcb and copied to vmcb02

- after the patch, vgif_enabled() is also just "vgif".  Outside guest 
mode the same reasoning applies.  If L2 has enabled vGIF,  vmcb01's 
V_GIF_ENABLE is equal to vgif per the previous bullet.  If L2 has not 
enabled vGIF, vmcb02's V_GIF_ENABLE uses svm->vmcb's int_ctl field which 
is always the same as vgif (see remark above).

You can make this simplification a separate patch.

>  static inline void enable_gif(struct vcpu_svm *svm)
>  {
> +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> +
>  	if (vgif_enabled(svm))
> -		svm->vmcb->control.int_ctl |= V_GIF_MASK;
> +		vmcb->control.int_ctl |= V_GIF_MASK;
>  	else
>  		svm->vcpu.arch.hflags |= HF_GIF_MASK;
>  }
>  
>  static inline void disable_gif(struct vcpu_svm *svm)
>  {
> +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> +
>  	if (vgif_enabled(svm))
> -		svm->vmcb->control.int_ctl &= ~V_GIF_MASK;
> +		vmcb->control.int_ctl &= ~V_GIF_MASK;
>  	else
>  		svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
> +
>  }

Looks good.  For a little optimization however you could write

static inline struct vmcb *get_vgif_vmcb(struct vcpu_svm *svm)
{
	if (!vgif)
		return NULL;
	if (!is_guest_mode(&svm->vcpu)
		return svm->vmcb01.ptr;
	if ((svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
		return svm->vmcb01.ptr;
	else
		return svm->nested.vmcb02.ptr;
}

and then

	struct vmcb *vmcb = get_vgif_vmcb(svm);
	if (vmcb)
		/* use vmcb->control.int_ctl */
	else
		/* use hflags */

Paolo

>  
> +static bool nested_vgif_enabled(struct vcpu_svm *svm)
> +{
> +	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
> +		return false;
> +	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
> +}
> +
Maxim Levitsky March 14, 2022, 3:21 p.m. UTC | #2 
On Wed, 2022-03-09 at 14:40 +0100, Paolo Bonzini wrote:
> The patch is good but I think it's possibly to rewrite some parts in an 
> easier way.
> 
> On 3/1/22 15:36, Maxim Levitsky wrote:
> > +	if (svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
> > +		int_ctl_vmcb12_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
> > +	else
> > +		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
> 
> To remember for later: svm->vmcb's V_GIF_ENABLE_MASK is always the same 
> as vgif:
> 
> - if it comes from vmcb12, it must be 1 (and then vgif is also 1)
> 
> - if it comes from vmcb01, it must be equal to vgif (because 
> V_GIF_ENABLE_MASK is set in init_vmcb and never touched again).
> 
> >   
> > +static bool nested_vgif_enabled(struct vcpu_svm *svm)
> > +{
> > +	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
> > +		return false;
> > +	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
> > +}
> > +
> >   static inline bool vgif_enabled(struct vcpu_svm *svm)
> >   {
> > -	return !!(svm->vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
> > +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> > +
> > +	return !!(vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
> >   }
> >   
> 
> Slight simplification:
> 
> - before the patch, vgif_enabled() is just "vgif", because 
> V_GIF_ENABLE_MASK is set in init_vmcb and copied to vmcb02
> 
> - after the patch, vgif_enabled() is also just "vgif".  Outside guest 
> mode the same reasoning applies.  If L2 has enabled vGIF,  vmcb01's 
> V_GIF_ENABLE is equal to vgif per the previous bullet.  If L2 has not 
> enabled vGIF, vmcb02's V_GIF_ENABLE uses svm->vmcb's int_ctl field which 
> is always the same as vgif (see remark above).
> 
> You can make this simplification a separate patch.


This is a very good idea - I'll do this in a separate patch.

> 
> >  static inline void enable_gif(struct vcpu_svm *svm)
> >  {
> > +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> > +
> >  	if (vgif_enabled(svm))
> > -		svm->vmcb->control.int_ctl |= V_GIF_MASK;
> > +		vmcb->control.int_ctl |= V_GIF_MASK;
> >  	else
> >  		svm->vcpu.arch.hflags |= HF_GIF_MASK;
> >  }
> >  
> >  static inline void disable_gif(struct vcpu_svm *svm)
> >  {
> > +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> > +
> >  	if (vgif_enabled(svm))
> > -		svm->vmcb->control.int_ctl &= ~V_GIF_MASK;
> > +		vmcb->control.int_ctl &= ~V_GIF_MASK;
> >  	else
> >  		svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
> > +
> >  }
> 
> Looks good.  For a little optimization however you could write
> 
> static inline struct vmcb *get_vgif_vmcb(struct vcpu_svm *svm)
> {
> 	if (!vgif)
> 		return NULL;
> 	if (!is_guest_mode(&svm->vcpu)
> 		return svm->vmcb01.ptr;
> 	if ((svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
> 		return svm->vmcb01.ptr;
> 	else
> 		return svm->nested.vmcb02.ptr;
> }
> 
> and then
> 
> 	struct vmcb *vmcb = get_vgif_vmcb(svm);
> 	if (vmcb)
> 		/* use vmcb->control.int_ctl */
> 	else
> 		/* use hflags */

Good idea as well.

Thanks for the review!
Best regards,
	Maxim Levitsky

> 
> Paolo
> 
> >  
> > +static bool nested_vgif_enabled(struct vcpu_svm *svm)
> > +{
> > +	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
> > +		return false;
> > +	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
> > +}
> > +
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 4cb0bc49986d5..4f8b5a330096e 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -436,6 +436,10 @@  void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)
 		 */
 		mask &= ~V_IRQ_MASK;
 	}
+
+	if (nested_vgif_enabled(svm))
+		mask |= V_GIF_MASK;
+
 	svm->nested.ctl.int_ctl        &= ~mask;
 	svm->nested.ctl.int_ctl        |= svm->vmcb->control.int_ctl & mask;
 }
@@ -602,10 +606,8 @@  static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 
 static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
 {
-	const u32 int_ctl_vmcb01_bits =
-		V_INTR_MASKING_MASK | V_GIF_MASK | V_GIF_ENABLE_MASK;
-
-	const u32 int_ctl_vmcb12_bits = V_TPR_MASK | V_IRQ_INJECTION_BITS_MASK;
+	u32 int_ctl_vmcb01_bits = V_INTR_MASKING_MASK;
+	u32 int_ctl_vmcb12_bits = V_TPR_MASK | V_IRQ_INJECTION_BITS_MASK;
 
 	struct kvm_vcpu *vcpu = &svm->vcpu;
 
@@ -620,6 +622,13 @@  static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
 	 */
 	WARN_ON(kvm_apicv_activated(svm->vcpu.kvm));
 
+
+
+	if (svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
+		int_ctl_vmcb12_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
+	else
+		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
+
 	/* Copied from vmcb01.  msrpm_base can be overwritten later.  */
 	svm->vmcb->control.nested_ctl = svm->vmcb01.ptr->control.nested_ctl;
 	svm->vmcb->control.iopm_base_pa = svm->vmcb01.ptr->control.iopm_base_pa;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 52198e63c5fc4..776585dd77769 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4019,6 +4019,8 @@  static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		svm->pause_threshold_enabled = false;
 	}
 
+	svm->vgif_enabled = vgif && guest_cpuid_has(vcpu, X86_FEATURE_VGIF);
+
 	svm_recalc_instruction_intercepts(vcpu, svm);
 
 	/* For sev guests, the memory encryption bit is not reserved in CR3.  */
@@ -4780,6 +4782,9 @@  static __init void svm_set_cpu_caps(void)
 		if (pause_filter_thresh)
 			kvm_cpu_cap_set(X86_FEATURE_PFTHRESHOLD);
 
+		if (vgif)
+			kvm_cpu_cap_set(X86_FEATURE_VGIF);
+
 		/* Nested VM can receive #VMEXIT instead of triggering #GP */
 		kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
 	}
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 6fa81eb3ffb78..7e2f9bddf47dd 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -236,6 +236,7 @@  struct vcpu_svm {
 	bool v_vmload_vmsave_enabled      : 1;
 	bool pause_filter_enabled         : 1;
 	bool pause_threshold_enabled      : 1;
+	bool vgif_enabled                 : 1;
 
 	u32 ldr_reg;
 	u32 dfr_reg;
@@ -454,31 +455,47 @@  static inline bool svm_is_intercept(struct vcpu_svm *svm, int bit)
 	return vmcb_is_intercept(&svm->vmcb->control, bit);
 }
 
+static bool nested_vgif_enabled(struct vcpu_svm *svm)
+{
+	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
+		return false;
+	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
+}
+
 static inline bool vgif_enabled(struct vcpu_svm *svm)
 {
-	return !!(svm->vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
+	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
+
+	return !!(vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
 }
 
 static inline void enable_gif(struct vcpu_svm *svm)
 {
+	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
+
 	if (vgif_enabled(svm))
-		svm->vmcb->control.int_ctl |= V_GIF_MASK;
+		vmcb->control.int_ctl |= V_GIF_MASK;
 	else
 		svm->vcpu.arch.hflags |= HF_GIF_MASK;
 }
 
 static inline void disable_gif(struct vcpu_svm *svm)
 {
+	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
+
 	if (vgif_enabled(svm))
-		svm->vmcb->control.int_ctl &= ~V_GIF_MASK;
+		vmcb->control.int_ctl &= ~V_GIF_MASK;
 	else
 		svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
+
 }
 
 static inline bool gif_set(struct vcpu_svm *svm)
 {
+	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
+
 	if (vgif_enabled(svm))
-		return !!(svm->vmcb->control.int_ctl & V_GIF_MASK);
+		return !!(vmcb->control.int_ctl & V_GIF_MASK);
 	else
 		return !!(svm->vcpu.arch.hflags & HF_GIF_MASK);
 }