On 2011-02-03 15:15, Gleb Natapov wrote: > On Thu, Feb 03, 2011 at 11:11:23AM +0100, Jan Kiszka wrote: >> On 2011-02-03 11:04, Marcelo Tosatti wrote: >>> On Thu, Feb 03, 2011 at 10:32:25AM +0100, Jan Kiszka wrote: >>>> On 2011-02-03 09:18, Avi Kivity wrote: >>>>> On 02/02/2011 05:52 PM, Jan Kiszka wrote: >>>>>>>> >>>>>>> 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? >>>> >>>> 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/kvm@xxxxxxxxxxxxxxx/msg41681.html >> >> Don't get the scenario yet: We do not inject (or set isr) over the >> context of apic_set_irq caller. >> >>> >>> 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. > > diff --git a/hw/apic.c b/hw/apic.c > index 146deca..fdcac88 100644 > --- a/hw/apic.c > +++ b/hw/apic.c > @@ -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. Cool, /me too. I would just suggest diff --git a/hw/apic.c b/hw/apic.c index 05a115f..13bd7b4 100644 --- a/hw/apic.c +++ b/hw/apic.c @@ -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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
