Remove APIC lock

Dong, Eddie
2007-08-24T13:08:38Z

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

Avi:

apic->lock is used in many place to avoid race condition with apic
timer call back
function which may run on different pCPU. This patch migrate the
apic timer to
same CPU with the one VP runs on, thus the lock is no longer
necessary.

thx,eddie

Signed-off-by: Yaozu (Eddie) Dong <Eddie.Dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
index b6283d2..f324cfb 100644
--- a/drivers/kvm/irq.h
+++ b/drivers/kvm/irq.h
@@ -106,7 +106,6 @@ struct kvm_ioapic {
};

struct kvm_lapic {
- spinlock_t lock; /* TODO for revise */
unsigned long base_address;
struct kvm_io_device dev;
struct {
@@ -159,5 +158,6 @@ void kvm_apic_timer_intr_post(struct kvm_vcpu *vcpu,
int vec);
void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
+void kvm_migrate_apic_timer(struct kvm_vcpu *vcpu);

#endif
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 751ec03..d9385a5 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -176,9 +176,7 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu
*vcpu)

if (!apic)
return 0;
- spin_lock_bh(&apic->lock);
highest_irr = apic_find_highest_irr(apic);
- spin_unlock_bh(&apic->lock);

return highest_irr;
}
@@ -525,9 +523,7 @@ static void apic_mmio_read(struct kvm_io_device
*this,
(unsigned long)address, len);
return;
}
- spin_lock_bh(&apic->lock);
result = __apic_read(apic, offset & ~0xf);
- spin_unlock_bh(&apic->lock);

switch (len) {
case 1:
@@ -571,7 +567,6 @@ static void apic_mmio_write(struct kvm_io_device
*this,

offset &= 0xff0;

- spin_lock_bh(&apic->lock);
switch (offset) {
case APIC_ID: /* Local APIC ID */
apic_set_reg(apic, APIC_ID, val);
@@ -645,7 +640,6 @@ static void apic_mmio_write(struct kvm_io_device
*this,
APIC_BUS_CYCLE_NS * apic->timer.divide_count
* val;

/* Make sure the lock ordering is coherent */
- spin_unlock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
atomic_set(&apic->timer.pending, 0);
hrtimer_start(&apic->timer.dev,
@@ -687,7 +681,6 @@ static void apic_mmio_write(struct kvm_io_device
*this,
break;
}

- spin_unlock_bh(&apic->lock);
}

static int apic_mmio_range(struct kvm_io_device *this, gpa_t addr)
@@ -695,15 +688,12 @@ static int apic_mmio_range(struct kvm_io_device
*this, gpa_t addr)
struct kvm_lapic *apic = (struct kvm_lapic *)this->private;
int ret = 0;

- spin_lock_bh(&apic->lock);

if (apic_hw_enabled(apic) &&
(addr >= apic->base_address) &&
(addr < (apic->base_address + LAPIC_MMIO_LENGTH)))
ret = 1;

- spin_unlock_bh(&apic->lock);
-
return ret;
}

@@ -711,7 +701,6 @@ void kvm_free_apic(struct kvm_lapic *apic)
{
if (!apic)
return;
- spin_lock_bh(&apic->lock);

hrtimer_cancel(&apic->timer.dev);

@@ -720,8 +709,6 @@ void kvm_free_apic(struct kvm_lapic *apic)
apic->regs_page = 0;
}

- spin_unlock_bh(&apic->lock);
-
kfree(apic);
}

@@ -737,9 +724,7 @@ void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu,
unsigned long cr8)

if (!apic)
return;
- spin_lock_bh(&apic->lock);
apic_set_tpr(apic, ((cr8 & 0x0f) << 4));
- spin_unlock_bh(&apic->lock);
}

u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
@@ -749,9 +734,7 @@ u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)

if (!apic)
return 0;
- spin_lock_bh(&apic->lock);
tpr = (u64) apic_get_reg(apic, APIC_TASKPRI);
- spin_unlock_bh(&apic->lock);

return (tpr & 0xf0) >> 4;
}
@@ -766,7 +749,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64
value)
vcpu->apic_base = value;
return;
}
- spin_lock_bh(&apic->lock);
if (apic->vcpu->vcpu_id)
value &= ~MSR_IA32_APICBASE_BSP;

