>>> On Thu, May 10, 2007 at 4:22 AM, in message
<[EMAIL PROTECTED]>,
"Dor Laor" <[EMAIL PROTECTED]> 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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
