KVM: Windows 64-bit troubles with user space irqchip

DISCUSSION Jan Kiszka 4 participants 35 messages Feb 1, 2011 – Feb 6, 2011 KVM

Share & tools

Jan Kiszka

author

2011-02-01 18:02 UTC #0 lore raw

Hi,

testing my KVM patches, I noticed that none of the 64-bit Windows versions I have around (early Win7 & 2003 server) boot in KVM mode when using 2 or more VCPUs and the user space irqchip. This applies to both upstream KVM and qemu-kvm, with our without any of my current patches. A subtle difference in the APIC/IOAPIC emulation?

Can anyone confirm this?

Jan

PS: Looks like they boot fine without CONFIG_IOTHREAD (over upstream).

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-02 11:55 UTC #1 lore raw

Quoted 10 lines · Jan Kiszka

Hi,
testing my KVM patches, I noticed that none of the 64-bit Windows versions I have around (early Win7 & 2003 server) boot in KVM mode when using 2 or more VCPUs and the user space irqchip. This applies to both upstream KVM and qemu-kvm, with our without any of my current patches. A subtle difference in the APIC/IOAPIC emulation?
Can anyone confirm this?

Just booted windows7-64 on qemu-kvm -no-kvm-irqchip + latest kernel here.

--
Gleb.

Jan Kiszka

author

2011-02-02 11:59 UTC #2 lore raw

Quoted 9 lines · Gleb Natapov

Quoted 8 lines · Jan Kiszka
Hi,
testing my KVM patches, I noticed that none of the 64-bit Windows versions I have around (early Win7 & 2003 server) boot in KVM mode when using 2 or more VCPUs and the user space irqchip. This applies to both upstream KVM and qemu-kvm, with our without any of my current patches. A subtle difference in the APIC/IOAPIC emulation?

Quoted 3 lines

Quoted 2 lines
Can anyone confirm this?
Just booted windows7-64 on qemu-kvm -no-kvm-irqchip + latest kernel here.

Strange. -smp 2?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-02 12:35 UTC #3 lore raw

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
testing my KVM patches, I noticed that none of the 64-bit Windows versions I have around (early Win7 & 2003 server) boot in KVM mode when using 2 or more VCPUs and the user space irqchip. This applies to both upstream KVM and qemu-kvm, with our without any of my current patches. A subtle difference in the APIC/IOAPIC emulation?

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
Can anyone confirm this?
Just booted windows7-64 on qemu-kvm -no-kvm-irqchip + latest kernel here.
Strange. -smp 2?

Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.

--
Gleb.

Jan Kiszka

author

2011-02-02 12:50 UTC #4 lore raw

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Hi,
testing my KVM patches, I noticed that none of the 64-bit Windows versions I have around (early Win7 & 2003 server) boot in KVM mode when using 2 or more VCPUs and the user space irqchip. This applies to both upstream KVM and qemu-kvm, with our without any of my current patches. A subtle difference in the APIC/IOAPIC emulation?

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
Can anyone confirm this?
Just booted windows7-64 on qemu-kvm -no-kvm-irqchip + latest kernel here.
Strange. -smp 2?
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.

Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-02 13:05 UTC #5 lore raw

Quoted 5 lines · Jan Kiszka

Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).

Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-02-02 13:10 UTC #6 lore raw

Quoted 10 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?

-ECOMMITNOTFOUND, neither in qemu-kvm nor upstream.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-02 13:11 UTC #7 lore raw

Quoted 5 lines · Jan Kiszka

Quoted 4 lines · Avi Kivity
Quoted 3 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.

Quoted 9 lines

Quoted 6 lines
Quoted 2 lines
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
-ECOMMITNOTFOUND, neither in qemu-kvm nor upstream.

This is kernel commit, but it is too old. I am pretty sure userspace irq chip worked back then.

--
Gleb.

Avi Kivity

2011-02-02 13:14 UTC #8 lore raw

Quoted 9 lines · Gleb Natapov

Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
-ECOMMITNOTFOUND, neither in qemu-kvm nor upstream.
This is kernel commit, but it is too old. I am pretty sure userspace irq chip worked back then.

