[PATCH 4/6] KVM: in-kernel LAPIC save and restore support

[PATCH 4/6] KVM: in-kernel LAPIC save and restore support

[He, Qing]

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

    KVM: in-kernel LAPIC save and restore support
    
    This patch adds a new vcpu-based IOCTL to save and restore the local
apic
    registers for a single vcpu. The kernel only copies the apic page as
a whole,
    extraction of registers is left to userspace side. On restore, the
APIC timer
    is restarted from the initial count, this introduces a little delay,
but works
    fine
    
    Signed-off-by: Yaozu (Eddie) Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
    Signed-off-by: Qing He <qing.he-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

---
 drivers/kvm/irq.h      |    1 +
 drivers/kvm/kvm_main.c |   46
++++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/lapic.c    |   30 ++++++++++++++++++++++++++++++
 include/linux/kvm.h    |    8 ++++++++
 4 files changed, 85 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
index 92e1cfb..44e1fa4 100644
--- a/drivers/kvm/irq.h
+++ b/drivers/kvm/irq.h
@@ -149,6 +149,7 @@ int kvm_apic_match_physical_addr(struct kvm_lapic
*apic, u16 dest);
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector);
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
 int kvm_apic_set_irq(struct kvm_lapic *apic, u8 vec, u8 trig);
+void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
 
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 4c66f6d..8fee9bc 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2636,6 +2636,27 @@ static int kvm_vcpu_ioctl_set_fpu(struct kvm_vcpu
*vcpu, struct kvm_fpu *fpu)
 	return 0;
 }
 
+static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
+				    struct kvm_lapic_state *s)
+{
+	vcpu_load(vcpu);
+	memcpy(s->regs, vcpu->apic->regs, sizeof *s);
+	vcpu_put(vcpu);
+
+	return 0;
+}
+
+static int kvm_vcpu_ioctl_set_lapic(struct kvm_vcpu *vcpu,
+				    struct kvm_lapic_state *s)
+{
+	vcpu_load(vcpu);
+	memcpy(vcpu->apic->regs, s->regs, sizeof *s);
+	kvm_apic_post_state_restore(vcpu);
+	vcpu_put(vcpu);
+
+	return 0;
+}
+
 static long kvm_vcpu_ioctl(struct file *filp,
 			   unsigned int ioctl, unsigned long arg)
 {
@@ -2805,6 +2826,31 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		r = 0;
 		break;
 	}
+	case KVM_GET_LAPIC: {
+		struct kvm_lapic_state lapic;
+
+		memset(&lapic, 0, sizeof lapic);
+		r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic);
+		if (r)
+			goto out;
+		r = -EFAULT;
+		if (copy_to_user(argp, &lapic, sizeof lapic))
+			goto out;
+		r = 0;
+		break;
+	}
+	case KVM_SET_LAPIC: {
+		struct kvm_lapic_state lapic;
+
+		r = -EFAULT;
+		if (copy_from_user(&lapic, argp, sizeof lapic))
+			goto out;
+		r = kvm_vcpu_ioctl_set_lapic(vcpu, &lapic);;
+		if (r)
+			goto out;
+		r = 0;
+		break;
+	}
 	default:
 		;
 	}
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index b198434..422e7ea 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -983,3 +983,33 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	apic_clear_irr(vector, apic);
 	return vector;
 }
