[PATCH] pc: apic: fix touch LAPIC when irqchip is split

[PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Add -kernel_irqchip=split
./x86-run x86/eventinj.flat

qemu-system-x86_64 -enable-kvm -machine kernel_irqchip=split -cpu host
-device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc
none -serial stdio -device pci-testdev -kernel x86/eventinj.flat
enabling apic
paging enabled
cr0 = 80010011
cr3 = 7fff000
cr4 = 20
Sending vec 33 and 62 and mask one with TPR
irq1 running
irq1 running
After 33/62 TPR test
FAIL: TPR
irq0 running
irq0 running

Both irq1 and irq0 are executing twice.

kvm_entry: vcpu 0
kvm_exit: reason MSR_WRITE rip 0x401f33 info 0 0
kvm_apic: apic_write APIC_EOI = 0x0
kvm_eoi: apicid 0 vector 62
kvm_msr: msr_write 80b = 0x0
kvm_entry: vcpu 0
kvm_exit: reason PENDING_INTERRUPT rip 0x401f35 info 0 0
kvm_userspace_exit: reason KVM_EXIT_IRQ_WINDOW_OPEN (7)
kvm_inj_virq: irq 62
kvm_entry: vcpu 0
kvm_exit: reason IO_INSTRUCTION rip 0x4016ec info 3fd0008 0

>From the trace we can see there is an interrupt window exit 
after the first interrupt EOI(irq 62), and the same irq(62) 
is injected duplicately after the interrupt window.

QEMU does KVM_INTERRUPT(62) ioctl after KVM exits with 
KVM_EXIT_IRQ_WINDOW_OPEN, which QEMU requested while the
guest was printing.  The printing calls

serial_update_irq() -> qemu_irq_lower() -> qemu_set_irq() ->
gsi_handler() -> qemu_set_irq() -> pic_irq_request() ->
apic_deliver_pic_intr() -> kvm_handle_interrupt()

kvm_handle_interrupt() does

	interrupt_request |= CPU_INTERRUPT_HARD

which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), 
but that function uses stale information from APIC and injects 
62 again. If we synchronized the APIC, then the test would #GP, 
because there would be no injectable interrupt in LAPIC or PIC, 
so pic_read_irq() would return 15, thinking it was spurious.

This patch fix it by don't touch LAPIC if LAPIC is in kernel.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 hw/i386/pc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e31f70f..4f3d508 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -161,14 +161,16 @@ int cpu_get_pic_interrupt(CPUX86State *env)
     X86CPU *cpu = x86_env_get_cpu(env);
     int intno;
 
-    intno = apic_get_interrupt(cpu->apic_state);
-    if (intno >= 0) {
-        return intno;
-    }
-    /* read the irq from the PIC */
-    if (!apic_accept_pic_intr(cpu->apic_state)) {
-        return -1;
-    }
+    if (!kvm_irqchip_in_kernel()) {
+         intno = apic_get_interrupt(cpu->apic_state);
+         if (intno >= 0) {
+             return intno;
+         }
+         /* read the irq from the PIC */
+         if (!apic_accept_pic_intr(cpu->apic_state)) {
+             return -1;
+         }
+    }
 
     intno = pic_read_irq(isa_pic);
     return intno;
@@ -180,7 +182,7 @@ static void pic_irq_request(void *opaque, int irq, int level)
     X86CPU *cpu = X86_CPU(cs);
 
     DPRINTF("pic_irqs: %s irq %d\n", level? "raise" : "lower", irq);
