[REGRESSION?] -no-kvm-irqchip broken with kvm.git

[REGRESSION?] -no-kvm-irqchip broken with kvm.git

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

Hi,

I'm currently facing problems to get kvm working with -no-kvm-irqchip.
It used to be fine up to kvm-71, but with latest git my Linux guest
stalls during early boot:

[   22.631200] Freeing unused kernel memory: 324k freed
[   22.857098] SCSI subsystem initialized
[   22.892601] scsi0 : ata_piix
[   22.903604] scsi1 : ata_piix
[   22.904473] ata1: PATA max MWDMA2 cmd 0x00000000000101f0 ctl 0x00000000000103f6 bmdma 0x000000000001c000 irq 14
[   22.917692] ata2: PATA max MWDMA2 cmd 0x0000000000010170 ctl 0x0000000000010376 bmdma 0x000000000001c008 irq 15

This seems to be a kernel-side issue, replacing the module set with the
one that came with -71 fixes the issue. Maybe due to
vmx_complete_interrupts refactoring?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [REGRESSION?] -no-kvm-irqchip broken with kvm.git

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

Jan Kiszka wrote:
> Hi,
> 
> I'm currently facing problems to get kvm working with -no-kvm-irqchip.
> It used to be fine up to kvm-71, but with latest git my Linux guest
> stalls during early boot:
> 
> [   22.631200] Freeing unused kernel memory: 324k freed
> [   22.857098] SCSI subsystem initialized
> [   22.892601] scsi0 : ata_piix
> [   22.903604] scsi1 : ata_piix
> [   22.904473] ata1: PATA max MWDMA2 cmd 0x00000000000101f0 ctl 0x00000000000103f6 bmdma 0x000000000001c000 irq 14
> [   22.917692] ata2: PATA max MWDMA2 cmd 0x0000000000010170 ctl 0x0000000000010376 bmdma 0x000000000001c008 irq 15
> 
> This seems to be a kernel-side issue, replacing the module set with the
> one that came with -71 fixes the issue. Maybe due to
> vmx_complete_interrupts refactoring?

Looks like my suspect was right: Kicking vmx_complete_interrupts out of
vmx_vcpu_run makes -no-kvm-irqchip work again.

Avi, could it be that 1c0f4f5011829dac96347b5f84ba37c2252e1e08 left
do_interrupt_requests outdated behind? Or is vmx_complete_interrupts
incorrectly queuing interrupts in the -no-kvm-irqchip case?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [REGRESSION?] -no-kvm-irqchip broken with kvm.git

[Avi Kivity]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>   
>> Hi,
>>
>> I'm currently facing problems to get kvm working with -no-kvm-irqchip.
>> It used to be fine up to kvm-71, but with latest git my Linux guest
>> stalls during early boot:
>>
>> [   22.631200] Freeing unused kernel memory: 324k freed
>> [   22.857098] SCSI subsystem initialized
>> [   22.892601] scsi0 : ata_piix
>> [   22.903604] scsi1 : ata_piix
>> [   22.904473] ata1: PATA max MWDMA2 cmd 0x00000000000101f0 ctl 0x00000000000103f6 bmdma 0x000000000001c000 irq 14
>> [   22.917692] ata2: PATA max MWDMA2 cmd 0x0000000000010170 ctl 0x0000000000010376 bmdma 0x000000000001c008 irq 15
>>
>> This seems to be a kernel-side issue, replacing the module set with the
>> one that came with -71 fixes the issue. Maybe due to
>> vmx_complete_interrupts refactoring?
>>     
>
> Looks like my suspect was right: Kicking vmx_complete_interrupts out of
> vmx_vcpu_run makes -no-kvm-irqchip work again.
>
> Avi, could it be that 1c0f4f5011829dac96347b5f84ba37c2252e1e08 left
> do_interrupt_requests outdated behind? Or is vmx_complete_interrupts
> incorrectly queuing interrupts in the -no-kvm-irqchip case?
>
>   

I thought I tested out -no-kvm-irqchip, but apprently not well enough.  
I'll try to see what went wrong.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [REGRESSION?] -no-kvm-irqchip broken with kvm.git

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>  
>>> Hi,
>>>
>>> I'm currently facing problems to get kvm working with -no-kvm-irqchip.
>>> It used to be fine up to kvm-71, but with latest git my Linux guest
>>> stalls during early boot:
>>>
>>> [   22.631200] Freeing unused kernel memory: 324k freed
>>> [   22.857098] SCSI subsystem initialized
>>> [   22.892601] scsi0 : ata_piix
>>> [   22.903604] scsi1 : ata_piix
>>> [   22.904473] ata1: PATA max MWDMA2 cmd 0x00000000000101f0 ctl
>>> 0x00000000000103f6 bmdma 0x000000000001c000 irq 14
>>> [   22.917692] ata2: PATA max MWDMA2 cmd 0x0000000000010170 ctl
>>> 0x0000000000010376 bmdma 0x000000000001c008 irq 15
>>>
>>> This seems to be a kernel-side issue, replacing the module set with the
>>> one that came with -71 fixes the issue. Maybe due to
>>> vmx_complete_interrupts refactoring?
>>>     
>>
>> Looks like my suspect was right: Kicking vmx_complete_interrupts out of
>> vmx_vcpu_run makes -no-kvm-irqchip work again.
>>
>> Avi, could it be that 1c0f4f5011829dac96347b5f84ba37c2252e1e08 left
>> do_interrupt_requests outdated behind? Or is vmx_complete_interrupts
>> incorrectly queuing interrupts in the -no-kvm-irqchip case?
>>
>>   
> 
> I thought I tested out -no-kvm-irqchip, but apprently not well enough. 
> I'll try to see what went wrong.

