[PATCH v2 0/3] KVM: SVM: Fix guest not booting w/ AVIC enabled

[PATCH v2 0/3] KVM: SVM: Fix guest not booting w/ AVIC enabled

[Suravee Suthikulpanit]

Certain QEMU options fails to boot VM guest w/ SVM AVIC enabled
(e.g. modprobe kvm_amd avic=1). Investigation shows that this mainly
due to AVIC hardware does not trap into hypervisor when guest OS
writes to APIC_EOI register. 

The boot hang is caused by missing timer interrupt when using in-kernel
PIT model (e.g. launch qemu w/ '-no-hpet' option) since it requires
irq acknowledgmen before injecting another interrupt in case
irq re-injection is enabled (normally default).

Changes from V1 (https://lkml.org/lkml/2017/9/5/826)
 * Consolidate irqchip_split() check to only one place (per Radim). 

Suravee Suthikulpanit (3):
  KVM: SVM: Refactor AVIC vcpu initialization into avic_init_vcpu()
  KVM: Add struct kvm_vcpu pointer parameter to get_enable_apicv()
  KVM: SVM: Add irqchip_split() checks before enabling AVIC

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/svm.c              | 43 ++++++++++++++++++++++++++++-------------
 arch/x86/kvm/vmx.c              |  2 +-
 arch/x86/kvm/x86.c              |  2 +-
 4 files changed, 33 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/3] KVM: SVM: Refactor AVIC vcpu initialization into avic_init_vcpu()

[Suravee Suthikulpanit]

Preparing the base code for subsequent changes. This does not change
existing logic.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index af256b7..316edbf 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1587,6 +1587,23 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 		avic_update_vapic_bar(svm, APIC_DEFAULT_PHYS_BASE);
 }
 