-    if (cpu->apic_state) {
+    if (cpu->apic_state && !kvm_irqchip_in_kernel()) {
         CPU_FOREACH(cs) {
             cpu = X86_CPU(cs);
             if (apic_accept_pic_intr(cpu->apic_state)) {
-- 
1.9.1

[Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Add -kernel_irqchip=split
./x86-run x86/eventinj.flat

qemu-system-x86_64 -enable-kvm -machine kernel_irqchip=split -cpu host
-device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc
none -serial stdio -device pci-testdev -kernel x86/eventinj.flat
enabling apic
paging enabled
cr0 = 80010011
cr3 = 7fff000
cr4 = 20
Sending vec 33 and 62 and mask one with TPR
irq1 running
irq1 running
After 33/62 TPR test
FAIL: TPR
irq0 running
irq0 running

Both irq1 and irq0 are executing twice.

kvm_entry: vcpu 0
kvm_exit: reason MSR_WRITE rip 0x401f33 info 0 0
kvm_apic: apic_write APIC_EOI = 0x0
kvm_eoi: apicid 0 vector 62
kvm_msr: msr_write 80b = 0x0
kvm_entry: vcpu 0
kvm_exit: reason PENDING_INTERRUPT rip 0x401f35 info 0 0
kvm_userspace_exit: reason KVM_EXIT_IRQ_WINDOW_OPEN (7)
kvm_inj_virq: irq 62
kvm_entry: vcpu 0
kvm_exit: reason IO_INSTRUCTION rip 0x4016ec info 3fd0008 0

>From the trace we can see there is an interrupt window exit 
after the first interrupt EOI(irq 62), and the same irq(62) 
is injected duplicately after the interrupt window.

QEMU does KVM_INTERRUPT(62) ioctl after KVM exits with 
KVM_EXIT_IRQ_WINDOW_OPEN, which QEMU requested while the
guest was printing.  The printing calls

serial_update_irq() -> qemu_irq_lower() -> qemu_set_irq() ->
gsi_handler() -> qemu_set_irq() -> pic_irq_request() ->
apic_deliver_pic_intr() -> kvm_handle_interrupt()

kvm_handle_interrupt() does

	interrupt_request |= CPU_INTERRUPT_HARD

which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), 
but that function uses stale information from APIC and injects 
62 again. If we synchronized the APIC, then the test would #GP, 
because there would be no injectable interrupt in LAPIC or PIC, 
so pic_read_irq() would return 15, thinking it was spurious.

This patch fix it by don't touch LAPIC if LAPIC is in kernel.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 hw/i386/pc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e31f70f..4f3d508 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -161,14 +161,16 @@ int cpu_get_pic_interrupt(CPUX86State *env)
     X86CPU *cpu = x86_env_get_cpu(env);
     int intno;
 
-    intno = apic_get_interrupt(cpu->apic_state);
-    if (intno >= 0) {
-        return intno;
-    }
-    /* read the irq from the PIC */
-    if (!apic_accept_pic_intr(cpu->apic_state)) {
-        return -1;
-    }
+    if (!kvm_irqchip_in_kernel()) {
+         intno = apic_get_interrupt(cpu->apic_state);
+         if (intno >= 0) {
+             return intno;
+         }
+         /* read the irq from the PIC */
+         if (!apic_accept_pic_intr(cpu->apic_state)) {
+             return -1;
+         }
+    }
 
     intno = pic_read_irq(isa_pic);
     return intno;
@@ -180,7 +182,7 @@ static void pic_irq_request(void *opaque, int irq, int level)
     X86CPU *cpu = X86_CPU(cs);
 
     DPRINTF("pic_irqs: %s irq %d\n", level? "raise" : "lower", irq);
-    if (cpu->apic_state) {
+    if (cpu->apic_state && !kvm_irqchip_in_kernel()) {
         CPU_FOREACH(cs) {
             cpu = X86_CPU(cs);
             if (apic_accept_pic_intr(cpu->apic_state)) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1473822299-6302-1-git-send-email-wanpeng.li@hotmail.com
Subject: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
9bcacc6 pc: apic: fix touch LAPIC when irqchip is split

=== OUTPUT BEGIN ===
Checking PATCH 1/1: pc: apic: fix touch LAPIC when irqchip is split...
ERROR: suspect code indent for conditional statements (4, 9)
#90: FILE: hw/i386/pc.c:164:
+    if (!kvm_irqchip_in_kernel()) {
+         intno = apic_get_interrupt(cpu->apic_state);

ERROR: suspect code indent for conditional statements (9, 13)
#92: FILE: hw/i386/pc.c:166:
+         if (intno >= 0) {
+             return intno;

ERROR: suspect code indent for conditional statements (9, 13)
#96: FILE: hw/i386/pc.c:170:
+         if (!apic_accept_pic_intr(cpu->apic_state)) {
+             return -1;

total: 3 errors, 0 warnings, 30 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Fam Zheng]

On Tue, 09/13 20:21, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 1473822299-6302-1-git-send-email-wanpeng.li@hotmail.com
> Subject: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 9bcacc6 pc: apic: fix touch LAPIC when irqchip is split
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: pc: apic: fix touch LAPIC when irqchip is split...
> ERROR: suspect code indent for conditional statements (4, 9)
> #90: FILE: hw/i386/pc.c:164:
> +    if (!kvm_irqchip_in_kernel()) {
> +         intno = apic_get_interrupt(cpu->apic_state);
       ^^^^^
Should be 4 spaces here.

> 
> ERROR: suspect code indent for conditional statements (9, 13)
> #92: FILE: hw/i386/pc.c:166:
> +         if (intno >= 0) {
> +             return intno;

Then this and later will be indented back by 1 column.

Fam

Re: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Wanpeng Li]

2016-09-14 11:40 GMT+08:00 Fam Zheng <famz@redhat.com>:
> On Tue, 09/13 20:21, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
>> Hi,
>>
>> Your series seems to have some coding style problems. See output below for
>> more information:
>>
>> Type: series
>> Message-id: 1473822299-6302-1-git-send-email-wanpeng.li@hotmail.com
>> Subject: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>>
>> BASE=base
>> n=1
>> total=$(git log --oneline $BASE.. | wc -l)
>> failed=0
>>
>> # Useful git options
>> git config --local diff.renamelimit 0
>> git config --local diff.renames True
>>
>> commits="$(git log --format=%H --reverse $BASE..)"
>> for c in $commits; do
>>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
>>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>>         failed=1
>>         echo
>>     fi
>>     n=$((n+1))
>> done
>>
>> exit $failed
>> === TEST SCRIPT END ===
>>
>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>> Switched to a new branch 'test'
>> 9bcacc6 pc: apic: fix touch LAPIC when irqchip is split
>>
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/1: pc: apic: fix touch LAPIC when irqchip is split...
>> ERROR: suspect code indent for conditional statements (4, 9)
>> #90: FILE: hw/i386/pc.c:164:
>> +    if (!kvm_irqchip_in_kernel()) {
>> +         intno = apic_get_interrupt(cpu->apic_state);
>        ^^^^^
> Should be 4 spaces here.
>
>>
>> ERROR: suspect code indent for conditional statements (9, 13)
>> #92: FILE: hw/i386/pc.c:166:
>> +         if (intno >= 0) {
>> +             return intno;
>
> Then this and later will be indented back by 1 column.

You are right, thanks Fam.

Regards,
Wanpeng Li

Re: [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Radim Krčmář]

2016-09-14 11:04+0800, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Add -kernel_irqchip=split
> ./x86-run x86/eventinj.flat
> 
> qemu-system-x86_64 -enable-kvm -machine kernel_irqchip=split -cpu host
> -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc
> none -serial stdio -device pci-testdev -kernel x86/eventinj.flat
> enabling apic
> paging enabled
> cr0 = 80010011
> cr3 = 7fff000
> cr4 = 20
> Sending vec 33 and 62 and mask one with TPR
> irq1 running
> irq1 running
> After 33/62 TPR test
> FAIL: TPR
> irq0 running
> irq0 running
> 
> Both irq1 and irq0 are executing twice.
> 
> kvm_entry: vcpu 0
> kvm_exit: reason MSR_WRITE rip 0x401f33 info 0 0
> kvm_apic: apic_write APIC_EOI = 0x0
> kvm_eoi: apicid 0 vector 62
> kvm_msr: msr_write 80b = 0x0
> kvm_entry: vcpu 0
> kvm_exit: reason PENDING_INTERRUPT rip 0x401f35 info 0 0
> kvm_userspace_exit: reason KVM_EXIT_IRQ_WINDOW_OPEN (7)
> kvm_inj_virq: irq 62
> kvm_entry: vcpu 0
> kvm_exit: reason IO_INSTRUCTION rip 0x4016ec info 3fd0008 0
> 
> From the trace we can see there is an interrupt window exit 
> after the first interrupt EOI(irq 62), and the same irq(62) 
> is injected duplicately after the interrupt window.
> 
> QEMU does KVM_INTERRUPT(62) ioctl after KVM exits with 
> KVM_EXIT_IRQ_WINDOW_OPEN, which QEMU requested while the
> guest was printing.  The printing calls
> 
> serial_update_irq() -> qemu_irq_lower() -> qemu_set_irq() ->
> gsi_handler() -> qemu_set_irq() -> pic_irq_request() ->
> apic_deliver_pic_intr() -> kvm_handle_interrupt()
> 
> kvm_handle_interrupt() does
> 
> 	interrupt_request |= CPU_INTERRUPT_HARD
> 
> which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), 
> but that function uses stale information from APIC and injects 
> 62 again. If we synchronized the APIC, then the test would #GP, 
> because there would be no injectable interrupt in LAPIC or PIC, 
> so pic_read_irq() would return 15, thinking it was spurious.
> 
> This patch fix it by don't touch LAPIC if LAPIC is in kernel.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

(The code is horribly tangled, so I might have missed something.)

Re: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Radim Krčmář]

2016-09-14 11:04+0800, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Add -kernel_irqchip=split
> ./x86-run x86/eventinj.flat
> 
> qemu-system-x86_64 -enable-kvm -machine kernel_irqchip=split -cpu host
> -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc
> none -serial stdio -device pci-testdev -kernel x86/eventinj.flat
> enabling apic
> paging enabled
> cr0 = 80010011
> cr3 = 7fff000
> cr4 = 20
> Sending vec 33 and 62 and mask one with TPR
> irq1 running
> irq1 running
> After 33/62 TPR test
> FAIL: TPR
> irq0 running
> irq0 running
> 
> Both irq1 and irq0 are executing twice.
> 
> kvm_entry: vcpu 0
> kvm_exit: reason MSR_WRITE rip 0x401f33 info 0 0
> kvm_apic: apic_write APIC_EOI = 0x0
> kvm_eoi: apicid 0 vector 62
> kvm_msr: msr_write 80b = 0x0
> kvm_entry: vcpu 0
> kvm_exit: reason PENDING_INTERRUPT rip 0x401f35 info 0 0
> kvm_userspace_exit: reason KVM_EXIT_IRQ_WINDOW_OPEN (7)
> kvm_inj_virq: irq 62
> kvm_entry: vcpu 0
> kvm_exit: reason IO_INSTRUCTION rip 0x4016ec info 3fd0008 0
> 
> From the trace we can see there is an interrupt window exit 
> after the first interrupt EOI(irq 62), and the same irq(62) 
> is injected duplicately after the interrupt window.
> 
> QEMU does KVM_INTERRUPT(62) ioctl after KVM exits with 
> KVM_EXIT_IRQ_WINDOW_OPEN, which QEMU requested while the
> guest was printing.  The printing calls
> 
> serial_update_irq() -> qemu_irq_lower() -> qemu_set_irq() ->
> gsi_handler() -> qemu_set_irq() -> pic_irq_request() ->
> apic_deliver_pic_intr() -> kvm_handle_interrupt()
> 
> kvm_handle_interrupt() does
> 
> 	interrupt_request |= CPU_INTERRUPT_HARD
> 
> which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), 
> but that function uses stale information from APIC and injects 
> 62 again. If we synchronized the APIC, then the test would #GP, 
> because there would be no injectable interrupt in LAPIC or PIC, 
> so pic_read_irq() would return 15, thinking it was spurious.
> 
> This patch fix it by don't touch LAPIC if LAPIC is in kernel.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

(The code is horribly tangled, so I might have missed something.)

Re: [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Radim Krčmář]

2016-09-14 16:02+0200, Radim Krčmář:
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> (The code is horribly tangled, so I might have missed something.)

This reply should have went for the v2, sorry.

Re: [Qemu-devel] [PATCH] pc: apic: fix touch LAPIC when irqchip is split

[Radim Krčmář]

2016-09-14 16:02+0200, Radim Krčmář:
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> (The code is horribly tangled, so I might have missed something.)

This reply should have went for the v2, sorry.