[PATCH 0/1] x2apic implementation for kvm

[PATCH 0/1] x2apic implementation for kvm

[Gleb Natapov]

This is implementation of x2apic for KVM that I wrote a while ago.
Unfortunately there is no guest that can take advantage of it since
Linux doesn't (yet?) use x2apic if interrupt remapping is not enabled
and I don't feel like implement interrupt remapping device :)

Re-based to latest kvm tree for your viewing pleasure and feedback.

[PATCH 1/1] x2apic interface to lapic

[Gleb Natapov]

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ae99d83..80ddfdf 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"
 
@@ -141,6 +142,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 */
@@ -251,7 +257,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 & 0xffff);
+	}
 
 	logical_id = GET_APIC_LOGICAL_ID(apic_get_reg(apic, APIC_LDR));
 
@@ -440,7 +451,8 @@ 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);
+	irq.dest_id = apic_x2apic_mode(apic) ? icr_high :
+		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, "
@@ -501,6 +513,10 @@ static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
 		return 0;
 
 	switch (offset) {
+	case APIC_ID:
+		val = apic_x2apic_mode(apic) ? apic->vcpu->vcpu_id :
+			apic_get_reg(apic, offset);
+		break;
 	case APIC_ARBPRI:
 		printk(KERN_WARNING "Access APIC ARBPRI register "
 		       "which is for P6\n");
@@ -522,19 +538,25 @@ static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
 	return val;
 }
 
-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 = (struct kvm_lapic *)this->private;
-	unsigned int offset = address - apic->base_address;
 	unsigned char alignment = offset & 0xf;
 	u32 result;
+	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 (!(rmask & (1ULL << (offset >> 4)))) {
+		printk(KERN_ERR "KVM_APIC_READ: read reserved register %x\n",
+				offset);
+		return 1;
+	}
+
 	result = __apic_read(apic, offset & ~0xf);
 
 	switch (len) {
@@ -548,6 +570,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 = (struct kvm_lapic *)this->private;
+	u32 offset = address - apic->base_address;
+
+	apic_reg_read(apic, offset, len, data);
 }
 
 static void update_divide_count(struct kvm_lapic *apic)
@@ -603,40 +635,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 = (struct kvm_lapic *)this->private;
-	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;
 
-	KVMTRACE_1D(APIC_ACCESS, apic->vcpu, (u32)offset, handler);
+	KVMTRACE_1D(APIC_ACCESS, apic->vcpu, reg, handler);
 
-	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:
@@ -649,11 +659,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:
@@ -680,7 +696,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:
@@ -694,8 +712,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;
 
@@ -703,7 +721,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)
@@ -712,12 +730,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 = (struct kvm_lapic *)this->private;
+	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,
@@ -791,6 +851,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;
 
@@ -1092,3 +1157,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 a587f83..1a9953a 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -44,4 +44,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 98c2434..d3df59a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -800,6 +800,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 0x800 ... 0xbff:
+        return kvm_x2apic_msr_write(vcpu, msr, data);
 	case MSR_IA32_MISC_ENABLE:
 		vcpu->arch.ia32_misc_enable_msr = data;
 		break;
@@ -958,6 +960,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 0x800 ... 0xbff:
+        return kvm_x2apic_msr_read(vcpu, msr, pdata);
+        break;
 	case MSR_IA32_MISC_ENABLE:
 		data = vcpu->arch.ia32_misc_enable_msr;
 		break;
-- 
1.6.2.1

[PATCH 2/2] Advertise X2APIC support.

[Gleb Natapov]

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

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f054af1..29d730e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -252,6 +252,7 @@
 #define MSR_IA32_APICBASE               0x1b
 #define MSR_IA32_APICBASE_BSP           (1<<8)
 #define MSR_IA32_APICBASE_ENABLE        (1<<11)
