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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
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
Preparing the base code for subsequent changes. This does not change
existing logic.
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> 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
>
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
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
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