+
+void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->apic;
+ 	ktime_t now;
+ 	u32 offset, val, tmp;
+ 
+	apic->base_address = vcpu->apic_base &
+			     MSR_IA32_APICBASE_BASE;
+	apic_set_reg(apic, APIC_LVR, APIC_VERSION);
+	val = apic_get_reg(apic, APIC_TMICT);
+	apic_set_reg(apic, APIC_TMCCT, val);
+	apic_update_ppr(apic);
+
+ 	/* TODO: following code can be in a common API */
+	spin_lock_bh(&apic->lock);
+ 	hrtimer_cancel(&apic->timer.dev);
+	apic->timer.pending = 0;
+	val = apic_get_reg(apic, APIC_TDCR);
+	tmp = ((val & 0x3) | ((val & 0x8) >> 1)) + 1;
+	apic->timer.divide_count = 0x1 << (tmp & 0x7);
+ 	now = apic->timer.dev.base->get_time();
+ 	apic->timer.last_update = now;
+	val = apic_get_reg(apic, APIC_TMICT);
+	offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count * val;
+ 	hrtimer_start(&apic->timer.dev,
+ 		      ktime_add_ns(now, offset),
+ 		      HRTIMER_MODE_ABS);
+	spin_unlock_bh(&apic->lock);
+}
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 3295a06..6cddf62 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -207,6 +207,12 @@ struct kvm_fpu {
 	__u32 pad2;
 };
 
+/* for KVM_GET_LAPIC and KVM_SET_LAPIC */
+#define KVM_APIC_REG_SIZE 0x400
+struct kvm_lapic_state {
+	char regs[KVM_APIC_REG_SIZE];
+};
+
 struct kvm_segment {
 	__u64 base;
 	__u32 limit;
@@ -379,5 +385,7 @@ struct kvm_signal_mask {
 #define KVM_SET_SIGNAL_MASK       _IOW(KVMIO,  0x8b, struct
kvm_signal_mask)
 #define KVM_GET_FPU               _IOR(KVMIO,  0x8c, struct kvm_fpu)
 #define KVM_SET_FPU               _IOW(KVMIO,  0x8d, struct kvm_fpu)
+#define KVM_GET_LAPIC             _IOR(KVMIO,  0x8e, struct
kvm_lapic_state)
+#define KVM_SET_LAPIC             _IOW(KVMIO,  0x8f, struct
kvm_lapic_state)
 
 #endif


[-- Attachment #2: kvm-kern-lapic-lm.patch --]
[-- Type: application/octet-stream, Size: 4817 bytes --]

    KVM: in-kernel LAPIC save and restore support
    
    This patch adds a new vcpu-based IOCTL to save and restore the local apic
    registers for a single vcpu. The kernel only copies the apic page as a whole,
    extraction of registers is left to userspace side. On restore, the APIC timer
    is restarted from the initial count, this introduces a little delay, but works
    fine
    
    Signed-off-by: Yaozu (Eddie) Dong <eddie.dong@intel.com>
    Signed-off-by: Qing He <qing.he@intel.com>

---
 drivers/kvm/irq.h      |    1 +
 drivers/kvm/kvm_main.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/lapic.c    |   30 ++++++++++++++++++++++++++++++
 include/linux/kvm.h    |    8 ++++++++
 4 files changed, 85 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
index 92e1cfb..44e1fa4 100644
--- a/drivers/kvm/irq.h
+++ b/drivers/kvm/irq.h
@@ -149,6 +149,7 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector);
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
 int kvm_apic_set_irq(struct kvm_lapic *apic, u8 vec, u8 trig);
+void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
 
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 4c66f6d..8fee9bc 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2636,6 +2636,27 @@ static int kvm_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 	return 0;
 }
 
+static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
+				    struct kvm_lapic_state *s)
+{
+	vcpu_load(vcpu);
+	memcpy(s->regs, vcpu->apic->regs, sizeof *s);
+	vcpu_put(vcpu);
+
+	return 0;
+}
+
+static int kvm_vcpu_ioctl_set_lapic(struct kvm_vcpu *vcpu,
+				    struct kvm_lapic_state *s)
+{
+	vcpu_load(vcpu);
+	memcpy(vcpu->apic->regs, s->regs, sizeof *s);
+	kvm_apic_post_state_restore(vcpu);
+	vcpu_put(vcpu);
+
+	return 0;
+}
+
 static long kvm_vcpu_ioctl(struct file *filp,
 			   unsigned int ioctl, unsigned long arg)
 {
@@ -2805,6 +2826,31 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		r = 0;
 		break;
 	}
