RE: kvm/ia64: Fix halt emulation logic.

Zhang, Xiantao
2008-11-03T04:36:32Z

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

Marcelo Tosatti wrote:
> Hi Xiantao,
>
> On Wed, Oct 15, 2008 at 09:47:24PM +0800, Zhang, Xiantao wrote:
>> + expires = div64_u64(itc_diff, cyc_per_usec);
>> + kt = ktime_set(0, 1000 * expires);
>> +
>> + down_read(&vcpu->kvm->slots_lock);
>> + vcpu->arch.ht_active = 1;
>> + hrtimer_start(p_ht, kt, HRTIMER_MODE_ABS);
>>
>> - if (irqchip_in_kernel(vcpu->kvm)) {
>> vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
>> kvm_vcpu_block(vcpu);
>> hrtimer_cancel(p_ht);
>> vcpu->arch.ht_active = 0;
>>
>> + if (test_and_clear_bit(KVM_REQ_UNHALT, &vcpu->requests))
>> + if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED)
>> + vcpu->arch.mp_state = + KVM_MP_STATE_RUNNABLE;
>> + up_read(&vcpu->kvm->slots_lock);
>
> You should release slots_lock when blocking via kvm_vcpu_block().
> Otherwise paths that grab it for write will depend on these vcpus to
> unhalt.
>
> BTW, none of the data structures in this section of code should be
> protected by slots_lock?

Hi, Marcelo
Agree, I have made a patch to remove it last week, but seems Avi hasn't commited it yet.

Avi,
Could you help to commit the attached patch, even though it is just a potential issue(write lock is very rare in current code).
Thanks
Xiantao

[-- Attachment #2: Type: message/rfc822, Size: 7147 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 1284 bytes --]

>From 4858a5c47c5dce88a62a6edf427d8709f3ebda15 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Thu, 23 Oct 2008 15:03:38 +0800
Subject: [PATCH] KVM: ia64: kvm halt logic doesn't need lock to protect.

Remove the lock protection for kvm halt logic, otherwise,
once other vcpus want to acquire the lock, and they have to
wait all vcpus are waken up from halt.
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
arch/ia64/kvm/kvm-ia64.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 6b1e31b..93c7f18 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -439,7 +439,6 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
expires = div64_u64(itc_diff, cyc_per_usec);
kt = ktime_set(0, 1000 * expires);

- down_read(&vcpu->kvm->slots_lock);
vcpu->arch.ht_active = 1;
hrtimer_start(p_ht, kt, HRTIMER_MODE_ABS);

@@ -452,7 +451,6 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED)
vcpu->arch.mp_state =
KVM_MP_STATE_RUNNABLE;
- up_read(&vcpu->kvm->slots_lock);

if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE)
return -EINTR;
--
1.6.0

[-- Attachment #2.1.2: 0003-KVM-ia64-kvm-halt-logic-doesn-t-need-lock-to-prote.patch --]
[-- Type: application/octet-stream, Size: 1249 bytes --]

From 4858a5c47c5dce88a62a6edf427d8709f3ebda15 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Thu, 23 Oct 2008 15:03:38 +0800
Subject: [PATCH] KVM: ia64: kvm halt logic doesn't need lock to protect.

Remove the lock protection for kvm halt logic, otherwise,
once other vcpus want to acquire the lock, and they have to
wait all vcpus are waken up from halt.
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
arch/ia64/kvm/kvm-ia64.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 6b1e31b..93c7f18 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -439,7 +439,6 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
expires = div64_u64(itc_diff, cyc_per_usec);
kt = ktime_set(0, 1000 * expires);

- down_read(&vcpu->kvm->slots_lock);
vcpu->arch.ht_active = 1;
hrtimer_start(p_ht, kt, HRTIMER_MODE_ABS);

@@ -452,7 +451,6 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED)
vcpu->arch.mp_state =
KVM_MP_STATE_RUNNABLE;
- up_read(&vcpu->kvm->slots_lock);

if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE)
return -EINTR;
--
1.6.0