Should this issue have been fixed meanwhile? I just gave latest git a
try and - as far as I recall my tests before holiday correctly - things
look the same. At least some Linux 2.6.23 kernel still hangs here during
early boot with -no-kvm-irqchip.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [REGRESSION?] -no-kvm-irqchip broken with kvm.git

[Avi Kivity]

Jan Kiszka wrote:
>> I thought I tested out -no-kvm-irqchip, but apprently not well enough. 
>> I'll try to see what went wrong.
>>     
>
> Should this issue have been fixed meanwhile? I just gave latest git a
> try and - as far as I recall my tests before holiday correctly - things
> look the same. At least some Linux 2.6.23 kernel still hangs here during
> early boot with -no-kvm-irqchip.
>
>   


No, I was on vacation, and haven't recovered completely since I've returned.

-- 
error compiling committee.c: too many arguments to function

Re: [REGRESSION?] -no-kvm-irqchip broken with kvm.git

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

Avi Kivity wrote:
> Jan Kiszka wrote:
>>> I thought I tested out -no-kvm-irqchip, but apprently not well
>>> enough. I'll try to see what went wrong.
>>>     
>>
>> Should this issue have been fixed meanwhile? I just gave latest git a
>> try and - as far as I recall my tests before holiday correctly - things
>> look the same. At least some Linux 2.6.23 kernel still hangs here during
>> early boot with -no-kvm-irqchip.
>>
>>   
> 
> 
> No, I was on vacation, and haven't recovered completely since I've
> returned.
> 

No problem, same here. :)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[PATCH] fix -no-kvm-irqchip regression

[Andrea Arcangeli]

On Wed, Aug 13, 2008 at 10:05:06AM +0200, Jan Kiszka wrote:
> Should this issue have been fixed meanwhile? I just gave latest git a
> try and - as far as I recall my tests before holiday correctly - things
> look the same. At least some Linux 2.6.23 kernel still hangs here during
> early boot with -no-kvm-irqchip.

The trouble was that clearing the idt_vectoring_info before handling
the exit_reason would lead to the handle_exception to fail setting the
irq_pending bit because is_external_interrupt was run on zero instead
of the vmcs IDT_VECTORING_INFO_FIELD, so it didn't notice it was an
external interrupt generating the exit.

This makes the userland irqchip code work again for me, there seem to
be no good reason to clear this value before returning in guest mode
and only setting it to zero didn't look an effective debugging aid, so
it's a microoptimization for the kernel irqchip too.

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 337670b..3c82593 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2890,13 +2890,10 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 			kvm_queue_exception_e(&vmx->vcpu, vector, error);
 		} else
 			kvm_queue_exception(&vmx->vcpu, vector);
-		vmx->idt_vectoring_info = 0;
 	}
 	kvm_clear_interrupt_queue(&vmx->vcpu);
-	if (idtv_info_valid && type == INTR_TYPE_EXT_INTR) {
+	if (idtv_info_valid && type == INTR_TYPE_EXT_INTR)
 		kvm_queue_interrupt(&vmx->vcpu, vector);
-		vmx->idt_vectoring_info = 0;
-	}
 }
 
 static void vmx_intr_assist(struct kvm_vcpu *vcpu)

Re: [PATCH] fix -no-kvm-irqchip regression

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

Andrea Arcangeli wrote:
> On Wed, Aug 13, 2008 at 10:05:06AM +0200, Jan Kiszka wrote:
>> Should this issue have been fixed meanwhile? I just gave latest git a
>> try and - as far as I recall my tests before holiday correctly - things
>> look the same. At least some Linux 2.6.23 kernel still hangs here during
>> early boot with -no-kvm-irqchip.
> 
> The trouble was that clearing the idt_vectoring_info before handling
> the exit_reason would lead to the handle_exception to fail setting the
> irq_pending bit because is_external_interrupt was run on zero instead
> of the vmcs IDT_VECTORING_INFO_FIELD, so it didn't notice it was an
> external interrupt generating the exit.
> 
> This makes the userland irqchip code work again for me, there seem to
> be no good reason to clear this value before returning in guest mode
> and only setting it to zero didn't look an effective debugging aid, so
> it's a microoptimization for the kernel irqchip too.
> 
> Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>