+	case KVM_GET_LAPIC: {
+		struct kvm_lapic_state lapic;
+
+		memset(&lapic, 0, sizeof lapic);
+		r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic);
+		if (r)
+			goto out;
+		r = -EFAULT;
+		if (copy_to_user(argp, &lapic, sizeof lapic))
+			goto out;
+		r = 0;
+		break;
+	}
+	case KVM_SET_LAPIC: {
+		struct kvm_lapic_state lapic;
+
+		r = -EFAULT;
+		if (copy_from_user(&lapic, argp, sizeof lapic))
+			goto out;
+		r = kvm_vcpu_ioctl_set_lapic(vcpu, &lapic);;
+		if (r)
+			goto out;
+		r = 0;
+		break;
+	}
 	default:
 		;
 	}
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index b198434..422e7ea 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -983,3 +983,33 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	apic_clear_irr(vector, apic);
 	return vector;
 }
+
+void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->apic;
+ 	ktime_t now;
+ 	u32 offset, val, tmp;
+ 
+	apic->base_address = vcpu->apic_base &
+			     MSR_IA32_APICBASE_BASE;
+	apic_set_reg(apic, APIC_LVR, APIC_VERSION);
+	val = apic_get_reg(apic, APIC_TMICT);
+	apic_set_reg(apic, APIC_TMCCT, val);
+	apic_update_ppr(apic);
+
+ 	/* TODO: following code can be in a common API */
+	spin_lock_bh(&apic->lock);
+ 	hrtimer_cancel(&apic->timer.dev);
+	apic->timer.pending = 0;
+	val = apic_get_reg(apic, APIC_TDCR);
+	tmp = ((val & 0x3) | ((val & 0x8) >> 1)) + 1;
+	apic->timer.divide_count = 0x1 << (tmp & 0x7);
+ 	now = apic->timer.dev.base->get_time();
+ 	apic->timer.last_update = now;
+	val = apic_get_reg(apic, APIC_TMICT);
+	offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count * val;
+ 	hrtimer_start(&apic->timer.dev,
+ 		      ktime_add_ns(now, offset),
+ 		      HRTIMER_MODE_ABS);
+	spin_unlock_bh(&apic->lock);
+}
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 3295a06..6cddf62 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -207,6 +207,12 @@ struct kvm_fpu {
 	__u32 pad2;
 };
 