+static int avic_init_vcpu(struct vcpu_svm *svm)
+{
+	int ret;
+
+	if (!avic)
+		return 0;
+
+	ret = avic_init_backing_page(&svm->vcpu);
+	if (ret)
+		return ret;
+
+	INIT_LIST_HEAD(&svm->ir_list);
+	spin_lock_init(&svm->ir_list_lock);
+
+	return ret;
+}
+
 static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 {
 	struct vcpu_svm *svm;
@@ -1623,14 +1640,9 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 	if (!hsave_page)
 		goto free_page3;
 
-	if (avic) {
-		err = avic_init_backing_page(&svm->vcpu);
-		if (err)
-			goto free_page4;
-
-		INIT_LIST_HEAD(&svm->ir_list);
-		spin_lock_init(&svm->ir_list_lock);
-	}
+	err = avic_init_vcpu(svm);
+	if (err)
+		goto free_page4;
 
 	/* We initialize this flag to true to make sure that the is_running
 	 * bit would be set the first time the vcpu is loaded.
-- 
1.8.3.1

[PATCH v2 2/3] KVM: Add struct kvm_vcpu pointer parameter to get_enable_apicv()

[Suravee Suthikulpanit]

Modify struct kvm_x86_ops.arch.apicv_active() to take struct kvm_vcpu
pointer as parameter in preparation to subsequent changes.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 arch/x86/kvm/svm.c              | 2 +-
 arch/x86/kvm/vmx.c              | 2 +-
 arch/x86/kvm/x86.c              | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 92c9032..bfe7b2a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -969,7 +969,7 @@ struct kvm_x86_ops {
 	void (*enable_nmi_window)(struct kvm_vcpu *vcpu);
 	void (*enable_irq_window)(struct kvm_vcpu *vcpu);
 	void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr);
-	bool (*get_enable_apicv)(void);
+	bool (*get_enable_apicv)(struct kvm_vcpu *vcpu);
 	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
 	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
 	void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 316edbf..d1b3eb4 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4386,7 +4386,7 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
 	return;
 }
 
-static bool svm_get_enable_apicv(void)
+static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu)
 {
 	return avic;
 }
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6ef294..8a7ab16 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4983,7 +4983,7 @@ static void vmx_disable_intercept_msr_x2apic(u32 msr, int type, bool apicv_activ
 	}
 }
 
-static bool vmx_get_enable_apicv(void)
+static bool vmx_get_enable_apicv(struct kvm_vcpu *vcpu)
 {
 	return enable_apicv;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 272320e..946875c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7935,7 +7935,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 	BUG_ON(vcpu->kvm == NULL);
 	kvm = vcpu->kvm;
 
-	vcpu->arch.apicv_active = kvm_x86_ops->get_enable_apicv();
+	vcpu->arch.apicv_active = kvm_x86_ops->get_enable_apicv(vcpu);
 	vcpu->arch.pv.pv_unhalted = false;
 	vcpu->arch.emulate_ctxt.ops = &emulate_ops;
 	if (!irqchip_in_kernel(kvm) || kvm_vcpu_is_reset_bsp(vcpu))
-- 
1.8.3.1

[PATCH v2 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC

[Suravee Suthikulpanit]

SVM AVIC hardware accelerates guest write to APIC_EOI register
(for edge-trigger interrupt), which means it does not trap to KVM.

So, only enable SVM AVIC only in split irqchip mode.
(e.g. launching qemu w/ option '-machine kernel_irqchip=split').

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d1b3eb4..a7b96e7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1176,7 +1176,6 @@ static void avic_init_vmcb(struct vcpu_svm *svm)
 	vmcb->control.avic_physical_id = ppa & AVIC_HPA_MASK;
 	vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID_COUNT;
 	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
-	svm->vcpu.arch.apicv_active = true;
 }
 
 static void init_vmcb(struct vcpu_svm *svm)
@@ -1292,7 +1291,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 		set_intercept(svm, INTERCEPT_PAUSE);
 	}
 
-	if (avic)
+	if (kvm_vcpu_apicv_active(&svm->vcpu))
 		avic_init_vmcb(svm);
 
 	/*
@@ -1594,6 +1593,12 @@ static int avic_init_vcpu(struct vcpu_svm *svm)
 	if (!avic)
 		return 0;
 
+	if (!kvm_vcpu_apicv_active(&svm->vcpu)) {
+		pr_debug("%s: Disable AVIC due to non-split irqchip.\n",
+			      __func__);
+		return 0;
+	}
+
 	ret = avic_init_backing_page(&svm->vcpu);
 	if (ret)
 		return ret;
@@ -4388,7 +4393,7 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
 
 static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu)
 {
-	return avic;
+	return avic && irqchip_split(vcpu->kvm);
 }
 
 static void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
@@ -4405,7 +4410,7 @@ static void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	struct vmcb *vmcb = svm->vmcb;
 
-	if (!avic)
+	if (!kvm_vcpu_apicv_active(&svm->vcpu))
 		return;
 
 	vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
-- 
1.8.3.1

Re: [PATCH v2 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC

[Radim Krčmář]

2017-09-12 10:42-0500, Suravee Suthikulpanit:
> SVM AVIC hardware accelerates guest write to APIC_EOI register
> (for edge-trigger interrupt), which means it does not trap to KVM.
> 
> So, only enable SVM AVIC only in split irqchip mode.
> (e.g. launching qemu w/ option '-machine kernel_irqchip=split').
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index d1b3eb4..a7b96e7 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1176,7 +1176,6 @@ static void avic_init_vmcb(struct vcpu_svm *svm)
>  	vmcb->control.avic_physical_id = ppa & AVIC_HPA_MASK;
>  	vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID_COUNT;
>  	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
> -	svm->vcpu.arch.apicv_active = true;
>  }
>  
>  static void init_vmcb(struct vcpu_svm *svm)
> @@ -1292,7 +1291,7 @@ static void init_vmcb(struct vcpu_svm *svm)
>  		set_intercept(svm, INTERCEPT_PAUSE);
>  	}
>  
> -	if (avic)
> +	if (kvm_vcpu_apicv_active(&svm->vcpu))
>  		avic_init_vmcb(svm);
>  
>  	/*
> @@ -1594,6 +1593,12 @@ static int avic_init_vcpu(struct vcpu_svm *svm)
>  	if (!avic)
>  		return 0;
>  
> +	if (!kvm_vcpu_apicv_active(&svm->vcpu)) {
> +		pr_debug("%s: Disable AVIC due to non-split irqchip.\n",
> +			      __func__);

We need to have an extra condition just because of this print ...
I removed the print altogether when applying -- I thought more about
that and it was aimed at people who wonder why AVIC was suddenly
disabled and it's unlikely that they will enable a debug message without
already knowing the reason,

thanks.

> @@ -4388,7 +4393,7 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
>  
>  static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu)
>  {

A close contender was pr_info_once() here:

	if (avic && !irqchip_split(vcpu->kvm))
		pr_info_once(...)

> -	return avic;
> +	return avic && irqchip_split(vcpu->kvm);

>  }
>  
>  static void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
> @@ -4405,7 +4410,7 @@ static void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct vmcb *vmcb = svm->vmcb;
>  
> -	if (!avic)
> +	if (!kvm_vcpu_apicv_active(&svm->vcpu))
>  		return;
>  
>  	vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
> -- 
> 1.8.3.1
> 

Re: [PATCH v2 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC

[Suravee Suthikulpanit]

Radim,

On 9/14/17 08:20, Radim Krčmář wrote:
>> @@ -1594,6 +1593,12 @@ static int avic_init_vcpu(struct vcpu_svm *svm)
>>  	if (!avic)
>>  		return 0;
>>
>> +	if (!kvm_vcpu_apicv_active(&svm->vcpu)) {
>> +		pr_debug("%s: Disable AVIC due to non-split irqchip.\n",
>> +			      __func__);
> We need to have an extra condition just because of this print ...
> I removed the print altogether when applying -- I thought more about
> that and it was aimed at people who wonder why AVIC was suddenly
> disabled and it's unlikely that they will enable a debug message without
> already knowing the reason,

Make sense. Thanks.

>
> thanks.
>
>> @@ -4388,7 +4393,7 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set)
>>
>>  static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu)
>>  {
> A close contender was pr_info_once() here:
>
> 	if (avic && !irqchip_split(vcpu->kvm))
> 		pr_info_once(...)
>

Looks good.

Thanks,
Suravee

Re: [PATCH v2 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC

[Suravee Suthikulpanit]

Radim,

On 9/14/17 08:23, Suravee Suthikulpanit wrote:
> Radim,
>
> On 9/14/17 08:20, Radim Krčmář wrote:
>>> @@ -1594,6 +1593,12 @@ static int avic_init_vcpu(struct vcpu_svm *svm)
>>>      if (!avic)
>>>          return 0;
>>>
>>> +    if (!kvm_vcpu_apicv_active(&svm->vcpu)) {
>>> +        pr_debug("%s: Disable AVIC due to non-split irqchip.\n",
>>> +                  __func__);
>> We need to have an extra condition just because of this print ...
>> I removed the print altogether when applying -- I thought more about
>> that and it was aimed at people who wonder why AVIC was suddenly
>> disabled and it's unlikely that they will enable a debug message without
>> already knowing the reason,
>
> Make sense. Thanks.
>
>>
>> thanks.
>>
>>> @@ -4388,7 +4393,7 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu
>>> *vcpu, bool set)
>>>
>>>  static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu)
>>>  {
>> A close contender was pr_info_once() here:
>>
>>     if (avic && !irqchip_split(vcpu->kvm))
>>         pr_info_once(...)
>>
>
> Looks good.

Actually, thinking about it again, this would not work either since 
pr_xxx_once() would only print the message once per loading of kvm_amd module. 
However, we would prefer the message to be printed per VM initialization. I also 
tried adding the check and print this message in the kvm_x86_ops.vm_init(), but 
this also does not work since the vm_init() function is called before the 
kvm_vm_ioctl_enable_cap(KVM_CAP_SPLIT_IRQCHIP).

Therefore, pr_info() might be better here, even though this would get print per 
VCPU initialization. Any other suggestions?

Thanks,
Suravee

Re: [PATCH v2 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC

[Paolo Bonzini]

On 15/09/2017 21:21, Suravee Suthikulpanit wrote:
>>>>
>>>>
>>>>  static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu)
>>>>  {
>>> A close contender was pr_info_once() here:
>>>
>>>     if (avic && !irqchip_split(vcpu->kvm))
>>>         pr_info_once(...)
>>>
>>
>> Looks good.
> 
> Actually, thinking about it again, this would not work either since
> pr_xxx_once() would only print the message once per loading of kvm_amd
> module. However, we would prefer the message to be printed per VM
> initialization. I also tried adding the check and print this message in
> the kvm_x86_ops.vm_init(), but this also does not work since the
> vm_init() function is called before the
> kvm_vm_ioctl_enable_cap(KVM_CAP_SPLIT_IRQCHIP).
> 
> Therefore, pr_info() might be better here, even though this would get
> print per VCPU initialization. Any other suggestions?

My suggestion is to make it pr_info_once.  No one looks at dmesg anyway
unless things go very wrong.

Paolo