+#define MSR_IA32_APICBASE_EXTD          (1<<10)
 #define MSR_IA32_APICBASE_BASE          (0xfffff<<12)
 
 #define MSR_MTRRcap			0xfe
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 23efcf4..170c3f7 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -148,7 +148,7 @@ static x86_def_t x86_defs[] = {
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
         /* this feature is needed for Solaris and isn't fully implemented */
             CPUID_PSE36,
-        .ext_features = CPUID_EXT_SSE3,
+        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_X2APIC,
         .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX |
             CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
@@ -199,7 +199,8 @@ static x86_def_t x86_defs[] = {
 	/* The original CPU also implements these ext features:
                CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_VMX, CPUID_EXT_EST,
                CPUID_EXT_TM2, CPUID_EXT_CX16, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
-        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3,
+        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
+            CPUID_EXT_X2APIC,
         .ext2_features = CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
         /* Missing: .ext3_features = CPUID_EXT3_LAHF_LM */
         .xlevel = 0x80000008,
-- 
1.6.2.1

Re: [PATCH 2/2] Advertise X2APIC support.

[Dor Laor]

Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  target-i386/cpu.h    |    1 +
>  target-i386/helper.c |    5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index f054af1..29d730e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -252,6 +252,7 @@
>  #define MSR_IA32_APICBASE               0x1b
>  #define MSR_IA32_APICBASE_BSP           (1<<8)
>  #define MSR_IA32_APICBASE_ENABLE        (1<<11)
> +#define MSR_IA32_APICBASE_EXTD          (1<<10)
>  #define MSR_IA32_APICBASE_BASE          (0xfffff<<12)
>  
>  #define MSR_MTRRcap			0xfe
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 23efcf4..170c3f7 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -148,7 +148,7 @@ static x86_def_t x86_defs[] = {
>              CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
>          /* this feature is needed for Solaris and isn't fully implemented */
>              CPUID_PSE36,
> -        .ext_features = CPUID_EXT_SSE3,
> +        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_X2APIC,
>   
Why not first test if the feature exists in the kernel?
>          .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 
>              CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX |
>              CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
> @@ -199,7 +199,8 @@ static x86_def_t x86_defs[] = {
>  	/* The original CPU also implements these ext features:
>                 CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_VMX, CPUID_EXT_EST,
>                 CPUID_EXT_TM2, CPUID_EXT_CX16, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
> -        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3,
> +        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
> +            CPUID_EXT_X2APIC,
>          .ext2_features = CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>          /* Missing: .ext3_features = CPUID_EXT3_LAHF_LM */
>          .xlevel = 0x80000008,
>   

Re: [PATCH 2/2] Advertise X2APIC support.

[Gleb Natapov]

On Sun, May 24, 2009 at 09:46:44AM +0300, Dor Laor wrote:
> Gleb Natapov wrote:
>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> ---
>>  target-i386/cpu.h    |    1 +
>>  target-i386/helper.c |    5 +++--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index f054af1..29d730e 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -252,6 +252,7 @@
>>  #define MSR_IA32_APICBASE               0x1b
>>  #define MSR_IA32_APICBASE_BSP           (1<<8)
>>  #define MSR_IA32_APICBASE_ENABLE        (1<<11)
>> +#define MSR_IA32_APICBASE_EXTD          (1<<10)
>>  #define MSR_IA32_APICBASE_BASE          (0xfffff<<12)
>>   #define MSR_MTRRcap			0xfe
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index 23efcf4..170c3f7 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -148,7 +148,7 @@ static x86_def_t x86_defs[] = {
>>              CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
>>          /* this feature is needed for Solaris and isn't fully implemented */
>>              CPUID_PSE36,
>> -        .ext_features = CPUID_EXT_SSE3,
>> +        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_X2APIC,
>>   
> Why not first test if the feature exists in the kernel?
Correct. This should be done for the final version. This patch series is more
of RFC at this stage.

--
			Gleb.

Re: [PATCH 0/1] x2apic implementation for kvm

[Sheng Yang]

On Friday 22 May 2009 01:37:53 Gleb Natapov wrote:
> This is implementation of x2apic for KVM that I wrote a while ago.
> Unfortunately there is no guest that can take advantage of it since
> Linux doesn't (yet?) use x2apic if interrupt remapping is not enabled
> and I don't feel like implement interrupt remapping device :)
>
> Re-based to latest kvm tree for your viewing pleasure and feedback.

Yeah... x2apic is for interrupt remapping, and interrupt remapping is for VT-d 
engine. So if we don't want to virtualize VT-d engine and interrupt remapping, 
x2apic is useless for the guest... And VT-d engine(and related things) 
virtualization is far from our scope now...

Thanks...

-- 
regards
Yang, Sheng

Re: [PATCH 0/1] x2apic implementation for kvm

[Gleb Natapov]

On Mon, May 25, 2009 at 02:08:26PM +0800, Sheng Yang wrote:
> On Friday 22 May 2009 01:37:53 Gleb Natapov wrote:
> > This is implementation of x2apic for KVM that I wrote a while ago.
> > Unfortunately there is no guest that can take advantage of it since
> > Linux doesn't (yet?) use x2apic if interrupt remapping is not enabled
> > and I don't feel like implement interrupt remapping device :)
> >
> > Re-based to latest kvm tree for your viewing pleasure and feedback.
> 
> Yeah... x2apic is for interrupt remapping, and interrupt remapping is for VT-d 
> engine. So if we don't want to virtualize VT-d engine and interrupt remapping, 
> x2apic is useless for the guest... And VT-d engine(and related things) 
> virtualization is far from our scope now...
> 
Can you explain why "x2apic is for interrupt remapping"? I can understand
why interrupt remapping is needed to use x2apic in certain circumstances
(apic ids > 256). x2apic has better virtualizable interface and that
is why we want to emulate it. Can you name one technical reason why it
can't be done in a context of KVM?

--
			Gleb.

Re: [PATCH 0/1] x2apic implementation for kvm

[Sheng Yang]

On Monday 25 May 2009 14:13:23 Gleb Natapov wrote:
> On Mon, May 25, 2009 at 02:08:26PM +0800, Sheng Yang wrote:
> > On Friday 22 May 2009 01:37:53 Gleb Natapov wrote:
> > > This is implementation of x2apic for KVM that I wrote a while ago.
> > > Unfortunately there is no guest that can take advantage of it since
> > > Linux doesn't (yet?) use x2apic if interrupt remapping is not enabled
> > > and I don't feel like implement interrupt remapping device :)
> > >
> > > Re-based to latest kvm tree for your viewing pleasure and feedback.
> >
> > Yeah... x2apic is for interrupt remapping, and interrupt remapping is for
> > VT-d engine. So if we don't want to virtualize VT-d engine and interrupt
> > remapping, x2apic is useless for the guest... And VT-d engine(and related
> > things) virtualization is far from our scope now...
>
> Can you explain why "x2apic is for interrupt remapping"? I can understand
> why interrupt remapping is needed to use x2apic in certain circumstances
> (apic ids > 256). x2apic has better virtualizable interface and that
> is why we want to emulate it. Can you name one technical reason why it
> can't be done in a context of KVM?

As you know, x2apic is for apic id > 256. So, at least on the real machine, if 
there is no interrupt remapping, x2apic almost can't take much advantage, 
because IOAPIC and MSI/MSI-X delivery still using 8bit apic id, so any 
external interrupt can't benefit from it(though IPI can benefit some, and 
maybe some gain from MSR rather than MMIO). And this is the main purpose for 
x2apic, that's why Linux kernel limited x2apic using with interrupt remapping.

I am not sure the things looks like in the context of KVM. I think mostly even 
you virtualize it, it can't replace lapic, for guest won't use it(I am not 
sure about the Windows). Can you explain more detail?

-- 
regards
Yang, Sheng

Re: [PATCH 0/1] x2apic implementation for kvm

[Gleb Natapov]

On Mon, May 25, 2009 at 02:30:05PM +0800, Sheng Yang wrote:
> On Monday 25 May 2009 14:13:23 Gleb Natapov wrote:
> > On Mon, May 25, 2009 at 02:08:26PM +0800, Sheng Yang wrote:
> > > On Friday 22 May 2009 01:37:53 Gleb Natapov wrote:
> > > > This is implementation of x2apic for KVM that I wrote a while ago.
> > > > Unfortunately there is no guest that can take advantage of it since
> > > > Linux doesn't (yet?) use x2apic if interrupt remapping is not enabled
> > > > and I don't feel like implement interrupt remapping device :)
> > > >
> > > > Re-based to latest kvm tree for your viewing pleasure and feedback.
> > >
> > > Yeah... x2apic is for interrupt remapping, and interrupt remapping is for
> > > VT-d engine. So if we don't want to virtualize VT-d engine and interrupt
> > > remapping, x2apic is useless for the guest... And VT-d engine(and related
> > > things) virtualization is far from our scope now...
> >
> > Can you explain why "x2apic is for interrupt remapping"? I can understand
> > why interrupt remapping is needed to use x2apic in certain circumstances
> > (apic ids > 256). x2apic has better virtualizable interface and that
> > is why we want to emulate it. Can you name one technical reason why it
> > can't be done in a context of KVM?
> 
> As you know, x2apic is for apic id > 256. So, at least on the real machine, if 
> there is no interrupt remapping, x2apic almost can't take much advantage, 
> because IOAPIC and MSI/MSI-X delivery still using 8bit apic id, so any 
> external interrupt can't benefit from it(though IPI can benefit some, and 
> maybe some gain from MSR rather than MMIO). And this is the main purpose for 
> x2apic, that's why Linux kernel limited x2apic using with interrupt remapping.
> 
> I am not sure the things looks like in the context of KVM. I think mostly even 
> you virtualize it, it can't replace lapic, for guest won't use it(I am not 
> sure about the Windows). Can you explain more detail?
KVM will expose x2apic, but will use apic ids < 256. Linux has to be
changed to use x2apic if it runs as a guest. Windows, when running as a
guest, partially paravirtualize lapic by using MSRs to access frequently
used registers. The MSR interface Windows uses is very similar to x2apic
by the way and KVM will support it too.

--
			Gleb.

Re: [PATCH 0/1] x2apic implementation for kvm

[Sheng Yang]

On Monday 25 May 2009 14:38:11 Gleb Natapov wrote:
> On Mon, May 25, 2009 at 02:30:05PM +0800, Sheng Yang wrote:
> > On Monday 25 May 2009 14:13:23 Gleb Natapov wrote:
> > > On Mon, May 25, 2009 at 02:08:26PM +0800, Sheng Yang wrote:
> > > > On Friday 22 May 2009 01:37:53 Gleb Natapov wrote:
> > > > > This is implementation of x2apic for KVM that I wrote a while ago.
> > > > > Unfortunately there is no guest that can take advantage of it since
> > > > > Linux doesn't (yet?) use x2apic if interrupt remapping is not
> > > > > enabled and I don't feel like implement interrupt remapping device
> > > > > :)
> > > > >
> > > > > Re-based to latest kvm tree for your viewing pleasure and feedback.
> > > >
> > > > Yeah... x2apic is for interrupt remapping, and interrupt remapping is
> > > > for VT-d engine. So if we don't want to virtualize VT-d engine and
> > > > interrupt remapping, x2apic is useless for the guest... And VT-d
> > > > engine(and related things) virtualization is far from our scope
> > > > now...
> > >
> > > Can you explain why "x2apic is for interrupt remapping"? I can
> > > understand why interrupt remapping is needed to use x2apic in certain
> > > circumstances (apic ids > 256). x2apic has better virtualizable
> > > interface and that is why we want to emulate it. Can you name one
> > > technical reason why it can't be done in a context of KVM?
> >
> > As you know, x2apic is for apic id > 256. So, at least on the real
> > machine, if there is no interrupt remapping, x2apic almost can't take
> > much advantage, because IOAPIC and MSI/MSI-X delivery still using 8bit
> > apic id, so any external interrupt can't benefit from it(though IPI can
> > benefit some, and maybe some gain from MSR rather than MMIO). And this is
> > the main purpose for x2apic, that's why Linux kernel limited x2apic using
> > with interrupt remapping.
> >
> > I am not sure the things looks like in the context of KVM. I think mostly
> > even you virtualize it, it can't replace lapic, for guest won't use it(I
> > am not sure about the Windows). Can you explain more detail?
>
> KVM will expose x2apic, but will use apic ids < 256. Linux has to be
> changed to use x2apic if it runs as a guest. Windows, when running as a
> guest, partially paravirtualize lapic by using MSRs to access frequently
> used registers. The MSR interface Windows uses is very similar to x2apic
> by the way and KVM will support it too.
>
OK, you are totally talking about PV. For PV, I think let host kernel accept 
the modification is more important here. (And for PV, using hypercall seems 
more directly).

-- 
regards
Yang, Sheng

Re: [PATCH 0/1] x2apic implementation for kvm

[Gleb Natapov]

On Mon, May 25, 2009 at 02:48:03PM +0800, Sheng Yang wrote:
> > > > > Yeah... x2apic is for interrupt remapping, and interrupt remapping is
> > > > > for VT-d engine. So if we don't want to virtualize VT-d engine and
> > > > > interrupt remapping, x2apic is useless for the guest... And VT-d
> > > > > engine(and related things) virtualization is far from our scope
> > > > > now...
> > > >
> > > > Can you explain why "x2apic is for interrupt remapping"? I can
> > > > understand why interrupt remapping is needed to use x2apic in certain
> > > > circumstances (apic ids > 256). x2apic has better virtualizable
> > > > interface and that is why we want to emulate it. Can you name one
> > > > technical reason why it can't be done in a context of KVM?
> > >
> > > As you know, x2apic is for apic id > 256. So, at least on the real
> > > machine, if there is no interrupt remapping, x2apic almost can't take
> > > much advantage, because IOAPIC and MSI/MSI-X delivery still using 8bit
> > > apic id, so any external interrupt can't benefit from it(though IPI can
> > > benefit some, and maybe some gain from MSR rather than MMIO). And this is
> > > the main purpose for x2apic, that's why Linux kernel limited x2apic using
> > > with interrupt remapping.
> > >
> > > I am not sure the things looks like in the context of KVM. I think mostly
> > > even you virtualize it, it can't replace lapic, for guest won't use it(I
> > > am not sure about the Windows). Can you explain more detail?
> >
> > KVM will expose x2apic, but will use apic ids < 256. Linux has to be
> > changed to use x2apic if it runs as a guest. Windows, when running as a
> > guest, partially paravirtualize lapic by using MSRs to access frequently
> > used registers. The MSR interface Windows uses is very similar to x2apic
> > by the way and KVM will support it too.
> >
> OK, you are totally talking about PV. For PV, I think let host kernel accept 
> the modification is more important here. (And for PV, using hypercall seems 
> more directly).
> 
I don't think hypercall will have performance advantage over MSR
intercept and using code that is already in kernel is better than
writing more code. Also Intel is better in defining interfaces that
we are ;)