@@ -778,19 +760,11 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64
value)
apic_debug("apic base msr is 0x%016" PRIx64 ", and base address
is "
"0x%lx.\n", vcpu->apic_base, apic->base_address);

- spin_unlock_bh(&apic->lock);
}

u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu)
{
- struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
- u64 base;
-
- spin_lock_bh(&apic->lock);
- base = vcpu->apic_base;
- spin_unlock_bh(&apic->lock);
-
- return base;
+ return vcpu->apic_base;
}
EXPORT_SYMBOL_GPL(kvm_lapic_get_base);

@@ -808,8 +782,6 @@ static void lapic_reset(struct kvm_vcpu *vcpu)
/* Stop the timer in case it's a reset to an active apic */
hrtimer_cancel(&apic->timer.dev);

- spin_lock_bh(&apic->lock);
-
apic_set_reg(apic, APIC_ID, vcpu->vcpu_id << 24);
apic_set_reg(apic, APIC_LVR, APIC_VERSION);

@@ -836,8 +808,6 @@ static void lapic_reset(struct kvm_vcpu *vcpu)
vcpu->apic_base |= MSR_IA32_APICBASE_BSP;
apic_update_ppr(apic);

- spin_unlock_bh(&apic->lock);
-
apic_debug(KERN_INFO "%s: vcpu=%p, id=%d, base_msr="
"0x%016" PRIx64 ", base_address=0x%0lx.\n",
__FUNCTION__,
vcpu, GET_APIC_ID(apic_get_reg(apic, APIC_ID)),
@@ -851,24 +821,13 @@ int kvm_lapic_enabled(struct kvm_vcpu *vcpu)

if (!apic)
return 0;
- spin_lock_bh(&apic->lock);
ret = apic_enabled(apic);
- spin_unlock_bh(&apic->lock);

return ret;
}
EXPORT_SYMBOL_GPL(kvm_lapic_enabled);

/*
-void *kvm_lapic_get_regs(struct kvm_vcpu *vcpu)
-{
- struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
- return apic->regs;
-}
-EXPORT_SYMBOL_GPL(kvm_lapic_get_regs);
-*/
-
-/*

*----------------------------------------------------------------------
* timer interface

*----------------------------------------------------------------------
@@ -907,9 +866,7 @@ static enum hrtimer_restart apic_timer_fn(struct
hrtimer *data)

apic = container_of(data, struct kvm_lapic, timer.dev);

- spin_lock_bh(&apic->lock);
restart_timer = __apic_timer_fn(apic);
- spin_unlock_bh(&apic->lock);

if (restart_timer)
return HRTIMER_RESTART;
@@ -929,7 +886,6 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
goto nomem;

vcpu->apic = apic;
- spin_lock_init(&apic->lock);

apic->regs_page = alloc_page(GFP_KERNEL);
if (apic->regs_page == NULL) {
@@ -1024,7 +980,6 @@ void kvm_apic_post_state_restore(struct kvm_vcpu
*vcpu)
apic_update_ppr(apic);

/* TODO: following code can be in a common API */
- spin_lock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
atomic_set(&apic->timer.pending, 0);
val = apic_get_reg(apic, APIC_TDCR);
@@ -1037,5 +992,18 @@ void kvm_apic_post_state_restore(struct kvm_vcpu
*vcpu)
hrtimer_start(&apic->timer.dev,
ktime_add_ns(now, offset),
HRTIMER_MODE_ABS);
- spin_unlock_bh(&apic->lock);
}
+
+void kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
+{
+ struct kvm_lapic *apic = vcpu->apic;
+ struct hrtimer *timer;
+
+ if (apic) {
+ timer = &apic->timer.dev;
+ hrtimer_cancel(timer);
+ hrtimer_start(timer, timer->expires, HRTIMER_MODE_ABS);
+ }
+}
+EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer);
+
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index c64fe6d..c584626 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -633,6 +633,7 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int
cpu)
delta = vcpu->host_tsc - tsc_this;
svm->vmcb->control.tsc_offset += delta;
vcpu->cpu = cpu;
+ kvm_migrate_apic_timer(vcpu);
}

for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index b537104..5ea1132 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -441,8 +441,10 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu,
int cpu)
u64 phys_addr = __pa(vmx->vmcs);
u64 tsc_this, delta;

- if (vcpu->cpu != cpu)
+ if (vcpu->cpu != cpu) {
vcpu_clear(vmx);
+ kvm_migrate_apic_timer(vcpu);
+ }