I have memories of it failing autotest, but the conclusion was that the commit did not cause the problem, simply enlarged the window in which it could happen.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2011-02-02 13:19 UTC #9 lore raw

Quoted 14 lines · Avi Kivity

Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Jan Kiszka
Quoted 10 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
-ECOMMITNOTFOUND, neither in qemu-kvm nor upstream.
This is kernel commit, but it is too old. I am pretty sure userspace irq chip worked back then.
I have memories of it failing autotest, but the conclusion was that the commit did not cause the problem, simply enlarged the window in which it could happen.

Ah, I remember something like that. RHEL6 kernel + qemu-kvm fails in the same way so problem is not new.

--
Gleb.

Jan Kiszka

author

2011-02-02 14:30 UTC #10 lore raw

In reply to Avi Kivity

Quoted 9 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?

Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-02 14:35 UTC #11 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).

Quoted 6 lines

Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?

The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.

If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-02-02 14:43 UTC #12 lore raw

Quoted 16 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?

Quoted 6 lines

The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).

You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2011-02-02 14:52 UTC #13 lore raw

Quoted 20 lines · Jan Kiszka

Quoted 19 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.

Quoted 7 lines

Quoted 3 lines
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?

Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.

Run apic_set_irq on the vcpu?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-02 15:09 UTC #14 lore raw

Quoted 25 lines · Jan Kiszka

Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).

Quoted 7 lines

Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.

And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.

Quoted 1 line

Run apic_set_irq on the vcpu?

static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)

{
    apic_irq_delivered += !get_bit(s->irr, vector_num);

    trace_apic_set_irq(apic_irq_delivered);

    set_bit(s->irr, vector_num);

This is even more async with kernel irqchip

    if (trigger_mode)
        set_bit(s->tmr, vector_num);
    else
        reset_bit(s->tmr, vector_num);

This is protected by qemu_mutex

    apic_update_irq(s);

This will be run the next time the vcpu exits, via apic_get_interrupt().

}

Did you check whether reverting that commit helps?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-02-02 15:35 UTC #15 lore raw

Quoted 54 lines · Avi Kivity

Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);

Quoted 5 lines

This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().

The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.

Quoted 5 lines

}
Did you check whether reverting that commit helps?

Just did so, and I can no longer reproduce the problem. Hmm...

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-02 15:44 UTC #16 lore raw

Quoted 23 lines · Jan Kiszka

Quoted 23 lines
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
 {
      apic_irq_delivered += !get_bit(s->irr, vector_num);

      trace_apic_set_irq(apic_irq_delivered);

      set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
      if (trigger_mode)
          set_bit(s->tmr, vector_num);
      else
          reset_bit(s->tmr, vector_num);
This is protected by qemu_mutex

Quoted 7 lines

Quoted 3 lines
      apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.

Right. My current understanding is that it is correct. But I distinctly remember that I came to a different conclusion when the failure first occurred (and the conclusion was that the patch did not cause the problem, just made it much more likely to see a not-up-to-date TPR).

Quoted 7 lines

Quoted 5 lines
}
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...

At least we have a pointer.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2011-02-02 15:46 UTC #17 lore raw

In reply to Jan Kiszka

Quoted 66 lines · Jan Kiszka

Quoted 54 lines · Avi Kivity
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);
Quoted 5 lines
This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.
Quoted 1 line

Quoted 7 lines

Quoted 4 lines
}
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...

If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?

--
Gleb.

Jan Kiszka

author

2011-02-02 15:52 UTC #18 lore raw

Quoted 69 lines · Gleb Natapov

Quoted 68 lines · Jan Kiszka
Quoted 54 lines · Avi Kivity
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);
Quoted 5 lines
This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.
Quoted 3 lines
}

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?

I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-02 16:29 UTC #19 lore raw

Quoted 79 lines · Jan Kiszka

Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Jan Kiszka
Quoted 54 lines · Avi Kivity
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);
Quoted 5 lines
This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.
Quoted 3 lines
}
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?

Quoted 7 lines

I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?