--
			Gleb.

Re: [PATCH 0/1] x2apic implementation for kvm

[Avi Kivity]

Sheng Yang wrote:
> OK, you are totally talking about PV. For PV, I think let host kernel accept 
> the modification is more important here. (And for PV, using hypercall seems 
> more directly).
>   

Microsoft already defined their interfaces, and they use MSRs (but a 
different range from x2apic).

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

Re: [PATCH 0/1] x2apic implementation for kvm

[Sheng Yang]

On Monday 25 May 2009 17:07:33 Avi Kivity wrote:
> Sheng Yang wrote:
> > OK, you are totally talking about PV. For PV, I think let host kernel
> > accept the modification is more important here. (And for PV, using
> > hypercall seems more directly).
>
> Microsoft already defined their interfaces, and they use MSRs (but a
> different range from x2apic).

I think that means the PV interface for lapic. And yes, we can support it 
follow MS's interface, but x2apic still seems another story as you noted... I 
still don't think support x2apic here would bring us more benefits.

-- 
regards
Yang, Sheng

Re: [PATCH 0/1] x2apic implementation for kvm

[Avi Kivity]

Sheng Yang wrote:
> I think that means the PV interface for lapic. And yes, we can support it 
> follow MS's interface, but x2apic still seems another story as you noted... I 
> still don't think support x2apic here would bring us more benefits.
>   