if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
u8 error;

[-- Attachment #2: 4-lock.patch --]
[-- Type: application/octet-stream, Size: 7656 bytes --]

apic->lock is used in many place to avoid
race condition with apic timer call back
function which may run on different pCPU.
This patch migrate the apic timer to be
same CPU with the one VP runs on.

Signed-off-by: Yaozu (Eddie) Dong <Eddie.Dong@intel.com>

diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
index b6283d2..f324cfb 100644
--- a/drivers/kvm/irq.h
+++ b/drivers/kvm/irq.h
@@ -106,7 +106,6 @@ struct kvm_ioapic {
};

struct kvm_lapic {
- spinlock_t lock; /* TODO for revise */
unsigned long base_address;
struct kvm_io_device dev;
struct {
@@ -159,5 +158,6 @@ void kvm_apic_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
+void kvm_migrate_apic_timer(struct kvm_vcpu *vcpu);

#endif
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 751ec03..d9385a5 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -176,9 +176,7 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)

if (!apic)
return 0;
- spin_lock_bh(&apic->lock);
highest_irr = apic_find_highest_irr(apic);
- spin_unlock_bh(&apic->lock);

return highest_irr;
}
@@ -525,9 +523,7 @@ static void apic_mmio_read(struct kvm_io_device *this,
(unsigned long)address, len);
return;
}
- spin_lock_bh(&apic->lock);
result = __apic_read(apic, offset & ~0xf);
- spin_unlock_bh(&apic->lock);

switch (len) {
case 1:
@@ -571,7 +567,6 @@ static void apic_mmio_write(struct kvm_io_device *this,

offset &= 0xff0;

- spin_lock_bh(&apic->lock);
switch (offset) {
case APIC_ID: /* Local APIC ID */
apic_set_reg(apic, APIC_ID, val);
@@ -645,7 +640,6 @@ static void apic_mmio_write(struct kvm_io_device *this,
APIC_BUS_CYCLE_NS * apic->timer.divide_count * val;

/* Make sure the lock ordering is coherent */
- spin_unlock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
atomic_set(&apic->timer.pending, 0);
hrtimer_start(&apic->timer.dev,
@@ -687,7 +681,6 @@ static void apic_mmio_write(struct kvm_io_device *this,
break;
}

- spin_unlock_bh(&apic->lock);
}

static int apic_mmio_range(struct kvm_io_device *this, gpa_t addr)
@@ -695,15 +688,12 @@ static int apic_mmio_range(struct kvm_io_device *this, gpa_t addr)
struct kvm_lapic *apic = (struct kvm_lapic *)this->private;
int ret = 0;

- spin_lock_bh(&apic->lock);

if (apic_hw_enabled(apic) &&
(addr >= apic->base_address) &&
(addr < (apic->base_address + LAPIC_MMIO_LENGTH)))
ret = 1;

- spin_unlock_bh(&apic->lock);
-
return ret;
}

@@ -711,7 +701,6 @@ void kvm_free_apic(struct kvm_lapic *apic)
{
if (!apic)
return;
- spin_lock_bh(&apic->lock);

hrtimer_cancel(&apic->timer.dev);

@@ -720,8 +709,6 @@ void kvm_free_apic(struct kvm_lapic *apic)
apic->regs_page = 0;
}

- spin_unlock_bh(&apic->lock);
-
kfree(apic);
}

@@ -737,9 +724,7 @@ void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)

if (!apic)
return;
- spin_lock_bh(&apic->lock);
apic_set_tpr(apic, ((cr8 & 0x0f) << 4));
- spin_unlock_bh(&apic->lock);
}

u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
@@ -749,9 +734,7 @@ u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)

if (!apic)
return 0;
- spin_lock_bh(&apic->lock);
tpr = (u64) apic_get_reg(apic, APIC_TASKPRI);
- spin_unlock_bh(&apic->lock);

return (tpr & 0xf0) >> 4;
}
@@ -766,7 +749,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
vcpu->apic_base = value;
return;
}
- spin_lock_bh(&apic->lock);
if (apic->vcpu->vcpu_id)
value &= ~MSR_IA32_APICBASE_BSP;

@@ -778,19 +760,11 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
apic_debug("apic base msr is 0x%016" PRIx64 ", and base address is "
"0x%lx.\n", vcpu->apic_base, apic->base_address);

