Re: switching from KVM guest to the host .... TLBs not present?

Re: switching from KVM guest to the host .... TLBs not present?

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

Zhang, Xiantao wrote:
> Yeah, that is very strange! I will check it tomorrow, and have to sleep now :-)
> Xiantao

Hi Xiantao,

I think I got it!

We were calling local_irqs_disable() before calling down_read(), but
down_read() can sleep and so may return with local interrupts
re-enabled.....

I have reordered things so now we should be safe.

How does this patch look to you?

Cheers,
Jes


[-- Attachment #2: 0001-kvm-ia64-vti-local-irq-disable.patch --]
[-- Type: text/x-patch, Size: 1237 bytes --]

Reorder locking as down_read() may return with local interrupts enabled,
which means we could go into vti_vcpu_run() with interrupts enabled.

This caused random crashes on the Altix as the timer interrupt tried
to read a memory mapped clock source, for which the TLB had not yet been
reinstated in the exit, before ipsr was retored.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 arch/ia64/kvm/kvm-ia64.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Index: linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
===================================================================
--- linux-2.6.git.orig/arch/ia64/kvm/kvm-ia64.c
+++ linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
@@ -610,20 +610,22 @@
 	int r;
 
 again:
-	preempt_disable();
-	local_irq_disable();
-
 	if (signal_pending(current)) {
-		local_irq_enable();
-		preempt_enable();
 		r = -EINTR;
 		kvm_run->exit_reason = KVM_EXIT_INTR;
 		goto out;
 	}
 
+	/*
+	 * down_read() may sleep and return with interrupts enabled
+	 */
+	down_read(&vcpu->kvm->slots_lock);
+
+	preempt_disable();
+	local_irq_disable();
+
 	vcpu->guest_mode = 1;
 	kvm_guest_enter();
-	down_read(&vcpu->kvm->slots_lock);
 	r = vti_vcpu_run(vcpu, kvm_run);
 	if (r < 0) {
 		local_irq_enable();

RE: switching from KVM guest to the host .... TLBs not present?

[Zhang, Xiantao]

Jes Sorensen wrote:
> Zhang, Xiantao wrote:
>> Yeah, that is very strange! I will check it tomorrow, and have to
>> sleep now :-) Xiantao
> 
> Hi Xiantao,
> 
> I think I got it!
> 
> We were calling local_irqs_disable() before calling down_read(), but
> down_read() can sleep and so may return with local interrupts
> re-enabled.....
> 
> I have reordered things so now we should be safe.
> 
> How does this patch look to you?

Good catch! That should be root-cause. The patch seems good! Thanks!
Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>

Avi, 
   Please queue it as the fix of 2.6.30. Thanks!
Xiantao

Re: switching from KVM guest to the host .... TLBs not present?

[Avi Kivity]

Zhang, Xiantao wrote:
> Jes Sorensen wrote:
>   
>> Zhang, Xiantao wrote:
>>     
>>> Yeah, that is very strange! I will check it tomorrow, and have to
>>> sleep now :-) Xiantao
>>>       
>> Hi Xiantao,
>>
>> I think I got it!
>>
>> We were calling local_irqs_disable() before calling down_read(), but
>> down_read() can sleep and so may return with local interrupts
>> re-enabled.....
>>
>> I have reordered things so now we should be safe.
>>
>> How does this patch look to you?
>>     
>
> Good catch! That should be root-cause. The patch seems good! Thanks!
> Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>
>
> Avi, 
>    Please queue it as the fix of 2.6.30. Thanks!
>   

Applied, but I note that entering the guest with any lock held is 
problematic, as the guest may spend an arbitrary amount of time in guest 
mode.  Really, entering the guest is almost exactly like exiting to 
userspace.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: switching from KVM guest to the host .... TLBs not present?

[Jes Sorensen]

Avi Kivity wrote:
> Applied, but I note that entering the guest with any lock held is 
> problematic, as the guest may spend an arbitrary amount of time in guest 
> mode.  Really, entering the guest is almost exactly like exiting to 
> userspace.

Thanks Avi,

I did wonder about this as I noticed x86_64 doesn't hold the lock while
entering a guest. I'll have a look - seems like it could be the reason
why I get hit by lockdep timeouts when running larger guests on ia64.

Cheers,
Jes

Re: switching from KVM guest to the host .... TLBs not present?

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

Avi Kivity wrote:
> Applied, but I note that entering the guest with any lock held is 
> problematic, as the guest may spend an arbitrary amount of time in guest 
> mode.  Really, entering the guest is almost exactly like exiting to 
> userspace.

Hi Avi,

I had a look at this and reworked the locking some, so we don't hold
the slots_lock when entering the guest.

How does this look? Xiantao any thoughts of whether it's unsafe to call
kvm_vcpu_post_transition without holding that semaphore? I believe it
should be fine.

I am still seeing issues where the host can get a lock timeout when
running large guests, but the situation seems to be better with this
patch applied.