x2apic has the following benefit:

- msr exits are faster than mmio (no page table walk, emulation)
- no need to read back ICR to look at the busy bit
- one ICR write instead of two
- potential to support large guests once we add interrupt remapping
- shared code with the Hyper-V paravirt interface

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

RE: [PATCH 0/1] x2apic implementation for kvm

[Dong, Eddie]

Avi Kivity wrote:
> Sheng Yang wrote:
>> I think that means the PV interface for lapic. And yes, we can
>> support it follow MS's interface, but x2apic still seems another
>> story as you noted... I still don't think support x2apic here would
>> bring us more benefits. 
>> 
> 
> x2apic has the following benefit:
> 
> - msr exits are faster than mmio (no page table walk, emulation)
> - no need to read back ICR to look at the busy bit
> - one ICR write instead of two
> - potential to support large guests once we add interrupt remapping
> - shared code with the Hyper-V paravirt interface
> 
Is there any plan to implement an PV irqchip such as Xenirqchip for KVM?

Re: [PATCH 0/1] x2apic implementation for kvm

[Avi Kivity]

Dong, Eddie wrote:
>> x2apic has the following benefit:
>>
>> - msr exits are faster than mmio (no page table walk, emulation)
>> - no need to read back ICR to look at the busy bit
>> - one ICR write instead of two
>> - potential to support large guests once we add interrupt remapping
>> - shared code with the Hyper-V paravirt interface
>>
>>     
> Is there any plan to implement an PV irqchip such as Xenirqchip for KVM?

