>>> On Tue, May 8, 2007 at 4:13 AM, in message <[EMAIL PROTECTED]>, Avi Kivity <[EMAIL PROTECTED]> wrote: > Gregory Haskins wrote: >> >> I am perhaps being a bit overzealous here. What I found in practice is that > the LVTT can screw things up on shutdown, so I was being pretty conservative > on the synchronization here. >> >> > > That may point out to a different sync problem. All pending timers > ought to have been canceled before we reach here. Please check to make > sure this isn't papering over another problem. >
You are definitely right there. I had added this logic in the early stage of debugging. It turned out that I was missing an apic_dropref, which effectively meant the hrtimer_cancel() was never being issued. That was the root-cause of my "LVTT expiration after guest shutdown" bug. I left the sync code in as a conservative measure, but I will clean this up. >> >> > > I approach it from the other direction: to me, a locked assignment says > that something is fundamentally wrong. Usually anything under a lock is > a read- modify- write operation, otherwise the writes just stomp on each > other. > Interesting. That makes sense. So if I replace the assignment cases with wmb, do I need to sprinkle rmbs anywhere or is that take care of naturally by the places where we take the lock for a compound operation? > > > [preemption is disabled here anyway] > Ack. I will remove the calls >>>> + smp_call_function_single(direct_ipi, >>>> + kvm_vcpu_guest_intr, >>>> + vcpu, 0, 0); >>>> + preempt_enable(); >>>> + } >>>> >>>> >>> I see why you must issue the IPI outside the spin_lock_irqsave(), but >>> aren't you now opening a race? vcpu enters guest mode, irq on other >>> cpu, irq sets direct_ipi to wakeup guest, releases lock, vcpu exits to >>> userspace (or migrates to another cpu), ipi is issued but nobody cares. >>> >> >> Its subtle, but I think its ok. The race is actually against the setting of > the irq.pending. This *must* happen inside the lock or the guest could exit > and miss the interrupt. Once the pending bit is set, however, the guest can > be woken up in any old fashion and the behavior should be correct. If the > guest has already exited before the IPI is issued, its effectively a no- op > (well, really its just a wasted IPI/reschedule event, but no harm is done). > Does this make sense? Did I miss something else? >> > > No, you are correct wrt the vcpu migrating to another cpu. > > What about vs. exit to userspace where we may sleep? My logic being correct is predicated on the assumption that you and I made a week or two ago: That the user-space will not sleep for anything but HLT. If userspace *can* sleep on other things besides HLT, I agree that there is a race here. If it is limited to HLT, we will be taken care of by the virtue of the fact that irq.pending be set before the handle_halt() logic is checked. I admit that I was coding against an assumption that I do not yet know for a fact to be true. I will update the comments to note this assumption so its clearer, and we can address it in the future if its ever revealed to be false. ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ _______________________________________________ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
