Re: [PATCH 4/9] KVM: Adds ability to preempt anexecutingVCPU

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Thu, May 10, 2007 at  4:22 AM, in message
<64F9B87B6B770947A9F8391472E032160BBA6616-yEcIvxbTEBqsx+V+t5oei8rau4O3wl8o3fe8/T/H7NteoWH0uzbU5w@public.gmane.org>,
"Dor Laor" <dor.laor-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
>>>> static void kvm_vcpu_irqsink_init(struct kvm_vcpu *vcpu)
>>>>diff --  git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
>>>>index 4c03881..91546ae 100644
>>>>---   a/drivers/kvm/svm.c
>>>>+++ b/drivers/kvm/svm.c
>>>>@@ -  1542,11 +1542,40 @@ static int svm_vcpu_run(struct kvm_vcpu
> *vcpu,
>>>>struct kvm_run *kvm_run)
>>>> 	u16 gs_selector;
>>>> 	u16 ldt_selector;
>>>> 	int r;
>>>>+	unsigned long irq_flags;
>>>>
>>>> again:
>>>>+	/*
>>>>+	 * We disable interrupts until the next VMEXIT to eliminate a
>>> race
>>>>+	 * condition for delivery of virtual interrutps.  Note that this
>>> is
>>>>+	 * probably not as bad as it sounds, as interrupts will still
>>> invoke
>>>>+	 * a VMEXIT once transitioned to GUEST mode (and thus exit this
>>> lock
>>>>+	 * scope) even if they are disabled.
>>>>+	 *
>>>>+	 * FIXME: Do we need to do anything additional to mask IPI/NMIs?
>>>>+	 */
>>>>+	local_irq_save(irq_flags);
>>>>+
>>>> 	spin_lock(&vcpu-  >irq.lock);
>>>>
>>>> 	/*
>>>>+	 * If there are any signals pending (virtual interrupt related
>>> or
>>>>+	 * otherwise), don't even bother trying to enter guest mode...
>>>>+	 */
>>>>+	if (signal_pending(current)) {
>>>>+		kvm_run-  >exit_reason = KVM_EXIT_INTR;
>>>>+		spin_unlock(&vcpu-  >irq.lock);
>>>>+		local_irq_restore(irq_flags);
>>>>+		return -  EINTR;
> 
> 
> Instead of returning you should 'goto out;' since the post_kvm_run_save
> is called there + minimize return paths.

Yeah, I agree.  I had already made this change for VMX in v2.  I have now fixed the SVN side for v3 which will follow this email

> 
> 
>>>>+	}
>>>
>>>
>>> A possible optimization would be to check if we have an irq to
> inject.
>>> If we have, then ignore the signal and enter guest mode.
>>> Since an irq is already pending, the signal would not be resulting in
>>> another irq injection.
>>> What do you think?
>>
>>I am a little fuzzy on whether this is necessary myself.  The
> motivation
>>for this code block originally was really more for the cases where
> signals
>>are sent that *aren't* related to interrupts.  For instance, what if
> QEMU
>>was trying to force the vCPU to exit for some reason (e.g. an AIO
>>completion event that has to be decoded by userspace before a IRQ is
>>generated)?  If the signal is queued beforehand AND linux doesn't
> already
>>reschedule things for us, I think there is a race against the VCPU
> being
>>stuck in guest mode until the next (unrelated) VMEXIT since the IPI
> would
>>have already happened.  This code (I believe) closes the window on that
>>scenario.  I am just not sure if its a realistic one.  Perhaps someone
> with
>>more linux signal handling knowledge than myself can chime in and
> confirm?
>>(And I am not saying that person isn't you, Dor.  I am only saying it
> isn't
>>me ;)
> 
> 
> You're right that there is a slight chance for a qemu signal such as AIO
> completion to reach right after we enter the kernel. My intention was to
> unit the signal pending checks -  there is another one after the guest
> exits vmx mode. Then once we're back on host mode we can optimize the
> path by checking whether we need to re- inject in- kernel- apic pending irq
> or a case where a physical irq prevented us from injecting the virq.
> 
> The optimization is not specific to apic and can take place even now.

I'm not entirely following you, but you if elaborate a little more perhaps I can get it incorporated.  Otherwise, we could just wait till this logic is merged and do a follow on patch.

> 
>>
>>>
>>>>+
>>>>+	/*
>>>>+	 * There are optimizations we can make when signaling interrupts
>>>>+	 * if we know the VCPU is in GUEST mode, so mark that here
>>>>+	 */
>>>>+	vcpu-  >irq.guest_mode = 1;
>>>>+
>>>>+	/*
>>>> 	 * We must inject interrupts (if any) while the irq_lock
>>>> 	 * is held
>>>> 	 */
>>>>@@ -  1688,6 +1717,13 @@ again:
>>>> #endif
>>>> 		: "cc", "memory" );
>>>>
>>>>+	/*
>>>>+	 * FIXME: We'd like to turn on interrupts ASAP, but is this so
>>> early
>>>>+	 * that we will mess up the state of the CPU before we fully
>>>>+	 * transition from guest to host?
>>>>+	 */
>>>
>>> The guest_mode can be assigned here, thus eliminating the cli-  sti
> below.
>>
>>Yeah....I was concerned about putting too much logic before the
> guest/host
>>state transfer because I didn't know enough about it to know if I would
>>stomp on some register that still hadn't been saved.  If you say its
> safe
>>to do that here, then I agree that is the optimal way to do it.
> 
> There is nothing to worry about since the ipi just caused the cpu to
> leave guest mode, the vcpu locks are still held and after getting the
> ipi (that you wrote and does nothing) the host will continue the same
> execution path.
> Note that today the code has irq enabled all the exit path too.

Fixed.



-------------------------------------------------------------------------
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/