No.  PV irqchips (and PV in general) have the following drawbacks:

- need to define and maintain an ABI
- only works on newer Linux guests
- obsoleted when hardware improves
- increase code size and maintenance effort
- have problems during transitions (boot, kexec)
- don't integrate well with device assignment
- require effort outside the kvm codebase

If a significant performance benefit can be demonstrated, I'll consider 
it, but until then my preference is full virtualization augmented by 
optional, targeted pv assists (like the TPR patching).

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

Re: [PATCH 0/1] x2apic implementation for kvm

[Sheng Yang]

On Monday 25 May 2009 17:22:34 Avi Kivity wrote:
> Sheng Yang wrote:
> > I think that means the PV interface for lapic. And yes, we can support it
> > follow MS's interface, but x2apic still seems another story as you
> > noted... I still don't think support x2apic here would bring us more
> > benefits.
>
> x2apic has the following benefit:
>
> - msr exits are faster than mmio (no page table walk, emulation)

Need PV(at least part of). I don't think Hyper-V considered this, and not sure 
the community's aptitude.

> - no need to read back ICR to look at the busy bit
> - one ICR write instead of two

Maybe the key issue.

> - potential to support large guests once we add interrupt remapping

Then it can be added before we have it. Compared to the workload, x2apic is 
not the problem, interrupt remapping/VT-d is. 