Injection happens by vcpu thread on cpu entry:
run->request_interrupt_window = kvm_arch_try_push_interrupts(env); and tpr is synced on vcpu exit, so I do not yet see how what you describe above may happen since during injection vcpu should see correct tpr.

--
Gleb.

Jan Kiszka

author

2011-02-02 16:37 UTC #20 lore raw

Quoted 87 lines · Gleb Natapov

Quoted 79 lines · Jan Kiszka
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Jan Kiszka
Quoted 54 lines · Avi Kivity
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Quoted 1 line
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);
Quoted 5 lines
This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.
Quoted 3 lines
}
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
Quoted 7 lines
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?

Quoted 4 lines

Injection happens by vcpu thread on cpu entry:
run->request_interrupt_window = kvm_arch_try_push_interrupts(env); and tpr is synced on vcpu exit, so I do not yet see how what you describe above may happen since during injection vcpu should see correct tpr.

Hmm, maybe this is the key: Once we call into apic_get_interrupt (because CPU_INTERRUPT_HARD was set as described above) and we find a pending irq below the tpr, we inject a spurious vector instead.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-02 16:39 UTC #21 lore raw

Quoted 88 lines · Jan Kiszka

Quoted 87 lines · Gleb Natapov
Quoted 86 lines · Jan Kiszka
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Jan Kiszka
Quoted 54 lines · Avi Kivity
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Quoted 2 lines
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);
Quoted 5 lines
This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.
Quoted 3 lines
}
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?

Quoted 9 lines

Quoted 4 lines
Injection happens by vcpu thread on cpu entry:
run->request_interrupt_window = kvm_arch_try_push_interrupts(env); and tpr is synced on vcpu exit, so I do not yet see how what you describe above may happen since during injection vcpu should see correct tpr.
Hmm, maybe this is the key: Once we call into apic_get_interrupt (because CPU_INTERRUPT_HARD was set as described above) and we find a pending irq below the tpr, we inject a spurious vector instead.

That should be easy to verify. I expect Windows to BSOD upon receiving spurious vector though.

--
Gleb.

Jan Kiszka

author

2011-02-02 16:51 UTC #22 lore raw

Quoted 94 lines · Gleb Natapov

Quoted 93 lines · Jan Kiszka
Quoted 91 lines · Gleb Natapov
Quoted 79 lines · Jan Kiszka
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Jan Kiszka
Quoted 54 lines · Avi Kivity
Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
Opps, -smp 1. With -smp 2 it boot almost completely and then hangs.
Ah, good (or not good). With Windows 2003 Server, I actually get a Blue
Screen (Stop 0x000000b8).
Quoted 6 lines
Quoted 2 lines
Userspace APIC is broken since it may run with an outdated cr8, does reverting 27a4f7976d5 help?
Can you elaborate on what is broken? The way hw/apic.c maintains the tpr? Would it make sense to compare this against the in-kernel model? Or do you mean something else?
The problem, IIRC, was that we look up the TPR but it may already have been changed by the running vcpu. Not 100% sure.
If that is indeed the problem then the fix would be to process the APIC in vcpu context (which is what the kernel does - we set a bit in the IRR and all further processing is synchronous).
Quoted 7 lines
Quoted 3 lines
You mean: user space changes the tpr value while the vcpu is in KVM_RUN, then we return from the kernel and overwrite the tpr in the apic with the vcpu's view, right?
Hmm, probably rather that there is a discrepancy between tpr and irr. The latter is changed asynchronously /wrt to the vcpu, the former /wrt the user space device model.
And yet, both are synchronized via qemu_mutex. So we're still missing something in this picture.
Quoted 1 line
Run apic_set_irq on the vcpu?
static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
{
     apic_irq_delivered += !get_bit(s->irr, vector_num);

     trace_apic_set_irq(apic_irq_delivered);

     set_bit(s->irr, vector_num);
This is even more async with kernel irqchip
     if (trigger_mode)
         set_bit(s->tmr, vector_num);
     else
         reset_bit(s->tmr, vector_num);
Quoted 5 lines
This is protected by qemu_mutex
     apic_update_irq(s);
This will be run the next time the vcpu exits, via apic_get_interrupt().
The decision to pend an IRQ (and potentially kick the vcpu) takes place immediately in acip_update_irq. And it is based on current irr as well as tpr. But we update again when user space returns with a new value.
Quoted 3 lines
}
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Did you check whether reverting that commit helps?
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
Quoted 7 lines
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
Injection happens by vcpu thread on cpu entry:
run->request_interrupt_window = kvm_arch_try_push_interrupts(env); and tpr is synced on vcpu exit, so I do not yet see how what you describe above may happen since during injection vcpu should see correct tpr.

