[PATCH 0/3 v2] x2APIC emulation for kvm

[PATCH 0/3 v2] x2APIC emulation for kvm

[Gleb Natapov]

This patch series implements x2APIC emulation for kvm. x2APIC is an MSR
interface to a local apic with performance/scalability enhancements. It
brings 32 bit apic ids (ids > 255 cannot be used without interrupt
remapping since MSR/IOAPIC still support 8 bit destination IDs), 64bit
ICR access, reading of ICR after IPI is no longer required.

--
	Gleb.

[PATCH 1/3 v2] Add Directed EOI support to APIC emulation

[Gleb Natapov]

Directed EOI is specified by x2APIC, but is available even when lapic is
in xAPIC mode.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/include/asm/apicdef.h |    2 ++
 arch/x86/kvm/lapic.c           |   13 ++++++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 7ddb36a..74ca38f 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -14,6 +14,7 @@
 
 #define	APIC_LVR	0x30
 #define		APIC_LVR_MASK		0xFF00FF
+#define		APIC_LVR_DIRECTED_EOI	(1 << 24)
 #define		GET_APIC_VERSION(x)	((x) & 0xFFu)
 #define		GET_APIC_MAXLVT(x)	(((x) >> 16) & 0xFFu)
 #ifdef CONFIG_X86_32
@@ -40,6 +41,7 @@
 #define		APIC_DFR_CLUSTER		0x0FFFFFFFul
 #define		APIC_DFR_FLAT			0xFFFFFFFFul
 #define	APIC_SPIV	0xF0
+#define		APIC_SPIV_DIRECTED_EOI		(1 << 12)
 #define		APIC_SPIV_FOCUS_DISABLED	(1 << 9)
 #define		APIC_SPIV_APIC_ENABLED		(1 << 8)
 #define	APIC_ISR	0x100
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2e02865..db0c6ae 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -54,7 +54,8 @@
 
 #define APIC_LVT_NUM			6
 /* 14 is the version for Xeon and Pentium 8.4.8*/
-#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1) << 16))
+#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1) << 16) | \
+					 APIC_LVR_DIRECTED_EOI)
 #define LAPIC_MMIO_LENGTH		(1 << 12)
 /* followed define is not in apicdef.h */
 #define APIC_SHORT_MASK			0xc0000
@@ -442,9 +443,11 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 		trigger_mode = IOAPIC_LEVEL_TRIG;
 	else
 		trigger_mode = IOAPIC_EDGE_TRIG;
-	mutex_lock(&apic->vcpu->kvm->irq_lock);
-	kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode);
-	mutex_unlock(&apic->vcpu->kvm->irq_lock);
+	if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)) {
+		mutex_lock(&apic->vcpu->kvm->irq_lock);
+		kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode);
+		mutex_unlock(&apic->vcpu->kvm->irq_lock);
+	}
 }
 
 static void apic_send_ipi(struct kvm_lapic *apic)
@@ -683,7 +686,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
 		break;
 
 	case APIC_SPIV:
-		apic_set_reg(apic, APIC_SPIV, val & 0x3ff);
+		apic_set_reg(apic, APIC_SPIV, val & 0xfff);
 		if (!(val & APIC_SPIV_APIC_ENABLED)) {
 			int i;
 			u32 lvt_val;
-- 
1.6.2.1

[PATCH 2/3 v2] x2APIC interface to local apic

[Gleb Natapov]

This patch implements MSR interface to a local apic as defines by x2APIC
Intel specification.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |  193 ++++++++++++++++++++++++++++++++++++++------------
 arch/x86/kvm/lapic.h |    2 +
 arch/x86/kvm/x86.c   |    7 ++-
 3 files changed, 154 insertions(+), 48 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index db0c6ae..983f414 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -32,6 +32,7 @@
 #include <asm/current.h>
 #include <asm/apicdef.h>
 #include <asm/atomic.h>
+#include <asm/apicdef.h>
 #include "kvm_cache_regs.h"
 #include "irq.h"
 #include "trace.h"
@@ -143,6 +144,11 @@ static inline int apic_lvt_nmi_mode(u32 lvt_val)
 	return (lvt_val & (APIC_MODE_MASK | APIC_LVT_MASKED)) == APIC_DM_NMI;
 }
 