Solves the issue here as well. Thanks.

Tested-by: Jan Kiszka <jan.kiszka@web.de>

> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 337670b..3c82593 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2890,13 +2890,10 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
>  			kvm_queue_exception_e(&vmx->vcpu, vector, error);
>  		} else
>  			kvm_queue_exception(&vmx->vcpu, vector);
> -		vmx->idt_vectoring_info = 0;
>  	}
>  	kvm_clear_interrupt_queue(&vmx->vcpu);
> -	if (idtv_info_valid && type == INTR_TYPE_EXT_INTR) {
> +	if (idtv_info_valid && type == INTR_TYPE_EXT_INTR)
>  		kvm_queue_interrupt(&vmx->vcpu, vector);
> -		vmx->idt_vectoring_info = 0;
> -	}
>  }
>  
>  static void vmx_intr_assist(struct kvm_vcpu *vcpu)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] fix -no-kvm-irqchip regression

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

Andrea Arcangeli wrote:
> On Wed, Aug 13, 2008 at 10:05:06AM +0200, Jan Kiszka wrote:
>   
>> Should this issue have been fixed meanwhile? I just gave latest git a
>> try and - as far as I recall my tests before holiday correctly - things
>> look the same. At least some Linux 2.6.23 kernel still hangs here during
>> early boot with -no-kvm-irqchip.
>>     
>
> The trouble was that clearing the idt_vectoring_info before handling
> the exit_reason would lead to the handle_exception to fail setting the
> irq_pending bit because is_external_interrupt was run on zero instead
> of the vmcs IDT_VECTORING_INFO_FIELD, so it didn't notice it was an
> external interrupt generating the exit.
>
> This makes the userland irqchip code work again for me, there seem to
> be no good reason to clear this value before returning in guest mode
> and only setting it to zero didn't look an effective debugging aid, so
> it's a microoptimization for the kernel irqchip too.
>   

I don't like the usage of idt_vectoring_info; so I've switched the 
!irqchip_in_kernel path to use the interrupt queue.  Attached patch 
boots Windows XP ACPI here.

Will push it out once it passes regression testing.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.


[-- Attachment #2: 0001-KVM-VMX-Use-interrupt-queue-for-irqchip_in_kernel.patch --]
[-- Type: text/plain, Size: 1569 bytes --]

>From 37304c6f9ced347cf013bcd4bf808d6fd4fb6ce1 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi@qumranet.com>
Date: Thu, 14 Aug 2008 11:13:16 +0300
Subject: [PATCH] KVM: VMX: Use interrupt queue for !irqchip_in_kernel

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 arch/x86/kvm/vmx.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 337670b..8693fb5 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2173,7 +2173,7 @@ static void kvm_do_inject_irq(struct kvm_vcpu *vcpu)
 	clear_bit(bit_index, &vcpu->arch.irq_pending[word_index]);
 	if (!vcpu->arch.irq_pending[word_index])
 		clear_bit(word_index, &vcpu->arch.irq_summary);
-	vmx_inject_irq(vcpu, irq);
+	kvm_queue_interrupt(vcpu, irq);
 }
 
 
@@ -2187,13 +2187,12 @@ static void do_interrupt_requests(struct kvm_vcpu *vcpu,
 		 (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0);
 
 	if (vcpu->arch.interrupt_window_open &&
-	    vcpu->arch.irq_summary &&
-	    !(vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) & INTR_INFO_VALID_MASK))
-		/*
-		 * If interrupts enabled, and not blocked by sti or mov ss. Good.
-		 */
+	    vcpu->arch.irq_summary && !vcpu->arch.interrupt.pending)
 		kvm_do_inject_irq(vcpu);
 
+	if (vcpu->arch.interrupt_window_open && vcpu->arch.interrupt.pending)
+		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
+
 	cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
 	if (!vcpu->arch.interrupt_window_open &&
 	    (vcpu->arch.irq_summary || kvm_run->request_interrupt_window))
-- 
1.5.6.3

Re: [PATCH] fix -no-kvm-irqchip regression

[Andrea Arcangeli]

On Thu, Aug 14, 2008 at 11:19:37AM +0300, Avi Kivity wrote:
> I don't like the usage of idt_vectoring_info; so I've switched the 
> !irqchip_in_kernel path to use the interrupt queue.  Attached patch boots 
> Windows XP ACPI here.
>
> Will push it out once it passes regression testing.

Works well in my testing too, thanks!

Re: [PATCH] fix -no-kvm-irqchip regression

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

Andrea Arcangeli wrote:
> On Thu, Aug 14, 2008 at 11:19:37AM +0300, Avi Kivity wrote:
>> I don't like the usage of idt_vectoring_info; so I've switched the 
>> !irqchip_in_kernel path to use the interrupt queue.  Attached patch boots 
>> Windows XP ACPI here.
>>
>> Will push it out once it passes regression testing.
> 
> Works well in my testing too, thanks!

Fine for me as well.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]