Quoted 6 lines

Quoted 4 lines
Hmm, maybe this is the key: Once we call into apic_get_interrupt (because CPU_INTERRUPT_HARD was set as described above) and we find a pending irq below the tpr, we inject a spurious vector instead.
That should be easy to verify. I expect Windows to BSOD upon receiving spurious vector though.

I hacked spurious irq injection away, but the issue remains. At the same time, Windows is receiving tons of spurious interrupts without any complaints, even without that tpr optimization in the kernel. So this is obviously not yet the key.

Let's try your idea that we miss a wakeup.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-03 07:42 UTC #23 lore raw

Quoted 25 lines · Jan Kiszka

Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
Quoted 7 lines
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
Injection happens by vcpu thread on cpu entry:
run->request_interrupt_window = kvm_arch_try_push_interrupts(env); and tpr is synced on vcpu exit, so I do not yet see how what you describe above may happen since during injection vcpu should see correct tpr.
Quoted 6 lines
Quoted 4 lines
Hmm, maybe this is the key: Once we call into apic_get_interrupt (because CPU_INTERRUPT_HARD was set as described above) and we find a pending irq below the tpr, we inject a spurious vector instead.
That should be easy to verify. I expect Windows to BSOD upon receiving spurious vector though.

Quoted 7 lines

I hacked spurious irq injection away, but the issue remains. At the same time, Windows is receiving tons of spurious interrupts without any complaints, even without that tpr optimization in the kernel. So this is obviously not yet the key.
Let's try your idea that we miss a wakeup.

That is unlikely too. If vcpu missed wakeup, "info cpus" would solve the hang since it would kick vcpu out of the kernel and missed interrupt would be injected on re-entry.

--
Gleb.

Jan Kiszka

author

2011-02-03 09:31 UTC #24 lore raw

Quoted 31 lines · Gleb Natapov

Quoted 30 lines · Jan Kiszka
Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
Just did so, and I can no longer reproduce the problem. Hmm...
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
Quoted 7 lines
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
Injection happens by vcpu thread on cpu entry:
run->request_interrupt_window = kvm_arch_try_push_interrupts(env); and tpr is synced on vcpu exit, so I do not yet see how what you describe above may happen since during injection vcpu should see correct tpr.
Quoted 6 lines
Quoted 4 lines
Hmm, maybe this is the key: Once we call into apic_get_interrupt (because CPU_INTERRUPT_HARD was set as described above) and we find a pending irq below the tpr, we inject a spurious vector instead.
That should be easy to verify. I expect Windows to BSOD upon receiving spurious vector though.
I hacked spurious irq injection away, but the issue remains. At the same time, Windows is receiving tons of spurious interrupts without any complaints, even without that tpr optimization in the kernel. So this is obviously not yet the key.

Quoted 5 lines

Quoted 2 lines
Let's try your idea that we miss a wakeup.
That is unlikely too. If vcpu missed wakeup, "info cpus" would solve the hang since it would kick vcpu out of the kernel and missed interrupt would be injected on re-entry.

Yeah, and it wouldn't explain the various BSOFs I'm seeing (you get an even broader spectrum when trying the Windows installations DVDs).

We are probably digging at the wrong site.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-03 08:19 UTC #25 lore raw

In reply to Jan Kiszka

Quoted 11 lines · Jan Kiszka

Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?

apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.

The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-02-03 09:32 UTC #26 lore raw

Quoted 13 lines · Avi Kivity

Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?

Quoted 5 lines

apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?

I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-03 10:01 UTC #27 lore raw

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.

Quoted 6 lines

Quoted 2 lines
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?
I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.

