diff mbox

[1/1] x2apic interface to lapic

Message ID 1242927475-6140-2-git-send-email-gleb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov May 21, 2009, 5:37 p.m. UTC 
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(-)

Comments

Avi Kivity May 31, 2009, 12:44 p.m. UTC | #1 
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...
diff mbox

Patch

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;