Cheers,
Jes




[-- Attachment #2: 0002-kvm-ia64-reorder-run-locking.patch --]
[-- Type: text/x-patch, Size: 2858 bytes --]

Reorder locking to avoid holding the slots_lock when entering
the guest.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 arch/ia64/kvm/kvm-ia64.c |   64 ++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

Index: linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
===================================================================
--- linux-2.6.git.orig/arch/ia64/kvm/kvm-ia64.c
+++ linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
@@ -580,34 +580,22 @@
 	vti_set_rr6(vcpu->arch.vmm_rr);
 	return kvm_insert_vmm_mapping(vcpu);
 }
+
 static void kvm_vcpu_post_transition(struct kvm_vcpu *vcpu)
 {
 	kvm_purge_vmm_mapping(vcpu);
 	vti_set_rr6(vcpu->arch.host_rr6);
 }
 
-static int  vti_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+static int __vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	union context *host_ctx, *guest_ctx;
 	int r;
 
-	/*Get host and guest context with guest address space.*/
-	host_ctx = kvm_get_host_context(vcpu);
-	guest_ctx = kvm_get_guest_context(vcpu);
-
-	r = kvm_vcpu_pre_transition(vcpu);
-	if (r < 0)
-		goto out;
-	kvm_vmm_info->tramp_entry(host_ctx, guest_ctx);
-	kvm_vcpu_post_transition(vcpu);
-	r = 0;
-out:
-	return r;
-}
-
-static int __vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
-{
-	int r;
+	/*
+	 * down_read() may sleep and return with interrupts enabled
+	 */
+	down_read(&vcpu->kvm->slots_lock);
 
 again:
 	if (signal_pending(current)) {
@@ -616,23 +604,28 @@
 		goto out;
 	}
 
-	/*
-	 * down_read() may sleep and return with interrupts enabled
-	 */
-	down_read(&vcpu->kvm->slots_lock);
-
 	preempt_disable();
 	local_irq_disable();
 
+	/*Get host and guest context with guest address space.*/
+	host_ctx = kvm_get_host_context(vcpu);
+	guest_ctx = kvm_get_guest_context(vcpu);
+
 	vcpu->guest_mode = 1;
+
+	r = kvm_vcpu_pre_transition(vcpu);
+	if (r < 0)
+		goto vcpu_run_fail;
+
+	up_read(&vcpu->kvm->slots_lock);
 	kvm_guest_enter();
-	r = vti_vcpu_run(vcpu, kvm_run);
-	if (r < 0) {
-		local_irq_enable();
-		preempt_enable();
-		kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
-		goto out;
-	}
+
+	/*
+	 * Transition to the guest
+	 */
+	kvm_vmm_info->tramp_entry(host_ctx, guest_ctx);
+
+	kvm_vcpu_post_transition(vcpu);
 
 	vcpu->arch.launched = 1;
 	vcpu->guest_mode = 0;
@@ -646,9 +639,10 @@
 	 */
 	barrier();
 	kvm_guest_exit();
-	up_read(&vcpu->kvm->slots_lock);
 	preempt_enable();
 
+	down_read(&vcpu->kvm->slots_lock);
+
 	r = kvm_handle_exit(kvm_run, vcpu);
 
 	if (r > 0) {
@@ -657,12 +651,20 @@
 	}
 
 out:
+	up_read(&vcpu->kvm->slots_lock);
 	if (r > 0) {
 		kvm_resched(vcpu);
+		down_read(&vcpu->kvm->slots_lock);
 		goto again;
 	}
 
 	return r;
+
+vcpu_run_fail:
+	local_irq_enable();
+	preempt_enable();
+	kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+	goto out;
 }
 
 static void kvm_set_mmio_data(struct kvm_vcpu *vcpu)

Re: switching from KVM guest to the host .... TLBs not present?

[Avi Kivity]

Jes Sorensen wrote:
> Avi Kivity wrote:
>> Applied, but I note that entering the guest with any lock held is 
>> problematic, as the guest may spend an arbitrary amount of time in 
>> guest mode.  Really, entering the guest is almost exactly like 
>> exiting to userspace.
>
> Hi Avi,
>
> I had a look at this and reworked the locking some, so we don't hold
> the slots_lock when entering the guest.
>
> How does this look? Xiantao any thoughts of whether it's unsafe to call
> kvm_vcpu_post_transition without holding that semaphore? I believe it
> should be fine.
>
> I am still seeing issues where the host can get a lock timeout when
> running large guests, but the situation seems to be better with this
> patch applied.
>

It looks fine, though I'm hardly qualified to review ia64 code.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

RE: switching from KVM guest to the host .... TLBs not present?

[Zhang, Xiantao]