+static inline int apic_x2apic_mode(struct kvm_lapic *apic)
+{
+	return apic->vcpu->arch.apic_base & X2APIC_ENABLE;
+}
+
 static unsigned int apic_lvt_mask[APIC_LVT_NUM] = {
 	LVT_MASK | APIC_LVT_TIMER_PERIODIC,	/* LVTT */
 	LVT_MASK | APIC_MODE_MASK,	/* LVTTHMR */
@@ -269,7 +275,12 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
 {
 	int result = 0;
-	u8 logical_id;
+	u32 logical_id;
+
+	if (apic_x2apic_mode(apic)) {
+		logical_id = apic_get_reg(apic, APIC_LDR);
+		return logical_id & (uint16_t)mda;
+	}
 
 	logical_id = GET_APIC_LOGICAL_ID(apic_get_reg(apic, APIC_LDR));
 
@@ -462,7 +473,10 @@ static void apic_send_ipi(struct kvm_lapic *apic)
 	irq.level = icr_low & APIC_INT_ASSERT;
 	irq.trig_mode = icr_low & APIC_INT_LEVELTRIG;
 	irq.shorthand = icr_low & APIC_SHORT_MASK;
-	irq.dest_id = GET_APIC_DEST_FIELD(icr_high);
+	if (apic_x2apic_mode(apic))
+		irq.dest_id = icr_high;
+	else
+		irq.dest_id = GET_APIC_DEST_FIELD(icr_high);
 
 	apic_debug("icr_high 0x%x, icr_low 0x%x, "
 		   "short_hand 0x%x, dest 0x%x, trig_mode 0x%x, level 0x%x, "
@@ -523,6 +537,9 @@ static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
 		return 0;
 
 	switch (offset) {
+	case APIC_ID:
+		apic_get_reg(apic, offset);
+		break;
 	case APIC_ARBPRI:
 		printk(KERN_WARNING "Access APIC ARBPRI register "
 		       "which is for P6\n");
@@ -549,19 +566,26 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
 	return container_of(dev, struct kvm_lapic, dev);
 }
 
-static void apic_mmio_read(struct kvm_io_device *this,
-			   gpa_t address, int len, void *data)
+static int apic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
+		void *data)
 {
-	struct kvm_lapic *apic = to_lapic(this);
-	unsigned int offset = address - apic->base_address;
 	unsigned char alignment = offset & 0xf;
 	u32 result;
+	/* this bitmask has a bit cleared for each reserver register */
+	static const uint64_t rmask = 0x43ff01ffffffe70c; 
 
 	if ((alignment + len) > 4) {
-		printk(KERN_ERR "KVM_APIC_READ: alignment error %lx %d",
-		       (unsigned long)address, len);
-		return;
+		printk(KERN_ERR "KVM_APIC_READ: alignment error %x %d\n",
+				offset, len);
+		return 1;
 	}
+
+	if (offset > 0x3f0 || !(rmask & (1ULL << (offset >> 4)))) {
+		printk(KERN_ERR "KVM_APIC_READ: read reserved register %x\n",
+				offset);
+		return 1;
+	}
+
 	result = __apic_read(apic, offset & ~0xf);
 
 	trace_kvm_apic_read(offset, result);
@@ -577,6 +601,16 @@ static void apic_mmio_read(struct kvm_io_device *this,
 		       "should be 1,2, or 4 instead\n", len);
 		break;
 	}
+	return 0;
+}
+
+static void apic_mmio_read(struct kvm_io_device *this,
+			   gpa_t address, int len, void *data)
+{
+	struct kvm_lapic *apic = to_lapic(this);
+	u32 offset = address - apic->base_address;
+
+	apic_reg_read(apic, offset, len, data);
 }
 
 static void update_divide_count(struct kvm_lapic *apic)
@@ -632,40 +666,18 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
 		apic->vcpu->kvm->arch.vapics_in_nmi_mode--;
 }
 
