diff mbox series

[v3,11/14] KVM: SVM: Introduce hybrid-AVIC mode

Message ID 20220504073128.12031-12-suravee.suthikulpanit@amd.com (mailing list archive)
State New, archived
Headers show
Series Introducing AMD x2AVIC and hybrid-AVIC modes | expand

Commit Message

Suravee Suthikulpanit May 4, 2022, 7:31 a.m. UTC
Currently, AVIC is inhibited when booting a VM w/ x2APIC support.
because AVIC cannot virtualize x2APIC MSR register accesses.
However, the AVIC doorbell can be used to accelerate interrupt
injection into a running vCPU, while all guest accesses to x2APIC MSRs
will be intercepted and emulated by KVM.

With hybrid-AVIC support, the APICV_INHIBIT_REASON_X2APIC is
no longer enforced.

Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 10 +++++++++-
 arch/x86/kvm/svm/svm.c  |  9 ---------
 2 files changed, 9 insertions(+), 10 deletions(-)

Comments

Maxim Levitsky May 4, 2022, 12:32 p.m. UTC | #1
On Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
> Currently, AVIC is inhibited when booting a VM w/ x2APIC support.
> because AVIC cannot virtualize x2APIC MSR register accesses.
> However, the AVIC doorbell can be used to accelerate interrupt
> injection into a running vCPU, while all guest accesses to x2APIC MSRs
> will be intercepted and emulated by KVM.
> 
> With hybrid-AVIC support, the APICV_INHIBIT_REASON_X2APIC is
> no longer enforced.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 10 +++++++++-
>  arch/x86/kvm/svm/svm.c  |  9 ---------
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index d07c58f06bed..3b6a96043633 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -92,12 +92,20 @@ static void avic_activate_vmcb(struct vcpu_svm *svm)
>  	vmcb->control.avic_physical_id &= ~AVIC_PHYSICAL_MAX_INDEX_MASK;
>  
>  	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> -	if (apic_x2apic_mode(svm->vcpu.arch.apic)) {
> +
> +	/* Note:
> +	 * KVM can support hybrid-x2AVIC mode, where KVM emulates x2APIC
> +	 * MSR accesses, while interrupt injection to a running vCPU
> +	 * can be achieve using AVIC doorbell.
> +	 */
> +	if (apic_x2apic_mode(svm->vcpu.arch.apic) &&
> +	    (avic_mode == AVIC_MODE_X2)) {
>  		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
>  		vmcb->control.avic_physical_id |= X2AVIC_MAX_PHYSICAL_ID;
>  		/* Disabling MSR intercept for x2APIC registers */
>  		avic_set_x2apic_msr_interception(svm, false);
>  	} else {
> +		/* For xAVIC and hybrid-x2AVIC modes */
>  		vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
>  		/* Enabling MSR intercept for x2APIC registers */
>  		avic_set_x2apic_msr_interception(svm, true);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 96a1fc1a1d1b..c0a3d4a1f3dc 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4041,7 +4041,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct kvm_cpuid_entry2 *best;
> -	struct kvm *kvm = vcpu->kvm;
>  
>  	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
>  				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> @@ -4073,14 +4072,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  			vcpu->arch.reserved_gpa_bits &= ~(1UL << (best->ebx & 0x3f));
>  	}
>  
> -	if (kvm_vcpu_apicv_active(vcpu)) {
> -		/*
> -		 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
> -		 * is exposed to the guest, disable AVIC.
> -		 */
> -		if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> -			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_X2APIC);
> -	}
>  	init_vmcb_after_set_cpuid(vcpu);
>  }
>  


Well strictly speaking, another thing that has to be done, other that removing the inhibit,
is to 'hide' the AVIC's private memslot if one of vCPUs is in x2apic mode, 
although not doing this doesn't cause any harm as the guest is not supposed to poke at xAPIC
mmio even when uses x2apic, and if it does it will get the normal AVIC acceleration,
so probably it is better to not add any more complexity and leave it like that.

Besides that my only note on this is that you forgot the most satisfying part of this,
removing the APICV_INHIBIT_REASON_X2APIC value ;-)

So besides the removal of the APICV_INHIBIT_REASON_X2APIC:

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


BTW, hardware wise, does 'X2APIC_MODE' keeps the emulation of the AVIC mmio, or
not?

Best regards,
	Maxim Levitsky
Suravee Suthikulpanit May 5, 2022, 3:57 a.m. UTC | #2
Maxim

On 5/4/22 7:32 PM, Maxim Levitsky wrote:
> Well strictly speaking, another thing that has to be done, other that removing the inhibit,
> is to 'hide' the AVIC's private memslot if one of vCPUs is in x2apic mode,
> although not doing this doesn't cause any harm as the guest is not supposed to poke at xAPIC
> mmio even when uses x2apic, and if it does it will get the normal AVIC acceleration,
> so probably it is better to not add any more complexity and leave it like that.

Agree, I'll note this detail in the comment above.

> ... 
> BTW, hardware wise, does 'X2APIC_MODE' keeps the emulation of the AVIC mmio, or
> not?

According to the AVIC documentation, when the AVIC is configured to use x2 APIC mode
the MMIO access method will be disabled. This is consistent with the Intel x2APIC specification,
where it states "accessing the MMIO interface when in x2APIC mode, the behavior is identical
to xAPIC in globally disabled state".

Regards,
Suravee
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index d07c58f06bed..3b6a96043633 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -92,12 +92,20 @@  static void avic_activate_vmcb(struct vcpu_svm *svm)
 	vmcb->control.avic_physical_id &= ~AVIC_PHYSICAL_MAX_INDEX_MASK;
 
 	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
-	if (apic_x2apic_mode(svm->vcpu.arch.apic)) {
+
+	/* Note:
+	 * KVM can support hybrid-x2AVIC mode, where KVM emulates x2APIC
+	 * MSR accesses, while interrupt injection to a running vCPU
+	 * can be achieve using AVIC doorbell.
+	 */
+	if (apic_x2apic_mode(svm->vcpu.arch.apic) &&
+	    (avic_mode == AVIC_MODE_X2)) {
 		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
 		vmcb->control.avic_physical_id |= X2AVIC_MAX_PHYSICAL_ID;
 		/* Disabling MSR intercept for x2APIC registers */
 		avic_set_x2apic_msr_interception(svm, false);
 	} else {
+		/* For xAVIC and hybrid-x2AVIC modes */
 		vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
 		/* Enabling MSR intercept for x2APIC registers */
 		avic_set_x2apic_msr_interception(svm, true);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 96a1fc1a1d1b..c0a3d4a1f3dc 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4041,7 +4041,6 @@  static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	struct kvm_cpuid_entry2 *best;
-	struct kvm *kvm = vcpu->kvm;
 
 	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
 				    boot_cpu_has(X86_FEATURE_XSAVE) &&
@@ -4073,14 +4072,6 @@  static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 			vcpu->arch.reserved_gpa_bits &= ~(1UL << (best->ebx & 0x3f));
 	}
 
-	if (kvm_vcpu_apicv_active(vcpu)) {
-		/*
-		 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
-		 * is exposed to the guest, disable AVIC.
-		 */
-		if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
-			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_X2APIC);
-	}
 	init_vmcb_after_set_cpuid(vcpu);
 }