Jes Sorensen wrote:
> Avi Kivity wrote:
>> Applied, but I note that entering the guest with any lock held is
>> problematic, as the guest may spend an arbitrary amount of time in
>> guest mode.  Really, entering the guest is almost exactly like
>> exiting to userspace.
> 
> Hi Avi,
> 
> I had a look at this and reworked the locking some, so we don't hold
> the slots_lock when entering the guest.
> 
> How does this look? Xiantao any thoughts of whether it's unsafe to
> call kvm_vcpu_post_transition without holding that semaphore? I
> believe it should be fine.
> 
> I am still seeing issues where the host can get a lock timeout when
> running large guests, but the situation seems to be better with this
> patch applied.

I remembered this lock should be used to avoid guest memory mappings' changes once vcpu in guest mode before.  Since ia64's memory virtualization locates in VMM, so have to take the lock in VMM, otherwise, guest may access memory with old mapping, but this machanism should be ineffective after optimizing guest's TLB flush in kvm_vcpu_pre_transition.  So this lock shouldn't be held noe. This patch looks fine! Thanks Jes!
  
Acked-by : Xiantao Zhang<xiantao.zhang@intel.com>

But anyway, we should implement kvm_arch_flush_shadow to prevent guest using old mapping to access memory, otherwise guest may have issues even probably crash host once guest memory mapping changes frequently. 

Xiantao 

Re: switching from KVM guest to the host .... TLBs not present?

[Avi Kivity]

Zhang, Xiantao wrote:
> Jes Sorensen wrote:
>   
>> Avi Kivity wrote:
>>     
>>> Applied, but I note that entering the guest with any lock held is
>>> problematic, as the guest may spend an arbitrary amount of time in
>>> guest mode.  Really, entering the guest is almost exactly like
>>> exiting to userspace.
>>>       
>> Hi Avi,
>>
>> I had a look at this and reworked the locking some, so we don't hold
>> the slots_lock when entering the guest.
>>
>> How does this look? Xiantao any thoughts of whether it's unsafe to
>> call kvm_vcpu_post_transition without holding that semaphore? I
>> believe it should be fine.
>>
>> I am still seeing issues where the host can get a lock timeout when
>> running large guests, but the situation seems to be better with this
>> patch applied.
>>     
>
> I remembered this lock should be used to avoid guest memory mappings' changes once vcpu in guest mode before.  Since ia64's memory virtualization locates in VMM, so have to take the lock in VMM, otherwise, guest may access memory with old mapping, but this machanism should be ineffective after optimizing guest's TLB flush in kvm_vcpu_pre_transition.  So this lock shouldn't be held noe. This patch looks fine! Thanks Jes!
>   
> Acked-by : Xiantao Zhang<xiantao.zhang@intel.com>
>
> But anyway, we should implement kvm_arch_flush_shadow to prevent guest using old mapping to access memory, otherwise guest may have issues even probably crash host once guest memory mapping changes frequently. 
>
>   

Shouldn't we implement kvm_arch_flush_shadow() before we apply this?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

RE: switching from KVM guest to the host .... TLBs not present?

[Zhang, Xiantao]

Avi Kivity wrote:
> Zhang, Xiantao wrote:
>> Jes Sorensen wrote:
>> 
>>> Avi Kivity wrote:
>>> 
>>>> Applied, but I note that entering the guest with any lock held is
>>>> problematic, as the guest may spend an arbitrary amount of time in
>>>> guest mode.  Really, entering the guest is almost exactly like
>>>> exiting to userspace. 
>>>> 
>>> Hi Avi,
>>> 
>>> I had a look at this and reworked the locking some, so we don't hold
>>> the slots_lock when entering the guest.
>>> 
>>> How does this look? Xiantao any thoughts of whether it's unsafe to
>>> call kvm_vcpu_post_transition without holding that semaphore? I
>>> believe it should be fine. 
>>> 
>>> I am still seeing issues where the host can get a lock timeout when
>>> running large guests, but the situation seems to be better with
>>> this patch applied. 
>>> 
>> 
>> I remembered this lock should be used to avoid guest memory
>> mappings' changes once vcpu in guest mode before.  Since ia64's
>> memory virtualization locates in VMM, so have to take the lock in
>> VMM, otherwise, guest may access memory with old mapping, but this
>> machanism should be ineffective after optimizing guest's TLB flush
>> in kvm_vcpu_pre_transition.  So this lock shouldn't be held noe.
>> This patch looks fine! Thanks Jes!      
>> 
>> Acked-by : Xiantao Zhang<xiantao.zhang@intel.com>
>> 
>> But anyway, we should implement kvm_arch_flush_shadow to prevent
>> guest using old mapping to access memory, otherwise guest may have
>> issues even probably crash host once guest memory mapping changes
>> frequently.   
>> 
>> 
> 
> Shouldn't we implement kvm_arch_flush_shadow() before we apply this?

It should be another issue.  Even holding the lock in guest mode, it still have issue.  Okay, anyway I will work out a patch to fix it. 
Xiantao