I don't see why there should be any spurious interrupts in normal operation. From the docs, these happen due to an INTA cycle racing with raising the TPR, but in ioapic mode, there shouldn't be any INTA cycles.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-02-03 10:14 UTC #28 lore raw

Quoted 21 lines · Avi Kivity

Quoted 20 lines · Jan Kiszka
Quoted 18 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?

Quoted 8 lines

Quoted 3 lines
I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.
I don't see why there should be any spurious interrupts in normal operation. From the docs, these happen due to an INTA cycle racing with raising the TPR, but in ioapic mode, there shouldn't be any INTA cycles.

I added an instrumentation to the line of apic_get_interrupt that returns the spurious vector, and it triggered fairly often. Just didn't examined why this happens even without the tpr optimization.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Marcelo Tosatti

2011-02-03 10:05 UTC #29 lore raw

In reply to Jan Kiszka

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.

Quoted 6 lines

Quoted 2 lines
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?
I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.

http://www.mail-archive.com/[email protected]/msg41681.html

tpr of a vcpu should always be inspected in vcpu context, instead of iothread context?

Jan Kiszka

author

2011-02-03 10:11 UTC #30 lore raw

Quoted 18 lines · Marcelo Tosatti

Quoted 17 lines · Jan Kiszka
Quoted 16 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?
I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.
http://www.mail-archive.com/[email protected]/msg41681.html

Don't get the scenario yet: We do not inject (or set isr) over the context of apic_set_irq caller.

Quoted 3 lines

tpr of a vcpu should always be inspected in vcpu context, instead of iothread context?

Maybe this is true for the in-kernel model, but I don't see the issue (anymore) for the way user space works.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gleb Natapov

2011-02-03 14:15 UTC #31 lore raw

Quoted 32 lines · Jan Kiszka

Quoted 18 lines · Marcelo Tosatti
Quoted 17 lines · Jan Kiszka
Quoted 16 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?
I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.
http://www.mail-archive.com/[email protected]/msg41681.html
Don't get the scenario yet: We do not inject (or set isr) over the context of apic_set_irq caller.
Quoted 1 line

Quoted 6 lines

Quoted 2 lines
tpr of a vcpu should always be inspected in vcpu context, instead of iothread context?
Maybe this is true for the in-kernel model, but I don't see the issue (anymore) for the way user space works.

With patch below I can boot Windows7.

hw/apic.c +1 -1

index 146deca..fdcac88 100644
@@ -600,7 +600,7 @@ int apic_get_interrupt(DeviceState *d)
     intno = get_highest_priority_int(s->irr);
     if (intno < 0)
         return -1;
-    if (s->tpr && intno <= s->tpr)
+    if ((s->tpr >> 4) && (intno >> 4) <= (s->tpr >> 4))
         return s->spurious_vec & 0xff;
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);

--
Gleb.

Jan Kiszka

author

2011-02-03 14:28 UTC #32 lore raw

Quoted 55 lines · Gleb Natapov

Quoted 32 lines · Jan Kiszka
Quoted 18 lines · Marcelo Tosatti
Quoted 17 lines · Jan Kiszka
Quoted 16 lines · Avi Kivity
Quoted 11 lines · Jan Kiszka
Quoted 4 lines
Quoted 1 line
If there is no problem in the logic of this commit (and I do not see one yet) then we somewhere miss kicking vcpu when interrupt, that should be handled, arrives?
I'm not yet confident about the logic of the kernel patch: mov to cr8 is serializing. If the guest raises the tpr and then signals this with a succeeding, non vm-exiting instruction to the other vcpus, one of those could inject an interrupt with a higher priority than the previous tpr, but a lower one than current tpr. QEMU user space would accept this interrupt - and would likely surprise the guest. Do I miss something?
apic_get_interrupt() is only called from the vcpu thread, so it should see a correct tpr.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
The only difference I can see with the patch is that we may issue a spurious cpu_interrupt(). But that shouldn't do anything bad, should it?
I tested this yesterday, and it doesn't confuse Windows. It actually receives quite a few spurious IRQs in normal operation, w/ or w/o the kernel's tpr optimization.
http://www.mail-archive.com/[email protected]/msg41681.html
Don't get the scenario yet: We do not inject (or set isr) over the context of apic_set_irq caller.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
tpr of a vcpu should always be inspected in vcpu context, instead of iothread context?
Maybe this is true for the in-kernel model, but I don't see the issue (anymore) for the way user space works.
With patch below I can boot Windows7.
hw/apic.c +1 -2
index 146deca..fdcac88 100644
@@ -600,7 +600,7 @@ int apic_get_interrupt(DeviceState *d)
     intno = get_highest_priority_int(s->irr);
     if (intno < 0)
         return -1;