+/* for KVM_GET_LAPIC and KVM_SET_LAPIC */
+#define KVM_APIC_REG_SIZE 0x400
+struct kvm_lapic_state {
+	char regs[KVM_APIC_REG_SIZE];
+};
+
 struct kvm_segment {
 	__u64 base;
 	__u32 limit;
@@ -379,5 +385,7 @@ struct kvm_signal_mask {
 #define KVM_SET_SIGNAL_MASK       _IOW(KVMIO,  0x8b, struct kvm_signal_mask)
 #define KVM_GET_FPU               _IOR(KVMIO,  0x8c, struct kvm_fpu)
 #define KVM_SET_FPU               _IOW(KVMIO,  0x8d, struct kvm_fpu)
+#define KVM_GET_LAPIC             _IOR(KVMIO,  0x8e, struct kvm_lapic_state)
+#define KVM_SET_LAPIC             _IOW(KVMIO,  0x8f, struct kvm_lapic_state)
 
 #endif


[-- 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
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 4/6] KVM: in-kernel LAPIC save and restore support

[Avi Kivity]

He, Qing wrote:
>     KVM: in-kernel LAPIC save and restore support
>     
>     This patch adds a new vcpu-based IOCTL to save and restore the local
> apic
>     registers for a single vcpu. The kernel only copies the apic page as
> a whole,
>     extraction of registers is left to userspace side. On restore, the
> APIC timer
>     is restarted from the initial count, this introduces a little delay,
> but works
>     fine
>     
>     Signed-off-by: Yaozu (Eddie) Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Qing He <qing.he-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
>  static long kvm_vcpu_ioctl(struct file *filp,
>  			   unsigned int ioctl, unsigned long arg)
>  {
> @@ -2805,6 +2826,31 @@ static long kvm_vcpu_ioctl(struct file *filp,
>  		r = 0;
>  		break;
>  	}
> +	case KVM_GET_LAPIC: {
> +		struct kvm_lapic_state lapic;
> +
> +		memset(&lapic, 0, sizeof lapic);
> +		r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic);
> +		if (r)
> +			goto out;
> +		r = -EFAULT;
> +		if (copy_to_user(argp, &lapic, sizeof lapic))
> +			goto out;
> +		r = 0;
> +		break;
> +	}
>  
> +/* for KVM_GET_LAPIC and KVM_SET_LAPIC */
> +#define KVM_APIC_REG_SIZE 0x400
> +struct kvm_lapic_state {
> +	char regs[KVM_APIC_REG_SIZE];
> +};
> +
>   

kvm_lapic_state is 1KB in size, and you are allocating it on the stack.  
On i386, the stack can be 4KB, and you're allocating  25% of it...

While it's true that this code path is short and can't be compounded 
with others like I/O code paths, still it's not a good idea to allocate 
so much stack space.  I suggest defining kvm_lapic_state as

#define KVM_LAPIC_NR_REGS 0x40
struct kvm_lapic_state {
    u32 regs[KVM_LAPIC_NR_REGS];
};

That reduces the state size to 256 bytes.

-- 
error compiling committee.c: too many arguments to function


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

Re: [PATCH 4/6] KVM: in-kernel LAPIC save and restore support

[Dong, Eddie]

kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org wrote:
> He, Qing wrote:
>>     KVM: in-kernel LAPIC save and restore support
>> 
>>     This patch adds a new vcpu-based IOCTL to save and restore the
>>     local apic registers for a single vcpu. The kernel only copies
>>     the apic page as a whole, extraction of registers is left to
>>     userspace side. On restore, the APIC timer is restarted from the
>>     initial count, this introduces a little delay, but works fine
>> 
>>     Signed-off-by: Yaozu (Eddie) Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>     Signed-off-by: Qing He <qing.he-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> 
>>  static long kvm_vcpu_ioctl(struct file *filp,
>>  			   unsigned int ioctl, unsigned long arg)
>>  {
>> @@ -2805,6 +2826,31 @@ static long kvm_vcpu_ioctl(struct file *filp,
>>  		r = 0; break;
>>  	}
>> +	case KVM_GET_LAPIC: {
>> +		struct kvm_lapic_state lapic;
>> +
>> +		memset(&lapic, 0, sizeof lapic);
>> +		r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic);
>> +		if (r)
>> +			goto out;
>> +		r = -EFAULT;
>> +		if (copy_to_user(argp, &lapic, sizeof lapic))
>> +			goto out;
>> +		r = 0;
>> +		break;
>> +	}
>> 
>> +/* for KVM_GET_LAPIC and KVM_SET_LAPIC */
>> +#define KVM_APIC_REG_SIZE 0x400
>> +struct kvm_lapic_state {
>> +	char regs[KVM_APIC_REG_SIZE];
>> +};
>> +
>> 
> 
> kvm_lapic_state is 1KB in size, and you are allocating it on
> the stack.
> On i386, the stack can be 4KB, and you're allocating  25% of it...
> 

How about we use dynamic allocate like kzalloc?

> While it's true that this code path is short and can't be compounded
> with others like I/O code paths, still it's not a good idea to
> allocate so much stack space.  I suggest defining kvm_lapic_state as
> 
> #define KVM_LAPIC_NR_REGS 0x40
> struct kvm_lapic_state {
>    u32 regs[KVM_LAPIC_NR_REGS];
> };
> 
> That reduces the state size to 256 bytes.

It could. 
The benefit of defining whole 1K register is that it is exactly same 
with real hardware which means we never need to worry about future data
structure change no matter in user level or kernel level, they will
always 
be able to fit into this state. Today kernel level use full hardware
register
state due to hardware acceleration consideration while user level use
similar
with what you suggested here.

Leave to your decision :-)

thx,eddie

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