- spin_unlock_bh(&apic->lock);
}

u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu)
{
- struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
- u64 base;
-
- spin_lock_bh(&apic->lock);
- base = vcpu->apic_base;
- spin_unlock_bh(&apic->lock);
-
- return base;
+ return vcpu->apic_base;
}
EXPORT_SYMBOL_GPL(kvm_lapic_get_base);

@@ -808,8 +782,6 @@ static void lapic_reset(struct kvm_vcpu *vcpu)
/* Stop the timer in case it's a reset to an active apic */
hrtimer_cancel(&apic->timer.dev);

- spin_lock_bh(&apic->lock);
-
apic_set_reg(apic, APIC_ID, vcpu->vcpu_id << 24);
apic_set_reg(apic, APIC_LVR, APIC_VERSION);

@@ -836,8 +808,6 @@ static void lapic_reset(struct kvm_vcpu *vcpu)
vcpu->apic_base |= MSR_IA32_APICBASE_BSP;
apic_update_ppr(apic);

- spin_unlock_bh(&apic->lock);
-
apic_debug(KERN_INFO "%s: vcpu=%p, id=%d, base_msr="
"0x%016" PRIx64 ", base_address=0x%0lx.\n", __FUNCTION__,
vcpu, GET_APIC_ID(apic_get_reg(apic, APIC_ID)),
@@ -851,24 +821,13 @@ int kvm_lapic_enabled(struct kvm_vcpu *vcpu)

if (!apic)
return 0;
- spin_lock_bh(&apic->lock);
ret = apic_enabled(apic);
- spin_unlock_bh(&apic->lock);

return ret;
}
EXPORT_SYMBOL_GPL(kvm_lapic_enabled);

/*
-void *kvm_lapic_get_regs(struct kvm_vcpu *vcpu)
-{
- struct kvm_lapic *apic = (struct kvm_lapic *)vcpu->apic;
- return apic->regs;
-}
-EXPORT_SYMBOL_GPL(kvm_lapic_get_regs);
-*/
-
-/*
*----------------------------------------------------------------------
* timer interface
*----------------------------------------------------------------------
@@ -907,9 +866,7 @@ static enum hrtimer_restart apic_timer_fn(struct hrtimer *data)

apic = container_of(data, struct kvm_lapic, timer.dev);

- spin_lock_bh(&apic->lock);
restart_timer = __apic_timer_fn(apic);
- spin_unlock_bh(&apic->lock);

if (restart_timer)
return HRTIMER_RESTART;
@@ -929,7 +886,6 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
goto nomem;

vcpu->apic = apic;
- spin_lock_init(&apic->lock);

apic->regs_page = alloc_page(GFP_KERNEL);
if (apic->regs_page == NULL) {
@@ -1024,7 +980,6 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
apic_update_ppr(apic);

/* TODO: following code can be in a common API */
- spin_lock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
atomic_set(&apic->timer.pending, 0);
val = apic_get_reg(apic, APIC_TDCR);
@@ -1037,5 +992,18 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
hrtimer_start(&apic->timer.dev,
ktime_add_ns(now, offset),
HRTIMER_MODE_ABS);
- spin_unlock_bh(&apic->lock);
}
+
+void kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
+{
+ struct kvm_lapic *apic = vcpu->apic;
+ struct hrtimer *timer;
+
+ if (apic) {
+ timer = &apic->timer.dev;
+ hrtimer_cancel(timer);
+ hrtimer_start(timer, timer->expires, HRTIMER_MODE_ABS);
+ }
+}
+EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer);
+
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index c64fe6d..c584626 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -633,6 +633,7 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
delta = vcpu->host_tsc - tsc_this;
svm->vmcb->control.tsc_offset += delta;
vcpu->cpu = cpu;
+ kvm_migrate_apic_timer(vcpu);
}

for (i = 0; i < NR_HOST_SAVE_USER_MSRS; i++)
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index b537104..5ea1132 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -441,8 +441,10 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
u64 phys_addr = __pa(vmx->vmcs);
u64 tsc_this, delta;

- if (vcpu->cpu != cpu)
+ if (vcpu->cpu != cpu) {
vcpu_clear(vmx);
+ kvm_migrate_apic_timer(vcpu);
+ }

if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
u8 error;

[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org