-    if (s->tpr && intno <= s->tpr)
+    if ((s->tpr >> 4) && (intno >> 4) <= (s->tpr >> 4))
         return s->spurious_vec & 0xff;
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
--

Quoted 1 line

Gleb.

Cool, /me too. I would just suggest

hw/apic.c +4 -1

index 05a115f..13bd7b4 100644
@@ -582,6 +582,7 @@ int apic_get_interrupt(DeviceState *d)
 {
     APICState *s = DO_UPCAST(APICState, busdev.qdev, d);
     int intno;
+    int tpr;
 
     /* if the APIC is installed or enabled, we let the 8259 handle the
        IRQs */

@@ -594,8 +595,10 @@ int apic_get_interrupt(DeviceState *d)
     intno = get_highest_priority_int(s->irr);
     if (intno < 0)
         return -1;
-    if (s->tpr && intno <= s->tpr)
+    tpr = s->tpr >> 4;
+    if (tpr && (intno >> 4) <= tpr) {
         return s->spurious_vec & 0xff;
+    }
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
     apic_update_irq(s);

Unfortunately, that issue was not related to the emulation mode problems of QEMU.

Thanks!
Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Avi Kivity

2011-02-06 10:26 UTC #33 lore raw

In reply to Gleb Natapov

Quoted 11 lines · Gleb Natapov

Quoted 4 lines
Maybe this is true for the in-kernel model, but I don't see the issue (anymore) for the way user space works.
With patch below I can boot Windows7.
hw/apic.c +0 -0
index 146deca..fdcac88 100644

hw/apic.c int apic_get_interrupt(DeviceState *d) · line 606

Quoted 8 lines

hw/apic.c +1 -1

@@ -600,7 +600,7 @@ int apic_get_interrupt(DeviceState *d)
      intno = get_highest_priority_int(s->irr);
      if (intno<  0)
          return -1;
-    if (s->tpr&&  intno<= s->tpr)
+    if ((s->tpr>>  4)&&  (intno>>  4)<= (s->tpr>>  4))
          return s->spurious_vec&  0xff;
      reset_bit(s->irr, intno);
      set_bit(s->isr, intno);

That still allows interrupts that have higher priority than the TPR, but lower priority than interrupts in the ISR to be injected. I think we need to use the PPR here (same as apic_update_irq()).

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2011-02-06 10:28 UTC #34 lore raw

Quoted 16 lines · Avi Kivity

Quoted 15 lines · Gleb Natapov
Quoted 4 lines
Maybe this is true for the in-kernel model, but I don't see the issue (anymore) for the way user space works.
With patch below I can boot Windows7.
hw/apic.c +0 -1
index 146deca..fdcac88 100644
@@ -600,7 +600,7 @@ int apic_get_interrupt(DeviceState *d)
     intno = get_highest_priority_int(s->irr);
     if (intno<  0)
         return -1;
-    if (s->tpr&&  intno<= s->tpr)

Quoted 9 lines

hw/apic.c int apic_get_interrupt(DeviceState *d) · line 606
Quoted 4 lines
hw/apic.c +1 -0
+    if ((s->tpr>>  4)&&  (intno>>  4)<= (s->tpr>>  4))
         return s->spurious_vec&  0xff;
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
That still allows interrupts that have higher priority than the TPR, but lower priority than interrupts in the ISR to be injected. I think we need to use the PPR here (same as apic_update_irq()).

We shouldn't get here if isr is non-empty, but see the patch I posted today to qemu-devel. It does what you say anyway.

--
Gleb.