-static void apic_mmio_write(struct kvm_io_device *this,
-			    gpa_t address, int len, const void *data)
+static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 {
-	struct kvm_lapic *apic = to_lapic(this);
-	unsigned int offset = address - apic->base_address;
-	unsigned char alignment = offset & 0xf;
-	u32 val;
-
-	/*
-	 * APIC register must be aligned on 128-bits boundary.
-	 * 32/64/128 bits registers must be accessed thru 32 bits.
-	 * Refer SDM 8.4.1
-	 */
-	if (len != 4 || alignment) {
-		/* Don't shout loud, $infamous_os would cause only noise. */
-		apic_debug("apic write: bad size=%d %lx\n",
-			   len, (long)address);
-		return;
-	}
-
-	val = *(u32 *) data;
-
-	/* too common printing */
-	if (offset != APIC_EOI)
-		apic_debug("%s: offset 0x%x with length 0x%x, and value is "
-			   "0x%x\n", __func__, offset, len, val);
-
-	offset &= 0xff0;
+	int ret = 0;
 
-	trace_kvm_apic_write(offset, val);
+	trace_kvm_apic_write(reg, val);
 
-	switch (offset) {
+	switch (reg) {
 	case APIC_ID:		/* Local APIC ID */
-		apic_set_reg(apic, APIC_ID, val);
+		if (!apic_x2apic_mode(apic))
+			apic_set_reg(apic, APIC_ID, val);
+		else
+			ret = 1;
 		break;
 
 	case APIC_TASKPRI:
@@ -678,11 +690,17 @@ static void apic_mmio_write(struct kvm_io_device *this,
 		break;
 
 	case APIC_LDR:
-		apic_set_reg(apic, APIC_LDR, val & APIC_LDR_MASK);
+		if (!apic_x2apic_mode(apic))
+			apic_set_reg(apic, APIC_LDR, val & APIC_LDR_MASK);
+		else
+			ret = 1;
 		break;
 
 	case APIC_DFR:
-		apic_set_reg(apic, APIC_DFR, val | 0x0FFFFFFF);
+		if (!apic_x2apic_mode(apic))
+			apic_set_reg(apic, APIC_DFR, val | 0x0FFFFFFF);
+		else
+			ret = 1;
 		break;
 
 	case APIC_SPIV:
@@ -709,7 +727,9 @@ static void apic_mmio_write(struct kvm_io_device *this,
 		break;
 
 	case APIC_ICR2:
-		apic_set_reg(apic, APIC_ICR2, val & 0xff000000);
+		if (!apic_x2apic_mode(apic))
+			val &= 0xff000000;
+		apic_set_reg(apic, APIC_ICR2, val);
 		break;
 
 	case APIC_LVT0:
@@ -723,8 +743,8 @@ static void apic_mmio_write(struct kvm_io_device *this,
 		if (!apic_sw_enabled(apic))
 			val |= APIC_LVT_MASKED;
 
-		val &= apic_lvt_mask[(offset - APIC_LVTT) >> 4];
-		apic_set_reg(apic, offset, val);
+		val &= apic_lvt_mask[(reg - APIC_LVTT) >> 4];
+		apic_set_reg(apic, reg, val);
 
 		break;
 
@@ -732,7 +752,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
 		hrtimer_cancel(&apic->lapic_timer.timer);
 		apic_set_reg(apic, APIC_TMICT, val);
 		start_apic_timer(apic);
-		return;
+		break;
 
 	case APIC_TDCR:
 		if (val & 4)
@@ -741,12 +761,54 @@ static void apic_mmio_write(struct kvm_io_device *this,
 		update_divide_count(apic);
 		break;
 
+	case APIC_ESR:
+		if (apic_x2apic_mode(apic) && val != 0) {
+			printk(KERN_ERR "KVM_WRITE:ESR not zero %x\n", val);
+			ret = 1;
+		}
+		break;
+
+	case APIC_SELF_IPI:
+		if (apic_x2apic_mode(apic)) {
+			apic_reg_write(apic, APIC_ICR, 0x40000 | (val & 0xff));
+		} else
+			ret = 1;
+		break;
 	default:
-		apic_debug("Local APIC Write to read-only register %x\n",
-			   offset);
+		ret = 1;
 		break;
 	}
+	if (ret)
+		apic_debug("Local APIC Write to read-only register %x\n", reg);
+	return ret;
+}
 
+static void apic_mmio_write(struct kvm_io_device *this,
+			    gpa_t address, int len, const void *data)
+{
+	struct kvm_lapic *apic = to_lapic(this);
+	unsigned int offset = address - apic->base_address;
+	u32 val;
+
+	/*
+	 * APIC register must be aligned on 128-bits boundary.
+	 * 32/64/128 bits registers must be accessed thru 32 bits.
+	 * Refer SDM 8.4.1
+	 */
+	if (len != 4 || (offset & 0xf)) {
+		/* Don't shout loud, $infamous_os would cause only noise. */
+		apic_debug("apic write: bad size=%d %lx\n", len, (long)address);
+		return;
+	}
+
+	val = *(u32*)data;
+
+	/* too common printing */
+	if (offset != APIC_EOI)
+		apic_debug("%s: offset 0x%x with length 0x%x, and value is "
+			   "0x%x\n", __func__, offset, len, val);
+
+	apic_reg_write(apic, offset & 0xff0, val);
 }
 
 static int apic_mmio_range(struct kvm_io_device *this, gpa_t addr,
@@ -819,6 +881,11 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 		value &= ~MSR_IA32_APICBASE_BSP;
 
 	vcpu->arch.apic_base = value;
+	if (apic_x2apic_mode(apic)) {
+		u32 id = kvm_apic_id(apic);
+		u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
+		apic_set_reg(apic, APIC_LDR, ldr);
+	}
 	apic->base_address = apic->vcpu->arch.apic_base &
 			     MSR_IA32_APICBASE_BASE;
 
@@ -1115,3 +1182,35 @@ void kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 	vcpu->arch.apic->vapic_addr = vapic_addr;
 }
+
+int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	u32 reg = (msr - APIC_BASE_MSR) << 4;
+
+	if (!apic_x2apic_mode(apic))
+		return 1;
+
+	/* if this is ICR write vector before command */
+	if (msr == 0x830)
+		apic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
+	return apic_reg_write(apic, reg, (u32)data);
+}
+
+int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	u32 reg = (msr - APIC_BASE_MSR) << 4, low, high = 0;
+
+	if (!apic_x2apic_mode(apic))
+		return 1;
+
+	if (apic_reg_read(apic, reg, 4, &low))
+		return 1;
+	if (msr == 0x830)
+		apic_reg_read(apic, APIC_ICR2, 4, &high);
+
+	*data = (((u64)high) << 32) | low;
+
+	return 0;
+}
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 3f3ecc6..fddaa9b 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -45,4 +45,6 @@ void kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr);
 void kvm_lapic_sync_from_vapic(struct kvm_vcpu *vcpu);
 void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu);
 
+int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
+int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5a66bb9..8ead54d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -817,6 +817,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	case MSR_IA32_APICBASE:
 		kvm_set_apic_base(vcpu, data);
 		break;
+	case APIC_BASE_MSR ... APIC_BASE_MSR + 0x3ff:
+		return kvm_x2apic_msr_write(vcpu, msr, data);
 	case MSR_IA32_MISC_ENABLE:
 		vcpu->arch.ia32_misc_enable_msr = data;
 		break;
@@ -1006,6 +1008,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_IA32_APICBASE:
 		data = kvm_get_apic_base(vcpu);
 		break;
+	case APIC_BASE_MSR ... APIC_BASE_MSR + 0x3ff:
+		return kvm_x2apic_msr_read(vcpu, msr, pdata);
+		break;
 	case MSR_IA32_MISC_ENABLE:
 		data = vcpu->arch.ia32_misc_enable_msr;
 		break;
@@ -1403,7 +1408,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
 		0 /* Reserved */ | F(CX16) | 0 /* xTPR Update, PDCM */ |
 		0 /* Reserved, DCA */ | F(XMM4_1) |
-		F(XMM4_2) | 0 /* x2APIC */ | F(MOVBE) | F(POPCNT) |
+		F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
 		0 /* Reserved, XSAVE, OSXSAVE */;
 	/* cpuid 0x80000001.ecx */
 	const u32 kvm_supported_word6_x86_features =
-- 
1.6.2.1

[PATCH 3/3 v2] Add x2APIC support to qemu-kvm.

[Gleb Natapov]

Add "x2apic" string to extended features name array to be recognizable
by -cpu cputype,+x2apic command line option. If kvm kernel module does
not support x2APIC the option will be trimmed from cpuid.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 target-i386/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index cc34c87..cf6153b 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -45,7 +45,7 @@ static const char *feature_name[] = {
 static const char *ext_feature_name[] = {
     "pni" /* Intel,AMD sse3 */, NULL, NULL, "monitor", "ds_cpl", "vmx", NULL /* Linux smx */, "est",
     "tm2", "ssse3", "cid", NULL, NULL, "cx16", "xtpr", NULL,
-    NULL, NULL, "dca", NULL, NULL, NULL, NULL, "popcnt",
+    NULL, NULL, "dca", NULL, NULL, "x2apic", NULL, "popcnt",
        NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 static const char *ext2_feature_name[] = {
-- 
1.6.2.1

Re: [PATCH 1/3 v2] Add Directed EOI support to APIC emulation

[Avi Kivity]

On 06/28/2009 03:15 PM, Gleb Natapov wrote:
> Directed EOI is specified by x2APIC, but is available even when lapic is
> in xAPIC mode.
>
>   #define APIC_LVT_NUM			6
>   /* 14 is the version for Xeon and Pentium 8.4.8*/
> -#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<  16))
> +#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<  16) | \
> +					 APIC_LVR_DIRECTED_EOI)
>    

Better make that depend on the x2apic cpuid bit.

>   static void apic_send_ipi(struct kvm_lapic *apic)
> @@ -683,7 +686,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
>   		break;
>
>   	case APIC_SPIV:
> -		apic_set_reg(apic, APIC_SPIV, val&  0x3ff);
> +		apic_set_reg(apic, APIC_SPIV, val&  0xfff);
>   		if (!(val&  APIC_SPIV_APIC_ENABLED)) {
>   			int i;
>   			u32 lvt_val;
>    

Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is 
bit 12?

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

Re: [PATCH 1/3 v2] Add Directed EOI support to APIC emulation

[Gleb Natapov]

On Mon, Jun 29, 2009 at 12:18:28PM +0300, Avi Kivity wrote:
> On 06/28/2009 03:15 PM, Gleb Natapov wrote:
>> Directed EOI is specified by x2APIC, but is available even when lapic is
>> in xAPIC mode.
>>
>>   #define APIC_LVT_NUM			6
>>   /* 14 is the version for Xeon and Pentium 8.4.8*/
>> -#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<  16))
>> +#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<  16) | \
>> +					 APIC_LVR_DIRECTED_EOI)
>>    
>
> Better make that depend on the x2apic cpuid bit.
>
Are you sure. It looks like this feature is independent from x2APIC. It just specified
by the same spec.

>>   static void apic_send_ipi(struct kvm_lapic *apic)
>> @@ -683,7 +686,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
>>   		break;
>>
>>   	case APIC_SPIV:
>> -		apic_set_reg(apic, APIC_SPIV, val&  0x3ff);
>> +		apic_set_reg(apic, APIC_SPIV, val&  0xfff);
>>   		if (!(val&  APIC_SPIV_APIC_ENABLED)) {
>>   			int i;
>>   			u32 lvt_val;
>>    
>
> Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is  
> bit 12?
For well behaved guests it doesn't matter :) And Intel keep changing
what reserved bits are in this register. Older doc says bit 9 is a Focus
Processor bit, x2APIC doc says bit 9 is registered. So what should we do
for bit 9?

--
			Gleb.

Re: [PATCH 2/3 v2] x2APIC interface to local apic

[Avi Kivity]

On 06/28/2009 03:15 PM, Gleb Natapov wrote:
> This patch implements MSR interface to a local apic as defines by x2APIC
> Intel specification.
>
> @@ -269,7 +275,12 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
>   int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
>   {
>   	int result = 0;
> -	u8 logical_id;
> +	u32 logical_id;
> +
> +	if (apic_x2apic_mode(apic)) {
> +		logical_id = apic_get_reg(apic, APIC_LDR);
> +		return logical_id&  (uint16_t)mda;
> +	}
>    

mda is u8, why cast it?

> +static int apic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> +		void *data)
>   {
> -	struct kvm_lapic *apic = to_lapic(this);
> -	unsigned int offset = address - apic->base_address;
>   	unsigned char alignment = offset&  0xf;
>   	u32 result;
> +	/* this bitmask has a bit cleared for each reserver register */
> +	static const uint64_t rmask = 0x43ff01ffffffe70c;
>    

s/uint64_t/u64/, add ULL to avoid warnings on i386?

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5a66bb9..8ead54d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -817,6 +817,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   	case MSR_IA32_APICBASE:
>   		kvm_set_apic_base(vcpu, data);
>   		break;
> +	case APIC_BASE_MSR ... APIC_BASE_MSR + 0x3ff:
> +		return kvm_x2apic_msr_write(vcpu, msr, data);
>    

Might be a good idea to special cases these in arch code to avoid the 
long if-trees the switches generate.  Only after profiling though.

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

Re: [PATCH 1/3 v2] Add Directed EOI support to APIC emulation

[Avi Kivity]

On 06/29/2009 12:29 PM, Gleb Natapov wrote:
> On Mon, Jun 29, 2009 at 12:18:28PM +0300, Avi Kivity wrote:
>    
>> On 06/28/2009 03:15 PM, Gleb Natapov wrote:
>>      
>>> Directed EOI is specified by x2APIC, but is available even when lapic is
>>> in xAPIC mode.
>>>
>>>    #define APIC_LVT_NUM			6
>>>    /* 14 is the version for Xeon and Pentium 8.4.8*/
>>> -#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<   16))
>>> +#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<   16) | \
>>> +					 APIC_LVR_DIRECTED_EOI)
>>>
>>>        
>> Better make that depend on the x2apic cpuid bit.
>>
>>      
> Are you sure. It looks like this feature is independent from x2APIC. It just specified
> by the same spec.
>    

We're changing something that the guests sees.  Suppose the guest has a 
bug in directed EOI, just upgrading kvm will cause it to trigger.  If we 
make it dependent on x2apic (or something else that needs to be selected 
by the user), we maintain compatibility.

>>>    	case APIC_SPIV:
>>> -		apic_set_reg(apic, APIC_SPIV, val&   0x3ff);
>>> +		apic_set_reg(apic, APIC_SPIV, val&   0xfff);
>>>    		if (!(val&   APIC_SPIV_APIC_ENABLED)) {
>>>    			int i;
>>>    			u32 lvt_val;
>>>
>>>        
>> Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is
>> bit 12?
>>      
> For well behaved guests it doesn't matter :) And Intel keep changing
> what reserved bits are in this register. Older doc says bit 9 is a Focus
> Processor bit, x2APIC doc says bit 9 is registered. So what should we do
> for bit 9?
>    

Let's make it a separate patch in case something blows.  I think you 
need to allow bit 9 even if x2apic retroactively reserves it.

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

Re: [PATCH 2/3 v2] x2APIC interface to local apic

[Gleb Natapov]

On Mon, Jun 29, 2009 at 12:42:02PM +0300, Avi Kivity wrote:
> On 06/28/2009 03:15 PM, Gleb Natapov wrote:
>> This patch implements MSR interface to a local apic as defines by x2APIC
>> Intel specification.
>>
>> @@ -269,7 +275,12 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
>>   int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
>>   {
>>   	int result = 0;
>> -	u8 logical_id;
>> +	u32 logical_id;
>> +
>> +	if (apic_x2apic_mode(apic)) {
>> +		logical_id = apic_get_reg(apic, APIC_LDR);
>> +		return logical_id&  (uint16_t)mda;
>> +	}
>>    
>
> mda is u8, why cast it?
Is mda will be zero extended without any cast at all? I suppose it
should.

--
			Gleb.

Re: [PATCH 1/3 v2] Add Directed EOI support to APIC emulation

[Gleb Natapov]

On Mon, Jun 29, 2009 at 12:49:00PM +0300, Avi Kivity wrote:
> On 06/29/2009 12:29 PM, Gleb Natapov wrote:
>> On Mon, Jun 29, 2009 at 12:18:28PM +0300, Avi Kivity wrote:
>>    
>>> On 06/28/2009 03:15 PM, Gleb Natapov wrote:
>>>      
>>>> Directed EOI is specified by x2APIC, but is available even when lapic is
>>>> in xAPIC mode.
>>>>
>>>>    #define APIC_LVT_NUM			6
>>>>    /* 14 is the version for Xeon and Pentium 8.4.8*/
>>>> -#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<   16))
>>>> +#define APIC_VERSION			(0x14UL | ((APIC_LVT_NUM - 1)<<   16) | \
>>>> +					 APIC_LVR_DIRECTED_EOI)
>>>>
>>>>        
>>> Better make that depend on the x2apic cpuid bit.
>>>
>>>      
>> Are you sure. It looks like this feature is independent from x2APIC. It just specified
>> by the same spec.
>>    
>
> We're changing something that the guests sees.  Suppose the guest has a  
> bug in directed EOI, just upgrading kvm will cause it to trigger.  If we  
> make it dependent on x2apic (or something else that needs to be selected  
> by the user), we maintain compatibility.
>
Yes, I thought about something else (not x2apic). But yet another command line
switch look like overkill.

>>>>    	case APIC_SPIV:
>>>> -		apic_set_reg(apic, APIC_SPIV, val&   0x3ff);
>>>> +		apic_set_reg(apic, APIC_SPIV, val&   0xfff);
>>>>    		if (!(val&   APIC_SPIV_APIC_ENABLED)) {
>>>>    			int i;
>>>>    			u32 lvt_val;
>>>>
>>>>        
>>> Confused, you're adding bits 10 and 11 while APIC_SPIV_DIRECTED_EOI is
>>> bit 12?
>>>      
>> For well behaved guests it doesn't matter :) And Intel keep changing
>> what reserved bits are in this register. Older doc says bit 9 is a Focus
>> Processor bit, x2APIC doc says bit 9 is registered. So what should we do
>> for bit 9?
>>    
>
> Let's make it a separate patch in case something blows.  I think you  
> need to allow bit 9 even if x2apic retroactively reserves it.
>
OK.

--
			Gleb.

Re: [PATCH 2/3 v2] x2APIC interface to local apic

[Avi Kivity]

On 06/29/2009 12:51 PM, Gleb Natapov wrote:
> On Mon, Jun 29, 2009 at 12:42:02PM +0300, Avi Kivity wrote:
>    
>> On 06/28/2009 03:15 PM, Gleb Natapov wrote:
>>      
>>> This patch implements MSR interface to a local apic as defines by x2APIC
>>> Intel specification.
>>>
>>> @@ -269,7 +275,12 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
>>>    int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
>>>    {
>>>    	int result = 0;
>>> -	u8 logical_id;
>>> +	u32 logical_id;
>>> +
>>> +	if (apic_x2apic_mode(apic)) {
>>> +		logical_id = apic_get_reg(apic, APIC_LDR);
>>> +		return logical_id&   (uint16_t)mda;
>>> +	}
>>>
>>>        
>> mda is u8, why cast it?
>>      
> Is mda will be zero extended without any cast at all? I suppose it
> should.
>
>    

Yes.

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

Re: [PATCH 1/3 v2] Add Directed EOI support to APIC emulation

[Avi Kivity]

On 06/29/2009 12:52 PM, Gleb Natapov wrote:
>> We're changing something that the guests sees.  Suppose the guest has a
>> bug in directed EOI, just upgrading kvm will cause it to trigger.  If we
>> make it dependent on x2apic (or something else that needs to be selected
>> by the user), we maintain compatibility.
>>
>>      
> Yes, I thought about something else (not x2apic). But yet another command line
> switch look like overkill.
>    

It is.  So enabling on x2apic avoids the compatibility issue and avoids 
adding another useless control.

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