> - shared code with the Hyper-V paravirt interface

So I think the key thing are ICR related(and seems no data available 
currently). Compare the benefit of ICR improve(can it improved in another way? 
Does Hyper-V interface has related things?), and the workload of x2apic 
virtualization as well as guest OS support, well, I don't know, but not 
optimistic.

-- 
regards
Yang, Sheng

Re: [PATCH 0/1] x2apic implementation for kvm

[Avi Kivity]

Sheng Yang wrote:
> On Monday 25 May 2009 17:22:34 Avi Kivity wrote:
>   
>> Sheng Yang wrote:
>>     
>>> I think that means the PV interface for lapic. And yes, we can support it
>>> follow MS's interface, but x2apic still seems another story as you
>>> noted... I still don't think support x2apic here would bring us more
>>> benefits.
>>>       
>> x2apic has the following benefit:
>>
>> - msr exits are faster than mmio (no page table walk, emulation)
>>     
>
> Need PV(at least part of). I don't think Hyper-V considered this, and not sure 
> the community's aptitude.
>   

Hyper-V does define MSRs for local apic access, as far as I can tell 
they're identical to x2apic except for the msr index.

>> - potential to support large guests once we add interrupt remapping
>>     
>
> Then it can be added before we have it. Compared to the workload, x2apic is 
> not the problem, interrupt remapping/VT-d is. 
>   

I'd like to have the benefit sooner.  x2apic provides two user-visible 
benefits: performance and large guests.  I don't want performance to 
wait for large guests.

>> - shared code with the Hyper-V paravirt interface
>>     
>
> So I think the key thing are ICR related(and seems no data available 
> currently). Compare the benefit of ICR improve(can it improved in another way? 
> Does Hyper-V interface has related things?), and the workload of x2apic 
> virtualization as well as guest OS support, well, I don't know, but not 
> optimistic

x2apic, without interrupt remapping, is fairly simple.

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

Re: [PATCH 1/1] x2apic interface to lapic

[Avi Kivity]

Gleb Natapov wrote:
>  static unsigned int apic_lvt_mask[APIC_LVT_NUM] = {
>  	LVT_MASK | APIC_LVT_TIMER_PERIODIC,	/* LVTT */
>  	LVT_MASK | APIC_MODE_MASK,	/* LVTTHMR */
> @@ -251,7 +257,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 & 0xffff);
> +	}
>   

!! unnecessary.  And one of the (cast) and (& 0xffff) is unnecessary.

>  
>  	logical_id = GET_APIC_LOGICAL_ID(apic_get_reg(apic, APIC_LDR));
>  
> @@ -440,7 +451,8 @@ 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);
> +	irq.dest_id = apic_x2apic_mode(apic) ? icr_high :
> +		GET_APIC_DEST_FIELD(icr_high);
>   

Please replace the ?: (here and elsewhere) with explicit if statements.  
?: is unreadable when split over two lines like this.

(I find it more readable to do

    blah = predicate
           ? iftrue
           : iffalse;

but that's almost the same as an if)

>  
> -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 = (struct kvm_lapic *)this->private;
> -	unsigned int offset = address - apic->base_address;
>  	unsigned char alignment = offset & 0xf;
>  	u32 result;
> +	static const uint64_t rmask = 0x43ff01ffffffe70c;
>   

Wow.  A comment perhaps?

>  
>  	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 (!(rmask & (1ULL << (offset >> 4)))) {
> +		printk(KERN_ERR "KVM_APIC_READ: read reserved register %x\n",
> +				offset);
> +		return 1;
> +	}
> +
>   

(offset >> 4) can still be 255, yielding unspecified results for the shift.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 98c2434..d3df59a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -800,6 +800,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 0x800 ... 0xbff:
> +        return kvm_x2apic_msr_write(vcpu, msr, data);
>  	case MSR_IA32_MISC_ENABLE:
>  		vcpu->arch.ia32_misc_enable_msr = data;
>  		break;
> @@ -958,6 +960,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 0x800 ... 0xbff:
> +        return kvm_x2apic_msr_read(vcpu, msr, pdata);
> +        break;
>  	case MSR_IA32_MISC_ENABLE:
>  		data = vcpu->arch.ia32_misc_enable_msr;
>  		break;
>   

Whitespace...

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

Re: [PATCH 2/2] Advertise X2APIC support.

[Avi Kivity]

Gleb Natapov wrote:
>  
>  #define MSR_MTRRcap			0xfe
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 23efcf4..170c3f7 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -148,7 +148,7 @@ static x86_def_t x86_defs[] = {
>              CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
>          /* this feature is needed for Solaris and isn't fully implemented */
>              CPUID_PSE36,
> -        .ext_features = CPUID_EXT_SSE3,
> +        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_X2APIC,
>          .ext2_features = (PPRO_FEATURES & 0x0183F3FF) | 
>              CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX |
>              CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
> @@ -199,7 +199,8 @@ static x86_def_t x86_defs[] = {
>  	/* The original CPU also implements these ext features:
>                 CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_VMX, CPUID_EXT_EST,
>                 CPUID_EXT_TM2, CPUID_EXT_CX16, CPUID_EXT_XTPR, CPUID_EXT_PDCM */
> -        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3,
> +        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
> +            CPUID_EXT_X2APIC,
>          .ext2_features = CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>          /* Missing: .ext3_features = CPUID_EXT3_LAHF_LM */
>          .xlevel = 0x80000008,
>   

x2apic is not present on most cpus, so it should not be available by 
default.  Users can enable it with -cpu +x2apic


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