[PATCH 00/20] KVM updates for 2.6.23, part 2

[PATCH 00/20] KVM updates for 2.6.23, part 2

[Avi Kivity]

This is the second set of updates for the 2.6.23 merge window.  This time
around, the patchset includes the cpu hotplug fixes (which also make
suspend and resume robust in the presence of running virtual machines). Also
included are a few fixes and core updates.

Avi Kivity (11):
      KVM: VMX: Reinitialize the real-mode tss when entering real mode
      KVM: VMX: Remove unnecessary code in vmx_tlb_flush()
      KVM: Remove kvmfs in favor of the anonymous inodes source
      KVM: Clean up #includes
      HOTPLUG: Add CPU_DYING notifier
      HOTPLUG: Adapt cpuset hotplug callback to CPU_DYING
      HOTPLUG: Adapt thermal throttle to CPU_DYING
      SMP: Implement on_cpu()
      KVM: Keep track of which cpus have virtualization enabled
      KVM: Tune hotplug/suspend IPIs
      KVM: Use CPU_DYING for disabling virtualization

Eddie Dong (1):
      KVM: Add support for in-kernel pio handlers

Gregory Haskins (2):
      KVM: Adds support for in-kernel mmio handlers
      KVM: VMX: Fix interrupt checking on lightweight exit

Joerg Roedel (1):
      KVM: SVM: Reliably detect if SVM was disabled by BIOS

Luca Tettamanti (2):
      KVM: Fix x86 emulator writeback
      KVM: Avoid useless memory write when possible

Nitin A Kamble (2):
      KVM: Implement emulation of "pop reg" instruction (opcode 0x58-0x5f)
      KVM: Implement emulation of instruction "ret" (opcode 0xc3)

Shaohua Li (1):
      KVM: MMU: Fix Wrong tlb flush order

[PATCH 01/20] KVM: Implement emulation of "pop reg" instruction (opcode 0x58-0x5f)

[Avi Kivity]

From: Nitin A Kamble <nitin.a.kamble@intel.com>

For use in real mode.

Signed-off-by: Nitin A Kamble <nitin.a.kamble@intel.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/x86_emulate.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index a4a8481..46c3806 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -98,8 +98,11 @@ static u8 opcode_table[256] = {
 	0, 0, 0, 0,
 	/* 0x40 - 0x4F */
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-	/* 0x50 - 0x5F */
-	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	/* 0x50 - 0x57 */
+	0, 0, 0, 0, 0, 0, 0, 0,
+	/* 0x58 - 0x5F */
+	ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+	ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
 	/* 0x60 - 0x6F */
 	0, 0, 0, DstReg | SrcMem32 | ModRM | Mov /* movsxd (x86/64) */ ,
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
@@ -1153,6 +1156,16 @@ special_insn:
 	case 0xf4:              /* hlt */
 		ctxt->vcpu->halt_request = 1;
 		goto done;
+	case 0x58 ... 0x5f: /* pop reg */
+		dst.ptr = (unsigned long *)&_regs[b & 0x7];
+
+		if ((rc = ops->read_std(register_address(ctxt->ss_base,
+			_regs[VCPU_REGS_RSP]), dst.ptr, op_bytes, ctxt)) != 0)
+			goto done;
+
+		register_address_increment(_regs[VCPU_REGS_RSP], dst.bytes);
+		dst.orig_val = dst.val; /* Disable writeback. */
+		break;
 	}
 	goto writeback;
 
-- 
1.5.2.2

[PATCH 02/20] KVM: Implement emulation of instruction "ret" (opcode 0xc3)

[Avi Kivity]

From: Nitin A Kamble <nitin.a.kamble@intel.com>

Signed-off-by: Nitin A Kamble <nitin.a.kamble@intel.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/x86_emulate.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 46c3806..92620e4 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -131,9 +131,9 @@ static u8 opcode_table[256] = {
 	/* 0xB0 - 0xBF */
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 	/* 0xC0 - 0xC7 */
-	ByteOp | DstMem | SrcImm | ModRM, DstMem | SrcImmByte | ModRM, 0, 0,
-	0, 0, ByteOp | DstMem | SrcImm | ModRM | Mov,
-	    DstMem | SrcImm | ModRM | Mov,
+	ByteOp | DstMem | SrcImm | ModRM, DstMem | SrcImmByte | ModRM,
+	0, ImplicitOps, 0, 0,
+	ByteOp | DstMem | SrcImm | ModRM | Mov, DstMem | SrcImm | ModRM | Mov,
 	/* 0xC8 - 0xCF */
 	0, 0, 0, 0, 0, 0, 0, 0,
 	/* 0xD0 - 0xD7 */
@@ -1156,14 +1156,18 @@ special_insn:
 	case 0xf4:              /* hlt */
 		ctxt->vcpu->halt_request = 1;
 		goto done;
+	case 0xc3: /* ret */
+		dst.ptr = &_eip;
+		goto pop_instruction;
 	case 0x58 ... 0x5f: /* pop reg */
 		dst.ptr = (unsigned long *)&_regs[b & 0x7];
 
+pop_instruction:
 		if ((rc = ops->read_std(register_address(ctxt->ss_base,
 			_regs[VCPU_REGS_RSP]), dst.ptr, op_bytes, ctxt)) != 0)
 			goto done;
 
-		register_address_increment(_regs[VCPU_REGS_RSP], dst.bytes);
+		register_address_increment(_regs[VCPU_REGS_RSP], op_bytes);
 		dst.orig_val = dst.val; /* Disable writeback. */
 		break;
 	}
-- 
1.5.2.2

[PATCH 03/20] KVM: Adds support for in-kernel mmio handlers

[Avi Kivity]

From: Gregory Haskins <ghaskins@novell.com>

Signed-off-by: Gregory Haskins <ghaskins@novell.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm.h      |   60 ++++++++++++++++++++++++++++++
 drivers/kvm/kvm_main.c |   94 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 142 insertions(+), 12 deletions(-)

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index b08272b..31846b1 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -265,6 +265,65 @@ struct kvm_stat {
 	u32 efer_reload;
 };
 
+struct kvm_io_device {
+	void (*read)(struct kvm_io_device *this,
+		     gpa_t addr,
+		     int len,
+		     void *val);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      int len,
+		      const void *val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+	void (*destructor)(struct kvm_io_device *this);
+
+	void             *private;
+};
+
+static inline void kvm_iodevice_read(struct kvm_io_device *dev,
+				     gpa_t addr,
+				     int len,
+				     void *val)
+{
+	dev->read(dev, addr, len, val);
+}
+
+static inline void kvm_iodevice_write(struct kvm_io_device *dev,
+				      gpa_t addr,
+				      int len,
+				      const void *val)
+{
+	dev->write(dev, addr, len, val);
+}
+
+static inline int kvm_iodevice_inrange(struct kvm_io_device *dev, gpa_t addr)
+{
+	return dev->in_range(dev, addr);
+}
+
+static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
+{
+	dev->destructor(dev);
+}
+
+/*
+ * It would be nice to use something smarter than a linear search, TBD...
+ * Thankfully we dont expect many devices to register (famous last words :),
+ * so until then it will suffice.  At least its abstracted so we can change
+ * in one place.
+ */
+struct kvm_io_bus {
+	int                   dev_count;
+#define NR_IOBUS_DEVS 6
+	struct kvm_io_device *devs[NR_IOBUS_DEVS];
+};
+
+void kvm_io_bus_init(struct kvm_io_bus *bus);
+void kvm_io_bus_destroy(struct kvm_io_bus *bus);
+struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr);
+void kvm_io_bus_register_dev(struct kvm_io_bus *bus,
+			     struct kvm_io_device *dev);
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {
@@ -393,6 +452,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct descriptor_table {
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 633c2ed..e157e28 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -366,6 +366,7 @@ static struct kvm *kvm_create_vm(void)
 	spin_lock(&kvm_lock);
 	list_add(&kvm->vm_list, &vm_list);
 	spin_unlock(&kvm_lock);
+	kvm_io_bus_init(&kvm->mmio_bus);
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		struct kvm_vcpu *vcpu = &kvm->vcpus[i];
 
@@ -474,6 +475,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	spin_lock(&kvm_lock);
 	list_del(&kvm->vm_list);
 	spin_unlock(&kvm_lock);
+	kvm_io_bus_destroy(&kvm->mmio_bus);
 	kvm_free_vcpus(kvm);
 	kvm_free_physmem(kvm);
 	kfree(kvm);
@@ -1097,12 +1099,25 @@ static int emulator_write_std(unsigned long addr,
 	return X86EMUL_UNHANDLEABLE;
 }
 
+static struct kvm_io_device *vcpu_find_mmio_dev(struct kvm_vcpu *vcpu,
+						gpa_t addr)
+{
+	/*
+	 * Note that its important to have this wrapper function because
+	 * in the very near future we will be checking for MMIOs against
+	 * the LAPIC as well as the general MMIO bus
+	 */
+	return kvm_io_bus_find_dev(&vcpu->kvm->mmio_bus, addr);
+}
+
 static int emulator_read_emulated(unsigned long addr,
 				  void *val,
 				  unsigned int bytes,
 				  struct x86_emulate_ctxt *ctxt)
 {
-	struct kvm_vcpu *vcpu = ctxt->vcpu;
+	struct kvm_vcpu      *vcpu = ctxt->vcpu;
+	struct kvm_io_device *mmio_dev;
+	gpa_t                 gpa;
 
 	if (vcpu->mmio_read_completed) {
 		memcpy(val, vcpu->mmio_data, bytes);
@@ -1111,18 +1126,26 @@ static int emulator_read_emulated(unsigned long addr,
 	} else if (emulator_read_std(addr, val, bytes, ctxt)
 		   == X86EMUL_CONTINUE)
 		return X86EMUL_CONTINUE;
-	else {
-		gpa_t gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
 
-		if (gpa == UNMAPPED_GVA)
-			return X86EMUL_PROPAGATE_FAULT;
-		vcpu->mmio_needed = 1;
-		vcpu->mmio_phys_addr = gpa;
-		vcpu->mmio_size = bytes;
-		vcpu->mmio_is_write = 0;
+	gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
+	if (gpa == UNMAPPED_GVA)
+		return X86EMUL_PROPAGATE_FAULT;
 
-		return X86EMUL_UNHANDLEABLE;
+	/*
+	 * Is this MMIO handled locally?
+	 */
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if (mmio_dev) {
+		kvm_iodevice_read(mmio_dev, gpa, bytes, val);
+		return X86EMUL_CONTINUE;
 	}
+
+	vcpu->mmio_needed = 1;
+	vcpu->mmio_phys_addr = gpa;
+	vcpu->mmio_size = bytes;
+	vcpu->mmio_is_write = 0;
+
+	return X86EMUL_UNHANDLEABLE;
 }
 
 static int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
@@ -1150,8 +1173,9 @@ static int emulator_write_emulated(unsigned long addr,
 				   unsigned int bytes,
 				   struct x86_emulate_ctxt *ctxt)
 {
-	struct kvm_vcpu *vcpu = ctxt->vcpu;
-	gpa_t gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
+	struct kvm_vcpu      *vcpu = ctxt->vcpu;
+	struct kvm_io_device *mmio_dev;
+	gpa_t                 gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
 
 	if (gpa == UNMAPPED_GVA) {
 		kvm_arch_ops->inject_page_fault(vcpu, addr, 2);
@@ -1161,6 +1185,15 @@ static int emulator_write_emulated(unsigned long addr,
 	if (emulator_write_phys(vcpu, gpa, val, bytes))
 		return X86EMUL_CONTINUE;
 
+	/*
+	 * Is this MMIO handled locally?
+	 */
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if (mmio_dev) {
+		kvm_iodevice_write(mmio_dev, gpa, bytes, val);
+		return X86EMUL_CONTINUE;
+	}
+
 	vcpu->mmio_needed = 1;
 	vcpu->mmio_phys_addr = gpa;
 	vcpu->mmio_size = bytes;
@@ -3031,6 +3064,43 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 	return NOTIFY_OK;
 }
 
+void kvm_io_bus_init(struct kvm_io_bus *bus)
+{
+	memset(bus, 0, sizeof(*bus));
+}
+
+void kvm_io_bus_destroy(struct kvm_io_bus *bus)
+{
+	int i;
+
+	for (i = 0; i < bus->dev_count; i++) {
+		struct kvm_io_device *pos = bus->devs[i];
+
+		kvm_iodevice_destructor(pos);
+	}
+}
+
+struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	int i;
+
+	for (i = 0; i < bus->dev_count; i++) {
+		struct kvm_io_device *pos = bus->devs[i];
+
+		if (pos->in_range(pos, addr))
+			return pos;
+	}
+
+	return NULL;
+}
+
+void kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	BUG_ON(bus->dev_count > (NR_IOBUS_DEVS-1));
+
+	bus->devs[bus->dev_count++] = dev;
+}
+
 static struct notifier_block kvm_cpu_notifier = {
 	.notifier_call = kvm_cpu_hotplug,
 	.priority = 20, /* must be > scheduler priority */
-- 
1.5.2.2

[PATCH 04/20] KVM: VMX: Fix interrupt checking on lightweight exit

[Avi Kivity]

From: Gregory Haskins <ghaskins@novell.com>

With kernel-injected interrupts, we need to check for interrupts on
lightweight exits too.

Signed-off-by: Gregory Haskins <ghaskins@novell.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/vmx.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index d06c362..b47ddcc 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -1992,13 +1992,13 @@ static int vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	int r;
 
 preempted:
-	if (!vcpu->mmio_read_completed)
-		do_interrupt_requests(vcpu, kvm_run);
-
 	if (vcpu->guest_debug.enabled)
 		kvm_guest_debug_pre(vcpu);
 
 again:
+	if (!vcpu->mmio_read_completed)
+		do_interrupt_requests(vcpu, kvm_run);
+
 	vmx_save_host_state(vcpu);
 	kvm_load_guest_fpu(vcpu);
 
-- 
1.5.2.2

[PATCH 05/20] KVM: Add support for in-kernel pio handlers

[Avi Kivity]

From: Eddie Dong <eddie.dong@intel.com>

Useful for the PIC and PIT.

Signed-off-by: Yaozu (Eddie) Dong <eddie.dong@intel.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm.h      |    5 ++++-
 drivers/kvm/kvm_main.c |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index 31846b1..a7c5e6b 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -241,6 +241,7 @@ struct kvm_pio_request {
 	struct page *guest_pages[2];
 	unsigned guest_page_offset;
 	int in;
+	int port;
 	int size;
 	int string;
 	int down;
@@ -303,7 +304,8 @@ static inline int kvm_iodevice_inrange(struct kvm_io_device *dev, gpa_t addr)
 
 static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
 {
-	dev->destructor(dev);
+	if (dev->destructor)
+		dev->destructor(dev);
 }
 
 /*
@@ -453,6 +455,7 @@ struct kvm {
 	struct list_head vm_list;
 	struct file *filp;
 	struct kvm_io_bus mmio_bus;
+	struct kvm_io_bus pio_bus;
 };
 
 struct descriptor_table {
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index e157e28..7826f16 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -361,6 +361,7 @@ static struct kvm *kvm_create_vm(void)
 	if (!kvm)
 		return ERR_PTR(-ENOMEM);
 
+	kvm_io_bus_init(&kvm->pio_bus);
 	spin_lock_init(&kvm->lock);
 	INIT_LIST_HEAD(&kvm->active_mmu_pages);
 	spin_lock(&kvm_lock);
@@ -475,6 +476,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	spin_lock(&kvm_lock);
 	list_del(&kvm->vm_list);
 	spin_unlock(&kvm_lock);
+	kvm_io_bus_destroy(&kvm->pio_bus);
 	kvm_io_bus_destroy(&kvm->mmio_bus);
 	kvm_free_vcpus(kvm);
 	kvm_free_physmem(kvm);
@@ -1110,6 +1112,12 @@ static struct kvm_io_device *vcpu_find_mmio_dev(struct kvm_vcpu *vcpu,
 	return kvm_io_bus_find_dev(&vcpu->kvm->mmio_bus, addr);
 }
 
+static struct kvm_io_device *vcpu_find_pio_dev(struct kvm_vcpu *vcpu,
+					       gpa_t addr)
+{
+	return kvm_io_bus_find_dev(&vcpu->kvm->pio_bus, addr);
+}
+
 static int emulator_read_emulated(unsigned long addr,
 				  void *val,
 				  unsigned int bytes,
@@ -1832,6 +1840,20 @@ static int complete_pio(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+void kernel_pio(struct kvm_io_device *pio_dev, struct kvm_vcpu *vcpu)
+{
+	/* TODO: String I/O for in kernel device */
+
+	if (vcpu->pio.in)
+		kvm_iodevice_read(pio_dev, vcpu->pio.port,
+				  vcpu->pio.size,
+				  vcpu->pio_data);
+	else
+		kvm_iodevice_write(pio_dev, vcpu->pio.port,
+				   vcpu->pio.size,
+				   vcpu->pio_data);
+}
+
 int kvm_setup_pio(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 		  int size, unsigned long count, int string, int down,
 		  gva_t address, int rep, unsigned port)
@@ -1840,6 +1862,7 @@ int kvm_setup_pio(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 	int i;
 	int nr_pages = 1;
 	struct page *page;
+	struct kvm_io_device *pio_dev;
 
 	vcpu->run->exit_reason = KVM_EXIT_IO;
 	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
@@ -1851,17 +1874,27 @@ int kvm_setup_pio(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 	vcpu->pio.cur_count = count;
 	vcpu->pio.size = size;
 	vcpu->pio.in = in;
+	vcpu->pio.port = port;
 	vcpu->pio.string = string;
 	vcpu->pio.down = down;
 	vcpu->pio.guest_page_offset = offset_in_page(address);
 	vcpu->pio.rep = rep;
 
+	pio_dev = vcpu_find_pio_dev(vcpu, port);
 	if (!string) {
 		kvm_arch_ops->cache_regs(vcpu);
 		memcpy(vcpu->pio_data, &vcpu->regs[VCPU_REGS_RAX], 4);
 		kvm_arch_ops->decache_regs(vcpu);
+		if (pio_dev) {
+			kernel_pio(pio_dev, vcpu);
+			complete_pio(vcpu);
+			return 1;
+		}
 		return 0;
 	}
+	/* TODO: String I/O for in kernel device */
+	if (pio_dev)
+		printk(KERN_ERR "kvm_setup_pio: no string io support\n");
 
 	if (!count) {
 		kvm_arch_ops->skip_emulated_instruction(vcpu);
-- 
1.5.2.2

[PATCH 06/20] KVM: Fix x86 emulator writeback

[Avi Kivity]

From: Luca Tettamanti <kronos.it@gmail.com>

When the old value and new one are the same the emulator skips the
write; this is undesirable when the destination is a MMIO area and the
write shall be performed regardless of the previous value. This
optimization breaks e.g. a Linux guest APIC compiled without
X86_GOOD_APIC.

Remove the check and perform the writeback stage in the emulation unless
it's explicitly disabled (currently push and some 2 bytes instructions
may disable the writeback).

Signed-Off-By: Luca Tettamanti <kronos.it@gmail.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/x86_emulate.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 92620e4..f60012d 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -485,6 +485,7 @@ x86_emulate_memop(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	int mode = ctxt->mode;
 	unsigned long modrm_ea;
 	int use_modrm_ea, index_reg = 0, base_reg = 0, scale, rip_relative = 0;
+	int no_wb = 0;
 
 	/* Shadow copy of register state. Committed on successful emulation. */
 	unsigned long _regs[NR_VCPU_REGS];
@@ -1051,7 +1052,7 @@ done_prefixes:
 						      _regs[VCPU_REGS_RSP]),
 				     &dst.val, dst.bytes, ctxt)) != 0)
 				goto done;
-			dst.val = dst.orig_val;	/* skanky: disable writeback */
+			no_wb = 1;
 			break;
 		default:
 			goto cannot_emulate;
@@ -1060,7 +1061,7 @@ done_prefixes:
 	}
 
 writeback:
-	if ((d & Mov) || (dst.orig_val != dst.val)) {
+	if (!no_wb) {
 		switch (dst.type) {
 		case OP_REG:
 			/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
@@ -1168,7 +1169,7 @@ pop_instruction:
 			goto done;
 
 		register_address_increment(_regs[VCPU_REGS_RSP], op_bytes);
-		dst.orig_val = dst.val; /* Disable writeback. */
+		no_wb = 1; /* Disable writeback. */
 		break;
 	}
 	goto writeback;
@@ -1323,7 +1324,7 @@ twobyte_insn:
 
 twobyte_special_insn:
 	/* Disable writeback. */
-	dst.orig_val = dst.val;
+	no_wb = 1;
 	switch (b) {
 	case 0x09:		/* wbinvd */
 		break;
-- 
1.5.2.2

[PATCH 07/20] KVM: Avoid useless memory write when possible

[Avi Kivity]

From: Luca Tettamanti <kronos.it@gmail.com>

When writing to normal memory and the memory area is unchanged the write
can be safely skipped, avoiding the costly kvm_mmu_pte_write.

Signed-Off-By: Luca Tettamanti <kronos.it@gmail.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm_main.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 7826f16..5603000 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -1170,8 +1170,10 @@ static int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
 		return 0;
 	mark_page_dirty(vcpu->kvm, gpa >> PAGE_SHIFT);
 	virt = kmap_atomic(page, KM_USER0);
-	kvm_mmu_pte_write(vcpu, gpa, virt + offset, val, bytes);
-	memcpy(virt + offset_in_page(gpa), val, bytes);
+	if (memcmp(virt + offset_in_page(gpa), val, bytes)) {
+		kvm_mmu_pte_write(vcpu, gpa, virt + offset, val, bytes);
+		memcpy(virt + offset_in_page(gpa), val, bytes);
+	}
 	kunmap_atomic(virt, KM_USER0);
 	return 1;
 }
-- 
1.5.2.2

[PATCH 08/20] KVM: VMX: Reinitialize the real-mode tss when entering real mode

[Avi Kivity]

Protected mode code may have corrupted the real-mode tss, so re-initialize
it when switching to real mode.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/vmx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index b47ddcc..42a9163 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -31,6 +31,8 @@
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
+static int init_rmode_tss(struct kvm *kvm);
+
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
 
@@ -951,6 +953,8 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
 	fix_rmode_seg(VCPU_SREG_DS, &vcpu->rmode.ds);
 	fix_rmode_seg(VCPU_SREG_GS, &vcpu->rmode.gs);
 	fix_rmode_seg(VCPU_SREG_FS, &vcpu->rmode.fs);
+
+	init_rmode_tss(vcpu->kvm);
 }
 
 #ifdef CONFIG_X86_64
-- 
1.5.2.2

[PATCH 09/20] KVM: MMU: Fix Wrong tlb flush order

[Avi Kivity]

From: Shaohua Li <shaohua.li@intel.com>

Need to flush the tlb after updating a pte, not before.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/mmu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index ad50cfd..49ffbd3 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -441,8 +441,8 @@ static void rmap_write_protect(struct kvm_vcpu *vcpu, u64 gfn)
 		BUG_ON(!(*spte & PT_WRITABLE_MASK));
 		rmap_printk("rmap_write_protect: spte %p %llx\n", spte, *spte);
 		rmap_remove(vcpu, spte);
-		kvm_flush_remote_tlbs(vcpu->kvm);
 		set_shadow_pte(spte, *spte & ~PT_WRITABLE_MASK);
+		kvm_flush_remote_tlbs(vcpu->kvm);
 	}
 }
 
-- 
1.5.2.2

Re: [PATCH 09/20] KVM: MMU: Fix Wrong tlb flush order

[Ingo Molnar]

* Avi Kivity <avi@qumranet.com> wrote:

> From: Shaohua Li <shaohua.li@intel.com>
> 
> Need to flush the tlb after updating a pte, not before.

>  		rmap_remove(vcpu, spte);
> -		kvm_flush_remote_tlbs(vcpu->kvm);
>  		set_shadow_pte(spte, *spte & ~PT_WRITABLE_MASK);
> +		kvm_flush_remote_tlbs(vcpu->kvm);

hm, isnt this a must-have fix for v2.6.22 (and .21) as well?

	Ingo

Re: [PATCH 09/20] KVM: MMU: Fix Wrong tlb flush order

[Avi Kivity]

Ingo Molnar wrote:
> * Avi Kivity <avi@qumranet.com> wrote:
>
>   
>> From: Shaohua Li <shaohua.li@intel.com>
>>
>> Need to flush the tlb after updating a pte, not before.
>>     
>
>   
>>  		rmap_remove(vcpu, spte);
>> -		kvm_flush_remote_tlbs(vcpu->kvm);
>>  		set_shadow_pte(spte, *spte & ~PT_WRITABLE_MASK);
>> +		kvm_flush_remote_tlbs(vcpu->kvm);
>>     
>
> hm, isnt this a must-have fix for v2.6.22 (and .21) as well?
>   

Guest SMP support is only enabled by another patch in this patchset.


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

[PATCH 10/20] KVM: VMX: Remove unnecessary code in vmx_tlb_flush()

[Avi Kivity]

A vmexit implicitly flushes the tlb; the code is bogus.

Noted by Shaohua Li.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/vmx.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index 42a9163..7d04ffa 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -1987,7 +1987,6 @@ static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu,
 
 static void vmx_flush_tlb(struct kvm_vcpu *vcpu)
 {
-	vmcs_writel(GUEST_CR3, vmcs_readl(GUEST_CR3));
 }
 
 static int vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
-- 
1.5.2.2

[PATCH 11/20] KVM: SVM: Reliably detect if SVM was disabled by BIOS

[Avi Kivity]

From: Joerg Roedel <joerg.roedel@amd.com>

This patch adds an implementation to the svm is_disabled function to
detect reliably if the BIOS disabled the SVM feature in the CPU. This
fixes the issues with kernel panics when loading the kvm-amd module on
machines where SVM is available but disabled.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/svm.c |    6 ++++++
 drivers/kvm/svm.h |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index 62ec38c..a0d4428 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -1735,6 +1735,12 @@ static void svm_inject_page_fault(struct kvm_vcpu *vcpu,
 
 static int is_disabled(void)
 {
+	u64 vm_cr;
+
+	rdmsrl(MSR_VM_CR, vm_cr);
+	if (vm_cr & (1 << SVM_VM_CR_SVM_DISABLE))
+		return 1;
+
 	return 0;
 }
 
diff --git a/drivers/kvm/svm.h b/drivers/kvm/svm.h
index 5e93814..3b1b0f3 100644
--- a/drivers/kvm/svm.h
+++ b/drivers/kvm/svm.h
@@ -175,8 +175,11 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_CPUID_FUNC 0x8000000a
 
 #define MSR_EFER_SVME_MASK (1ULL << 12)
+#define MSR_VM_CR       0xc0010114
 #define MSR_VM_HSAVE_PA 0xc0010117ULL
 
+#define SVM_VM_CR_SVM_DISABLE 4
+
 #define SVM_SELECTOR_S_SHIFT 4
 #define SVM_SELECTOR_DPL_SHIFT 5
 #define SVM_SELECTOR_P_SHIFT 7
-- 
1.5.2.2

Re: [PATCH 11/20] KVM: SVM: Reliably detect if SVM was disabled by BIOS

[Roland Dreier]

seems like we probably want this for 2.6.22... it fixes a panic that
I've seen actually reported by several users.  Is there any risk to
merging it?

i guess getting it into 2.6.22.1 would be OK too.

Re: [PATCH 11/20] KVM: SVM: Reliably detect if SVM was disabled by BIOS

[Avi Kivity]

Roland Dreier wrote:
> seems like we probably want this for 2.6.22... it fixes a panic that
> I've seen actually reported by several users.  Is there any risk to
> merging it?
>
> i guess getting it into 2.6.22.1 would be OK too.
>   

Well, there were reports of miscompiles with the original patch.  This 
one is hopefully fixed.  I'll wait a day or two and upstream it.

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

[PATCH 12/20] KVM: Remove kvmfs in favor of the anonymous inodes source

[Avi Kivity]

kvm uses a pseudo filesystem, kvmfs, to generate inodes, a job that the
new anonymous inodes source does much better.

Cc: Davide Libenzi <davidel@xmailserver.org>
Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm_main.c |  143 ++++--------------------------------------------
 fs/anon_inodes.c       |    1 +
 include/linux/magic.h  |    1 -
 3 files changed, 12 insertions(+), 133 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 5603000..26ca90f 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -43,6 +43,7 @@
 #include <linux/sched.h>
 #include <linux/cpumask.h>
 #include <linux/smp.h>
+#include <linux/anon_inodes.h>
 
 #include "x86_emulate.h"
 #include "segment_descriptor.h"
@@ -81,8 +82,6 @@ static struct kvm_stats_debugfs_item {
 
 static struct dentry *debugfs_dir;
 
-struct vfsmount *kvmfs_mnt;
-
 #define MAX_IO_MSRS 256
 
 #define CR0_RESEVED_BITS 0xffffffff1ffaffc0ULL
@@ -104,55 +103,6 @@ struct segment_descriptor_64 {
 static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
 			   unsigned long arg);
 
-static struct inode *kvmfs_inode(struct file_operations *fops)
-{
-	int error = -ENOMEM;
-	struct inode *inode = new_inode(kvmfs_mnt->mnt_sb);
-
-	if (!inode)
-		goto eexit_1;
-
-	inode->i_fop = fops;
-
-	/*
-	 * Mark the inode dirty from the very beginning,
-	 * that way it will never be moved to the dirty
-	 * list because mark_inode_dirty() will think
-	 * that it already _is_ on the dirty list.
-	 */
-	inode->i_state = I_DIRTY;
-	inode->i_mode = S_IRUSR | S_IWUSR;
-	inode->i_uid = current->fsuid;
-	inode->i_gid = current->fsgid;
-	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
-	return inode;
-
-eexit_1:
-	return ERR_PTR(error);
-}
-
-static struct file *kvmfs_file(struct inode *inode, void *private_data)
-{
-	struct file *file = get_empty_filp();
-
-	if (!file)
-		return ERR_PTR(-ENFILE);
-
-	file->f_path.mnt = mntget(kvmfs_mnt);
-	file->f_path.dentry = d_alloc_anon(inode);
-	if (!file->f_path.dentry)
-		return ERR_PTR(-ENOMEM);
-	file->f_mapping = inode->i_mapping;
-
-	file->f_pos = 0;
-	file->f_flags = O_RDWR;
-	file->f_op = inode->i_fop;
-	file->f_mode = FMODE_READ | FMODE_WRITE;
-	file->f_version = 0;
-	file->private_data = private_data;
-	return file;
-}
-
 unsigned long segment_base(u16 selector)
 {
 	struct descriptor_table gdt;
@@ -2413,34 +2363,12 @@ static int create_vcpu_fd(struct kvm_vcpu *vcpu)
 	struct inode *inode;
 	struct file *file;
 
+	r = anon_inode_getfd(&fd, &inode, &file,
+			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
+	if (r)
+		return r;
 	atomic_inc(&vcpu->kvm->filp->f_count);
-	inode = kvmfs_inode(&kvm_vcpu_fops);
-	if (IS_ERR(inode)) {
-		r = PTR_ERR(inode);
-		goto out1;
-	}
-
-	file = kvmfs_file(inode, vcpu);
-	if (IS_ERR(file)) {
-		r = PTR_ERR(file);
-		goto out2;
-	}
-
-	r = get_unused_fd();
-	if (r < 0)
-		goto out3;
-	fd = r;
-	fd_install(fd, file);
-
 	return fd;
-
-out3:
-	fput(file);
-out2:
-	iput(inode);
-out1:
-	fput(vcpu->kvm->filp);
-	return r;
 }
 
 /*
@@ -2905,41 +2833,18 @@ static int kvm_dev_ioctl_create_vm(void)
 	struct file *file;
 	struct kvm *kvm;
 
-	inode = kvmfs_inode(&kvm_vm_fops);
-	if (IS_ERR(inode)) {
-		r = PTR_ERR(inode);
-		goto out1;
-	}
-
 	kvm = kvm_create_vm();
-	if (IS_ERR(kvm)) {
-		r = PTR_ERR(kvm);
-		goto out2;
+	if (IS_ERR(kvm))
+		return PTR_ERR(kvm);
+	r = anon_inode_getfd(&fd, &inode, &file, "kvm-vm", &kvm_vm_fops, kvm);
+	if (r) {
+		kvm_destroy_vm(kvm);
+		return r;
 	}
 
-	file = kvmfs_file(inode, kvm);
-	if (IS_ERR(file)) {
-		r = PTR_ERR(file);
-		goto out3;
-	}
 	kvm->filp = file;
 
-	r = get_unused_fd();
-	if (r < 0)
-		goto out4;
-	fd = r;
-	fd_install(fd, file);
-
 	return fd;
-
-out4:
-	fput(file);
-out3:
-	kvm_destroy_vm(kvm);
-out2:
-	iput(inode);
-out1:
-	return r;
 }
 
 static long kvm_dev_ioctl(struct file *filp,
@@ -3211,18 +3116,6 @@ static struct sys_device kvm_sysdev = {
 
 hpa_t bad_page_address;
 
-static int kvmfs_get_sb(struct file_system_type *fs_type, int flags,
-			const char *dev_name, void *data, struct vfsmount *mnt)
-{
-	return get_sb_pseudo(fs_type, "kvm:", NULL, KVMFS_SUPER_MAGIC, mnt);
-}
-
-static struct file_system_type kvm_fs_type = {
-	.name		= "kvmfs",
-	.get_sb		= kvmfs_get_sb,
-	.kill_sb	= kill_anon_super,
-};
-
 int kvm_init_arch(struct kvm_arch_ops *ops, struct module *module)
 {
 	int r;
@@ -3307,14 +3200,6 @@ static __init int kvm_init(void)
 	if (r)
 		goto out4;
 
-	r = register_filesystem(&kvm_fs_type);
-	if (r)
-		goto out3;
-
-	kvmfs_mnt = kern_mount(&kvm_fs_type);
-	r = PTR_ERR(kvmfs_mnt);
-	if (IS_ERR(kvmfs_mnt))
-		goto out2;
 	kvm_init_debug();
 
 	kvm_init_msr_list();
@@ -3331,10 +3216,6 @@ static __init int kvm_init(void)
 
 out:
 	kvm_exit_debug();
-	mntput(kvmfs_mnt);
-out2:
-	unregister_filesystem(&kvm_fs_type);
-out3:
 	kvm_mmu_module_exit();
 out4:
 	return r;
@@ -3344,8 +3225,6 @@ static __exit void kvm_exit(void)
 {
 	kvm_exit_debug();
 	__free_page(pfn_to_page(bad_page_address >> PAGE_SHIFT));
-	mntput(kvmfs_mnt);
-	unregister_filesystem(&kvm_fs_type);
 	kvm_mmu_module_exit();
 }
 
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 40fe3a3..edc6748 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -139,6 +139,7 @@ err_put_filp:
 	put_filp(file);
 	return error;
 }
+EXPORT_SYMBOL_GPL(anon_inode_getfd);
 
 /*
  * A single inode exist for all anon_inode files. Contrary to pipes,
diff --git a/include/linux/magic.h b/include/linux/magic.h
index 9d713c0..36cc20d 100644
--- a/include/linux/magic.h
+++ b/include/linux/magic.h
@@ -13,7 +13,6 @@
 #define HPFS_SUPER_MAGIC	0xf995e849
 #define ISOFS_SUPER_MAGIC	0x9660
 #define JFFS2_SUPER_MAGIC	0x72b6
-#define KVMFS_SUPER_MAGIC	0x19700426
 #define ANON_INODE_FS_MAGIC	0x09041934
 
 #define MINIX_SUPER_MAGIC	0x137F		/* original minix fs */
-- 
1.5.2.2

[PATCH 13/20] KVM: Clean up #includes

[Avi Kivity]

Remove unnecessary ones, and rearange the remaining in the standard order.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm_main.c |   18 +++++++-----------
 drivers/kvm/mmu.c      |   10 ++++++----
 drivers/kvm/svm.c      |    7 ++++---
 drivers/kvm/vmx.c      |    5 +++--
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 26ca90f..ea02719 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -16,37 +16,33 @@
  */
 
 #include "kvm.h"
+#include "x86_emulate.h"
+#include "segment_descriptor.h"
 
 #include <linux/kvm.h>
 #include <linux/module.h>
 #include <linux/errno.h>
-#include <linux/magic.h>
-#include <asm/processor.h>
 #include <linux/percpu.h>
 #include <linux/gfp.h>
-#include <asm/msr.h>
 #include <linux/mm.h>
 #include <linux/miscdevice.h>
 #include <linux/vmalloc.h>
-#include <asm/uaccess.h>
 #include <linux/reboot.h>
-#include <asm/io.h>
 #include <linux/debugfs.h>
 #include <linux/highmem.h>
 #include <linux/file.h>
-#include <asm/desc.h>
 #include <linux/sysdev.h>
 #include <linux/cpu.h>
-#include <linux/file.h>
-#include <linux/fs.h>
-#include <linux/mount.h>
 #include <linux/sched.h>
 #include <linux/cpumask.h>
 #include <linux/smp.h>
 #include <linux/anon_inodes.h>
 
-#include "x86_emulate.h"
-#include "segment_descriptor.h"
+#include <asm/processor.h>
+#include <asm/msr.h>
+#include <asm/io.h>
+#include <asm/uaccess.h>
+#include <asm/desc.h>
 
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index 49ffbd3..b297a6b 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -16,16 +16,18 @@
  * the COPYING file in the top-level directory.
  *
  */
+
+#include "vmx.h"
+#include "kvm.h"
+
 #include <linux/types.h>
 #include <linux/string.h>
-#include <asm/page.h>
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/module.h>
-#include <asm/cmpxchg.h>
 
-#include "vmx.h"
-#include "kvm.h"
+#include <asm/page.h>
+#include <asm/cmpxchg.h>
 
 #undef MMU_DEBUG
 
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index a0d4428..bc818cc 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -14,16 +14,17 @@
  *
  */
 
+#include "kvm_svm.h"
+#include "x86_emulate.h"
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/vmalloc.h>
 #include <linux/highmem.h>
 #include <linux/profile.h>
 #include <linux/sched.h>
-#include <asm/desc.h>
 
-#include "kvm_svm.h"
-#include "x86_emulate.h"
+#include <asm/desc.h>
 
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index 7d04ffa..80628f6 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -17,17 +17,18 @@
 
 #include "kvm.h"
 #include "vmx.h"
+#include "segment_descriptor.h"
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/profile.h>
 #include <linux/sched.h>
+
 #include <asm/io.h>
 #include <asm/desc.h>
 
-#include "segment_descriptor.h"
-
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
-- 
1.5.2.2

[PATCH 14/20] HOTPLUG: Add CPU_DYING notifier

[Avi Kivity]

KVM wants a notification when a cpu is about to die, so it can disable
hardware extensions, but at a time when user processes cannot be scheduled
on the cpu, so it doesn't try to use virtualization extensions after they
have been disabled.

This adds a CPU_DYING notification.  The notification is called in atomic
context on the doomed cpu.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 include/linux/notifier.h |    3 +++
 kernel/cpu.c             |   16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 9431101..576f2bb 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -196,6 +196,8 @@ extern int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
 #define CPU_DEAD		0x0007 /* CPU (unsigned)v dead */
 #define CPU_LOCK_ACQUIRE	0x0008 /* Acquire all hotcpu locks */
 #define CPU_LOCK_RELEASE	0x0009 /* Release all hotcpu locks */
+#define CPU_DYING		0x000A /* CPU (unsigned)v not running any task,
+				        * not handling interrupts, soon dead */
 
 /* Used for CPU hotplug events occuring while tasks are frozen due to a suspend
  * operation in progress
@@ -208,6 +210,7 @@ extern int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
 #define CPU_DOWN_PREPARE_FROZEN	(CPU_DOWN_PREPARE | CPU_TASKS_FROZEN)
 #define CPU_DOWN_FAILED_FROZEN	(CPU_DOWN_FAILED | CPU_TASKS_FROZEN)
 #define CPU_DEAD_FROZEN		(CPU_DEAD | CPU_TASKS_FROZEN)
+#define CPU_DYING_FROZEN	(CPU_DYING | CPU_TASKS_FROZEN)
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_NOTIFIER_H */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 208cf34..181ae70 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -103,11 +103,19 @@ static inline void check_for_tasks(int cpu)
 	write_unlock_irq(&tasklist_lock);
 }
 
+struct take_cpu_down_param {
+	unsigned long mod;
+	void *hcpu;
+};
+
 /* Take this CPU down. */
-static int take_cpu_down(void *unused)
+static int take_cpu_down(void *_param)
 {
+	struct take_cpu_down_param *param = _param;
 	int err;
 
+	raw_notifier_call_chain(&cpu_chain, CPU_DYING | param->mod,
+				param->hcpu);
 	/* Ensure this CPU doesn't handle any more interrupts. */
 	err = __cpu_disable();
 	if (err < 0)
@@ -127,6 +135,10 @@ static int _cpu_down(unsigned int cpu, int tasks_frozen)
 	cpumask_t old_allowed, tmp;
 	void *hcpu = (void *)(long)cpu;
 	unsigned long mod = tasks_frozen ? CPU_TASKS_FROZEN : 0;
+	struct take_cpu_down_param tcd_param = {
+		.mod = mod,
+		.hcpu = hcpu,
+	};
 
 	if (num_online_cpus() == 1)
 		return -EBUSY;
@@ -153,7 +165,7 @@ static int _cpu_down(unsigned int cpu, int tasks_frozen)
 	set_cpus_allowed(current, tmp);
 
 	mutex_lock(&cpu_bitmask_lock);
-	p = __stop_machine_run(take_cpu_down, NULL, cpu);
+	p = __stop_machine_run(take_cpu_down, &tcd_param, cpu);
 	mutex_unlock(&cpu_bitmask_lock);
 
 	if (IS_ERR(p) || cpu_online(cpu)) {
-- 
1.5.2.2

[PATCH 15/20] HOTPLUG: Adapt cpuset hotplug callback to CPU_DYING

[Avi Kivity]

CPU_DYING is called in atomic context, so don't try to take any locks.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 kernel/cpuset.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 4c49188..c4d123f 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -2138,6 +2138,9 @@ static void common_cpu_mem_hotplug_unplug(void)
 static int cpuset_handle_cpuhp(struct notifier_block *nb,
 				unsigned long phase, void *cpu)
 {
+	if (phase == CPU_DYING || phase == CPU_DYING_FROZEN)
+		return NOTIFY_DONE;
+
 	common_cpu_mem_hotplug_unplug();
 	return 0;
 }
-- 
1.5.2.2

[PATCH 16/20] HOTPLUG: Adapt thermal throttle to CPU_DYING

[Avi Kivity]

CPU_DYING is notified in atomic context, so no taking mutexes here.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 arch/i386/kernel/cpu/mcheck/therm_throt.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/i386/kernel/cpu/mcheck/therm_throt.c b/arch/i386/kernel/cpu/mcheck/therm_throt.c
index 7ba7c3a..1203dc5 100644
--- a/arch/i386/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/i386/kernel/cpu/mcheck/therm_throt.c
@@ -134,19 +134,21 @@ static __cpuinit int thermal_throttle_cpu_callback(struct notifier_block *nfb,
 	int err;
 
 	sys_dev = get_cpu_sysdev(cpu);
-	mutex_lock(&therm_cpu_lock);
 	switch (action) {
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
+		mutex_lock(&therm_cpu_lock);
 		err = thermal_throttle_add_dev(sys_dev);
+		mutex_unlock(&therm_cpu_lock);
 		WARN_ON(err);
 		break;
 	case CPU_DEAD:
 	case CPU_DEAD_FROZEN:
+		mutex_lock(&therm_cpu_lock);
 		thermal_throttle_remove_dev(sys_dev);
+		mutex_unlock(&therm_cpu_lock);
 		break;
 	}
-	mutex_unlock(&therm_cpu_lock);
 	return NOTIFY_OK;
 }
 
-- 
1.5.2.2

[PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

This defines on_cpu() which is similar to smp_call_function_single()
except that it works if cpu happens to be the current cpu.  Can also be
seen as a complement to on_each_cpu() (which also doesn't treat the
current cpu specially).

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 include/linux/smp.h |   16 ++++++++++++++++
 kernel/softirq.c    |   24 ++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index 96ac21f..613edd2 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -7,6 +7,7 @@
  */
 
 #include <linux/errno.h>
+#include <asm/system.h>
 
 extern void cpu_idle(void);
 
@@ -61,6 +62,11 @@ int smp_call_function_single(int cpuid, void (*func) (void *info), void *info,
  * Call a function on all processors
  */
 int on_each_cpu(void (*func) (void *info), void *info, int retry, int wait);
+/*
+ * Call a function on one processor
+ */
+int on_cpu(int cpu, void (*func)(void *info), void *info,
+	       int retry, int wait);
 
 #define MSG_ALL_BUT_SELF	0x8000	/* Assume <32768 CPU's */
 #define MSG_ALL			0x8001
@@ -96,6 +102,16 @@ static inline int up_smp_call_function(void)
 		local_irq_enable();		\
 		0;				\
 	})
+
+static inline int on_cpu(int cpu, void (*func)(void *info), void *info,
+			     int retry, int wait)
+{
+	local_irq_disable();
+	func(info);
+	local_irq_enable();
+	return 0;
+}
+
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
 #define smp_prepare_boot_cpu()			do {} while (0)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 0b9886a..11666f7 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -658,4 +658,28 @@ int on_each_cpu(void (*func) (void *info), void *info, int retry, int wait)
 	return ret;
 }
 EXPORT_SYMBOL(on_each_cpu);
+
+/*
+ * Call a function on one processor, which might be the currently executing
+ * processor.
+ */
+int on_cpu(int cpu, void (*func) (void *info), void *info,
+	       int retry, int wait)
+{
+	int ret;
+	int this_cpu;
+
+	this_cpu = get_cpu();
+	if (this_cpu == cpu) {
+		local_irq_disable();
+		func(info);
+		local_irq_enable();
+		ret = 0;
+	} else
+		ret = smp_call_function_single(cpu, func, info, retry, wait);
+	put_cpu();
+	return ret;
+}
+EXPORT_SYMBOL(on_cpu);
+
 #endif
-- 
1.5.2.2

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Andi Kleen]

> This defines on_cpu() which is similar to smp_call_function_single()
> except that it works if cpu happens to be the current cpu.  Can also be
> seen as a complement to on_each_cpu() (which also doesn't treat the
> current cpu specially).

I think it would be better to fix smp_call_function_single to just
handle this case transparently. There aren't that many callers yet because it is
fairly new.

-Andi

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

[cc list restored]

Andi Kleen wrote:
>> This defines on_cpu() which is similar to smp_call_function_single()
>> except that it works if cpu happens to be the current cpu.  Can also be
>> seen as a complement to on_each_cpu() (which also doesn't treat the
>> current cpu specially).
>>     
>
> I think it would be better to fix smp_call_function_single to just
> handle this case transparently. There aren't that many callers yet because it is
> fairly new.
>   

Well, smp_call_function_single() is arch specific whereas on_cpu() is
generic code.  Perhaps rename smp_call_function_single() to
__smp_call_function_single() and on_cpu() to smp_call_function_single()?

I dislike the loss of symmetry though.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Andi Kleen]

> Well, smp_call_function_single() is arch specific whereas on_cpu() is

Yes, but the few instances should be relatively easy to fix.

> generic code.  Perhaps rename smp_call_function_single() to
> __smp_call_function_single() and on_cpu() to smp_call_function_single()?

The low level function checks for this anyways. Instead of erroring
it should just DTRT.

> 
> I dislike the loss of symmetry though.

on_each_cpu() was imho always a mistake. It would have been better
to just fix smp_call_function() directly

-Andi

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

Andi Kleen wrote:
>> Well, smp_call_function_single() is arch specific whereas on_cpu() is
>>     
>
> Yes, but the few instances should be relatively easy to fix.
>
>   
>> generic code.  Perhaps rename smp_call_function_single() to
>> __smp_call_function_single() and on_cpu() to smp_call_function_single()?
>>     
>
> The low level function checks for this anyways. Instead of erroring
> it should just DTRT.
>   

Okay.  I'll make that change.


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

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

Avi Kivity wrote:
> Andi Kleen wrote:
>>> Well, smp_call_function_single() is arch specific whereas on_cpu() is
>>>     
>>
>> Yes, but the few instances should be relatively easy to fix.
>>
>>  
>>> generic code.  Perhaps rename smp_call_function_single() to
>>> __smp_call_function_single() and on_cpu() to 
>>> smp_call_function_single()?
>>>     
>>
>> The low level function checks for this anyways. Instead of erroring
>> it should just DTRT.
>>   
>
> Okay.  I'll make that change.
>
>

Here it is (whitespace-mangled, don't try to apply).

diff --git a/arch/i386/kernel/smpcommon.c b/arch/i386/kernel/smpcommon.c
index 1868ae1..bbfe85a 100644
--- a/arch/i386/kernel/smpcommon.c
+++ b/arch/i386/kernel/smpcommon.c
@@ -47,7 +47,7 @@ int smp_call_function(void (*func) (void *info), void *info, int nonatomic,
 EXPORT_SYMBOL(smp_call_function);
 
 /**
- * smp_call_function_single - Run a function on another CPU
+ * smp_call_function_single - Run a function on a specific CPU
  * @cpu: The target CPU.  Cannot be the calling CPU.
  * @func: The function to run. This must be fast and non-blocking.
  * @info: An arbitrary pointer to pass to the function.
@@ -66,9 +66,11 @@ int smp_call_function_single(int cpu, void (*func) (void *info), void *info,
 	int ret;
 	int me = get_cpu();
 	if (cpu == me) {
-		WARN_ON(1);
+		local_irq_disable();
+		func(info);
+		local_irq_enable();
 		put_cpu();
-		return -EBUSY;
+		return 0;
 	}
 
 	ret = smp_call_function_mask(cpumask_of_cpu(cpu), func, info, wait);
diff --git a/arch/x86_64/kernel/smp.c b/arch/x86_64/kernel/smp.c
index 2ff4685..e6e5017 100644
--- a/arch/x86_64/kernel/smp.c
+++ b/arch/x86_64/kernel/smp.c
@@ -357,7 +357,7 @@ __smp_call_function_single(int cpu, void (*func) (void *info), void *info,
 }
 
 /*
- * smp_call_function_single - Run a function on another CPU
+ * smp_call_function_single - Run a function on a specific CPU
  * @func: The function to run. This must be fast and non-blocking.
  * @info: An arbitrary pointer to pass to the function.
  * @nonatomic: Currently unused.
@@ -372,16 +372,19 @@ __smp_call_function_single(int cpu, void (*func) (void *info), void *info,
 int smp_call_function_single (int cpu, void (*func) (void *info), void *info,
 	int nonatomic, int wait)
 {
+	/* Can deadlock when called with interrupts disabled */
+	WARN_ON(irqs_disabled());
+
 	/* prevent preemption and reschedule on another processor */
 	int me = get_cpu();
 	if (cpu == me) {
+		local_irq_disable();
+		func(info);
+		local_irq_enable();
 		put_cpu();
 		return 0;
 	}
 
-	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
-
 	spin_lock_bh(&call_lock);
 	__smp_call_function_single(cpu, func, info, nonatomic, wait);
 	spin_unlock_bh(&call_lock);
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 613edd2..ed38a3d 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -118,7 +118,11 @@ static inline void smp_send_reschedule(int cpu) { }
 static inline int smp_call_function_single(int cpuid, void (*func) (void *info),
 					   void *info, int retry, int wait)
 {
-	return -EBUSY;
+	WARN_ON(cpuid != 0);
+	local_irq_disable();
+	func(info);
+	local_irq_enable();
+	return 0;
 }
 
 #endif /* !SMP */


If there are no objections, I will push it (split up) through my kvm updates patchset, as other kvm patches depend on it.  I will submit patches to other archs through the arch maintainers as kvm doesn't care about them yet.


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

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Satyam Sharma]

Hi,

ISTR participating in a similar discussion some time back, but ...
anyway, I don't like the change in semantics of smp_call_function()
being proposed here *at* *all* ...

On 7/9/07, Avi Kivity <avi@qumranet.com> wrote:
> >> This defines on_cpu() which is similar to smp_call_function_single()
> >> except that it works if cpu happens to be the current cpu.  Can also be
> >> seen as a complement to on_each_cpu() (which also doesn't treat the
> >> current cpu specially).

I like the patch being originally proposed here. For the sake of
correctness, it is _compulsory_ to wrap a get_cpu() / put_cpu()
pair around calls to smp_call_function{_single} in any case,
so it makes sense to provide a function that does this wrapping
in itself, to reduce likelihood of bugs and also get rid of open-coding.

[ In fact I don't like the fact that for the UP case you're simply
executing the function locally without even checking that the
cpu argument passed is indeed == 0. We had discussed this
previously and you did mention that cpu == 0 for !SMP is
assumed to be true, but I don't see what we lose by asserting
that "trivial assumption" either. ]

On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
> [...]
> on_each_cpu() was imho always a mistake. It would have been better
> to just fix smp_call_function() directly

I'm not sure what you mean by "fix" here, but if you're proposing
that we change smp_call_function() semantics to _include_ the
current CPU (and just run the given function locally also along
with the others -- and hence get rid of on_each_cpu) then I'm sorry
but I'll have to *violently* disagree with that. Please remember that
the current CPU _must_ be treated specially in a whole *lot* of
usage scenarios ...

Take smp_send_stop() for instance. We need to send a suicide
function:

for (;;)
        halt();

to all _other_ CPUs *only* -- it would be *insane* to include ourselves
(current CPU, current thread) and just execute this suicide function
_locally_ *in current thread* too.

OTOH, there are plenty of situations where we actually _want_ to
get some function executed on *each* CPU (_including_ the current
local CPU that is executing that thread) -- naturally on_each_cpu()
would make sense for those cases.

Both have their purposes -- both must co-exist.

On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
> > I think it would be better to fix smp_call_function_single to just
> > handle this case transparently. There aren't that many callers yet
> > because it is
> > fairly new.

Take the same example here -- let's say we want to send a
"for (;;) ;" kind of function to a specified CPU. Now let's say
by the time we've called smp_call_function_single() on that
target CPU, we're preempted out and then get rescheduled
on the target CPU itself. There, we begin executing the
smp_call_function_single() (as modified by Avi here with your
proposed changed semantics) and notice that we've landed
on the target CPU itself, execute the suicidal function
_locally_ *in current thread* itself, and ... well, I hope you
get the picture.

So my opinion is to go with the get_cpu() / put_cpu() wrapper
Avi is proposing here and keep smp_call_function{_single}
semantics unchanged. [ Also please remember that for
*correctness*, preemption needs to be disabled by the
_caller_ of smp_call_function{_single} functions, doing so
inside them is insufficient. ]

Satyam

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

Satyam Sharma wrote:
>
>
> On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
>> [...]
>> on_each_cpu() was imho always a mistake. It would have been better
>> to just fix smp_call_function() directly
>
> I'm not sure what you mean by "fix" here, but if you're proposing
> that we change smp_call_function() semantics to _include_ the
> current CPU (and just run the given function locally also along
> with the others -- and hence get rid of on_each_cpu) then I'm sorry
> but I'll have to *violently* disagree with that. Please remember that
> the current CPU _must_ be treated specially in a whole *lot* of
> usage scenarios ...

I imagine that by "fix" Andi means also updating all callers.  Otherwise
he would just have said "break".

>
> On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
>> > I think it would be better to fix smp_call_function_single to just
>> > handle this case transparently. There aren't that many callers yet
>> > because it is
>> > fairly new.
>
> Take the same example here -- let's say we want to send a
> "for (;;) ;" kind of function to a specified CPU. Now let's say
> by the time we've called smp_call_function_single() on that
> target CPU, we're preempted out and then get rescheduled
> on the target CPU itself. There, we begin executing the
> smp_call_function_single() (as modified by Avi here with your
> proposed changed semantics) and notice that we've landed
> on the target CPU itself, execute the suicidal function
> _locally_ *in current thread* itself, and ... well, I hope you
> get the picture.

So you disable preemption before calling smp_call_function_single().

>
> So my opinion is to go with the get_cpu() / put_cpu() wrapper
> Avi is proposing here and keep smp_call_function{_single}
> semantics unchanged. [ Also please remember that for
> *correctness*, preemption needs to be disabled by the
> _caller_ of smp_call_function{_single} functions, doing so
> inside them is insufficient. ]

That's not correct.  kvm has two places where you can call the new
smp_call_function_single() (or on_cpu()) without disabling preemption. 
There are also a couple of existing places that don't need to disable
preemption with the new semantics (see mtrr_save_state(), do_cpuid(),  
_rdmsr_on_cpu(), all in arch/i386 for examples).  In fact I think more
places can take advantage of the new semantics than not.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Satyam Sharma]

On 7/10/07, Avi Kivity <avi@qumranet.com> wrote:
> Satyam Sharma wrote:
> >
> >
> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
> >> [...]
> >> on_each_cpu() was imho always a mistake. It would have been better
> >> to just fix smp_call_function() directly
> >
> > I'm not sure what you mean by "fix" here, but if you're proposing
> > that we change smp_call_function() semantics to _include_ the
> > current CPU (and just run the given function locally also along
> > with the others -- and hence get rid of on_each_cpu) then I'm sorry
> > but I'll have to *violently* disagree with that. Please remember that
> > the current CPU _must_ be treated specially in a whole *lot* of
> > usage scenarios ...
>
> I imagine that by "fix" Andi means also updating all callers.  Otherwise
> he would just have said "break".

But that's the point. How do you plan / intend to update
smp_send_stop()?

More importantly, what's wrong with it in the first place (to "fix")?

> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
> >> > I think it would be better to fix smp_call_function_single to just
> >> > handle this case transparently. There aren't that many callers yet
> >> > because it is
> >> > fairly new.
> >
> > Take the same example here -- let's say we want to send a
> > "for (;;) ;" kind of function to a specified CPU. Now let's say
> > by the time we've called smp_call_function_single() on that
> > target CPU, we're preempted out and then get rescheduled
> > on the target CPU itself. There, we begin executing the
> > smp_call_function_single() (as modified by Avi here with your
> > proposed changed semantics) and notice that we've landed
> > on the target CPU itself, execute the suicidal function
> > _locally_ *in current thread* itself, and ... well, I hope you
> > get the picture.
>
> So you disable preemption before calling smp_call_function_single().

Which is what on_cpu() and which is why I like that.

And which is *not* what Andi's proposal (or your later patch
implementing that proposal) does, and which is why I *don't*
like that.

> > So my opinion is to go with the get_cpu() / put_cpu() wrapper
> > Avi is proposing here and keep smp_call_function{_single}
> > semantics unchanged. [ Also please remember that for
> > *correctness*, preemption needs to be disabled by the
> > _caller_ of smp_call_function{_single} functions, doing so
> > inside them is insufficient. ]
>
> That's not correct.  kvm has two places where you can call the new
> smp_call_function_single() (or on_cpu()) without disabling preemption.

on_cpu() _is_ the wrapper that does the necessary get_cpu()
(i.e. preemption-disabling wrap over smp_call_function_single).

Obviously a caller of on_cpu() does not need to disable preemption.

[ This was w.r.t. existing smp_call_function{_single} semantics. ]

> There are also a couple of existing places that don't need to disable
> preemption with the new semantics (see mtrr_save_state(), do_cpuid(),
> _rdmsr_on_cpu(), all in arch/i386 for examples).  In fact I think more
> places can take advantage of the new semantics than not.

I presume you mean these are places where we just specify the CPU
to execute the function on, and don't really care if by that time we've
gone over to that CPU itself -- so the new semantics are fine too?
So these are places where you can use on_cpu(). But why change
existing semantics of smp_call_function_single is what I can't quite
understand, when there are perfectly legitimate usage cases where we
_don't_ want the function to get executed locally.

Satyam

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

Satyam Sharma wrote:
> On 7/10/07, Avi Kivity <avi@qumranet.com> wrote:
>> Satyam Sharma wrote:
>> >
>> >
>> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
>> >> [...]
>> >> on_each_cpu() was imho always a mistake. It would have been better
>> >> to just fix smp_call_function() directly
>> >
>> > I'm not sure what you mean by "fix" here, but if you're proposing
>> > that we change smp_call_function() semantics to _include_ the
>> > current CPU (and just run the given function locally also along
>> > with the others -- and hence get rid of on_each_cpu) then I'm sorry
>> > but I'll have to *violently* disagree with that. Please remember that
>> > the current CPU _must_ be treated specially in a whole *lot* of
>> > usage scenarios ...
>>
>> I imagine that by "fix" Andi means also updating all callers.  Otherwise
>> he would just have said "break".
>
> But that's the point. How do you plan / intend to update
> smp_send_stop()?
>

Well, I don't plan to do anything to smp_call_function().  I imagine you 
can add a flag, or compare smp_processor_id() to the cpu that's not 
stopping, or use smp_call_function_mask().

> More importantly, what's wrong with it in the first place (to "fix")?

If most use cases want to run a function on all cpus, they shouldn't 
need to open code it.


>
>> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
>> >> > I think it would be better to fix smp_call_function_single to just
>> >> > handle this case transparently. There aren't that many callers yet
>> >> > because it is
>> >> > fairly new.
>> >
>> > Take the same example here -- let's say we want to send a
>> > "for (;;) ;" kind of function to a specified CPU. Now let's say
>> > by the time we've called smp_call_function_single() on that
>> > target CPU, we're preempted out and then get rescheduled
>> > on the target CPU itself. There, we begin executing the
>> > smp_call_function_single() (as modified by Avi here with your
>> > proposed changed semantics) and notice that we've landed
>> > on the target CPU itself, execute the suicidal function
>> > _locally_ *in current thread* itself, and ... well, I hope you
>> > get the picture.
>>
>> So you disable preemption before calling smp_call_function_single().
>
> Which is what on_cpu() and which is why I like that.
>
> And which is *not* what Andi's proposal (or your later patch
> implementing that proposal) does, and which is why I *don't*
> like that.

It does disable preemption.  Look more carefully.

>
>> > So my opinion is to go with the get_cpu() / put_cpu() wrapper
>> > Avi is proposing here and keep smp_call_function{_single}
>> > semantics unchanged. [ Also please remember that for
>> > *correctness*, preemption needs to be disabled by the
>> > _caller_ of smp_call_function{_single} functions, doing so
>> > inside them is insufficient. ]
>>
>> That's not correct.  kvm has two places where you can call the new
>> smp_call_function_single() (or on_cpu()) without disabling preemption.
>
> on_cpu() _is_ the wrapper that does the necessary get_cpu()
> (i.e. preemption-disabling wrap over smp_call_function_single).
>
> Obviously a caller of on_cpu() does not need to disable preemption.

Neither does the caller of the new smp_call_function_single().  Look at 
the code.

>
>> There are also a couple of existing places that don't need to disable
>> preemption with the new semantics (see mtrr_save_state(), do_cpuid(),
>> _rdmsr_on_cpu(), all in arch/i386 for examples).  In fact I think more
>> places can take advantage of the new semantics than not.
>
> I presume you mean these are places where we just specify the CPU
> to execute the function on, and don't really care if by that time we've
> gone over to that CPU itself -- so the new semantics are fine too?
> So these are places where you can use on_cpu(). But why change
> existing semantics of smp_call_function_single is what I can't quite
> understand, when there are perfectly legitimate usage cases where we
> _don't_ want the function to get executed locally.

Most (all?) do.  And there's not harm done if they don't.  Look at the code.


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

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Satyam Sharma]

On 7/10/07, Avi Kivity <avi@qumranet.com> wrote:
> Satyam Sharma wrote:
> > On 7/10/07, Avi Kivity <avi@qumranet.com> wrote:
> >> Satyam Sharma wrote:
> >> >
> >> >
> >> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
> >> >> [...]
> >> >> on_each_cpu() was imho always a mistake. It would have been better
> >> >> to just fix smp_call_function() directly
> >> >
> >> > I'm not sure what you mean by "fix" here, but if you're proposing
> >> > that we change smp_call_function() semantics to _include_ the
> >> > current CPU (and just run the given function locally also along
> >> > with the others -- and hence get rid of on_each_cpu) then I'm sorry
> >> > but I'll have to *violently* disagree with that. Please remember that
> >> > the current CPU _must_ be treated specially in a whole *lot* of
> >> > usage scenarios ...
> >>
> >> I imagine that by "fix" Andi means also updating all callers.  Otherwise
> >> he would just have said "break".
> >
> > But that's the point. How do you plan / intend to update
> > smp_send_stop()?
>
> Well, I don't plan to do anything to smp_call_function().

Thanks, I wish smp_call_function_single() could be left alone as well,
different semantics of the two would be ugly and only breed confusion.

> I imagine you
> can add a flag, or compare smp_processor_id() to the cpu that's not
> stopping, or use smp_call_function_mask().

Ok, so what we (presently) have is:

1. smp_call_function() -> run given function on all *other* CPUs
2. on_each_cpu() -> run given function on _all_ CPUs

This is perfectly fine, and serves all possible use cases.

And I think what's proposed is:

1. Change smp_call_function() semantics, to run given function
on _all_ CPUs (thus getting rid of the on_each_cpu() "mistake")

2. Resort to (most probably implement another function?) using
smp_call_function_mask() or flags appropriately to also serve
the use cases where we need to run a given function on all
_other_ CPUs

Does this pointless/gratuitous code-churn really make sense?
Definitely not to me ...

[ For the _single() case we now have on_cpu() as you originally
proposed, which I definitely like and fills the other gap in the API. ]

So I still don't quite understand what is the need to change existing
semantics of smp_call_function{_single} in the first place.

> > More importantly, what's wrong with it in the first place (to "fix")?
>
> If most use cases want to run a function on all cpus, they shouldn't
> need to open code it.

We already have on_each_cpu() for precisely such usage. I don't see
what / why would someone be open-coding ...

> >> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote:
> >> >> > I think it would be better to fix smp_call_function_single to just
> >> >> > handle this case transparently. There aren't that many callers yet
> >> >> > because it is
> >> >> > fairly new.
> >> >
> >> > Take the same example here -- let's say we want to send a
> >> > "for (;;) ;" kind of function to a specified CPU. Now let's say
> >> > by the time we've called smp_call_function_single() on that
> >> > target CPU, we're preempted out and then get rescheduled
> >> > on the target CPU itself. There, we begin executing the
> >> > smp_call_function_single() (as modified by Avi here with your
> >> > proposed changed semantics) and notice that we've landed
> >> > on the target CPU itself, execute the suicidal function
> >> > _locally_ *in current thread* itself, and ... well, I hope you
> >> > get the picture.
> >>
> >> So you disable preemption before calling smp_call_function_single().
> >
> > Which is what on_cpu() and which is why I like that.
> >
> > And which is *not* what Andi's proposal (or your later patch
> > implementing that proposal) does, and which is why I *don't*
> > like that.
>
> It does disable preemption.  Look more carefully.

But for a different reason. You've confused existing and the
proposed/new semantics of smp_call_function_single() here.

* Existing semantics:

Why is disabling preemption *compulsory* for correctness with
existing smp_call_function_single semantics?

Because we want to _detect_ (and WARN() + return error) for cases
when the _caller_ failed to disable preemption => by the time we
execute smp_call_function_single, we (current thread) could
actually have gone over to the target CPU (which is illegal with
existing semantics), so we need to check for that and warn.

Now, note that disabling preemption in smp_call_function{_single}
themselves is *insufficient*. We could be running on the target
CPU _already_ by the time we disable preemption inside those
functions. We'd end up finding this, WARN'ing about it, returning
an error -- all that could've been avoided if only the _caller_ had
the good sense to disable preemption _before_ calling smp_...()

So the *only* sane way (with existing semantics) to call the
smp_...() functions is to do that with preemption disabled in
the _caller_.

* Proposed semantics:

Here, we just don't care about what CPU we are on when we're
executing smp_call_function_single() -- if we're on the target
CPU, we'll just execute the function locally (in current thread
itself) else we'll fire off the IPI to get it executed on the target
CPU.

Of course, to make the decision above, we would need to have
a "if (current_cpu == target_cpu)" kind of check, and there's no
safe way to obtain "current_cpu" without disabling preemption --
so *that* is why the proposed code did the get_cpu() /
preemption-disabling inside smp_call_function_single().

> >> > So my opinion is to go with the get_cpu() / put_cpu() wrapper
> >> > Avi is proposing here and keep smp_call_function{_single}
> >> > semantics unchanged. [ Also please remember that for
> >> > *correctness*, preemption needs to be disabled by the
> >> > _caller_ of smp_call_function{_single} functions, doing so
> >> > inside them is insufficient. ]
> >>
> >> That's not correct.  kvm has two places where you can call the new
> >> smp_call_function_single() (or on_cpu()) without disabling preemption.
> >
> > on_cpu() _is_ the wrapper that does the necessary get_cpu()
> > (i.e. preemption-disabling wrap over smp_call_function_single).
> >
> > Obviously a caller of on_cpu() does not need to disable preemption.
>
> Neither does the caller of the new smp_call_function_single().  Look at
> the code.

As I said, that's because the caller wouldn't _care_ with the new
semantics.

Anyway, I don't really care enough about this (I'm not writing any code
that could get directly affected by all this) to continue objecting to the
proposed change. All I'm pointing out is just that it's unnecessary, and
I suspect nobody likes code churn just for the sake of it.

Satyam

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Avi Kivity]

Satyam Sharma wrote:
>
> And I think what's proposed is:
>
> 1. Change smp_call_function() semantics, to run given function
> on _all_ CPUs (thus getting rid of the on_each_cpu() "mistake")
>
> 2. Resort to (most probably implement another function?) using
> smp_call_function_mask() or flags appropriately to also serve
> the use cases where we need to run a given function on all
> _other_ CPUs
>
> Does this pointless/gratuitous code-churn really make sense?
> Definitely not to me ...


It's not proposed.  Andi mentioned it in passing.  The only churn is in 
this thread.

>
> [ For the _single() case we now have on_cpu() as you originally
> proposed, which I definitely like and fills the other gap in the API. ]
>
> So I still don't quite understand what is the need to change existing
> semantics of smp_call_function{_single} in the first place.
>

I imagine Andi's motivation was that most uses benefit from this change, 
and the rest don't suffer.  It's better not to have a proliferation of 
ever-so-similar APIs.


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

Re: [PATCH 17/20] SMP: Implement on_cpu()

[Satyam Sharma]

On 7/11/07, Avi Kivity <avi@qumranet.com> wrote:
> [...]
>
> It's not proposed.  Andi mentioned it in passing.

Ok, thanks for clarifying that.

gcc + kvm + 64 bit ? confused :-/

[Benjamin Budts]

Hi all,

question...

I'm a bit confused using the -march=xxx options for gcc when using a 
kernel virtual machine...
Getting errors on smp when using the -march=nocoma for gcc... possibly 
because smp is not fully supported yet I suppse.

I guess my question is, when installing kvm, is the virtual machine 64 
bit capable or should I install 32 bit OS ?

I installed an amd64 debian image and it seems to be running 
fine...except for the problem mentioned above.

And what type of architecture do i have to mention with gcc in a kvm ? 
march ... arch...

thx !

karnak:/usr/local/src/sendmail-8.14.1# cat /proc/cpuinfo
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 2
model name      : QEMU Virtual CPU version 0.9.0
stepping        : 3
cpu MHz         : 1994.974
cache size      : 128 KB
fpu             : yes
fpu_exception   : yes
cpuid level     : 2
wp              : yes
flags           : fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
cmov pat pse36 clflush mmx fxsr sse sse2 syscall lm up pni
bogomips        : 3929.02
clflush size    : 64
cache_alignment : 64
address sizes   : 40 bits physical, 48 bits virtual
power management:

Re: [kvm-devel] gcc + kvm + 64 bit ? confused :-/

[Avi Kivity]

Benjamin Budts wrote:
> Hi all,
>
> question...
>
> I'm a bit confused using the -march=xxx options for gcc when using a 
> kernel virtual machine...
> Getting errors on smp when using the -march=nocoma for gcc... possibly 
> because smp is not fully supported yet I suppse.
>
> I guess my question is, when installing kvm, is the virtual machine 64 
> bit capable or should I install 32 bit OS ?
>   

kvm supports 64-bit and 32-bit guests if running on a 64-bit host.

> I installed an amd64 debian image and it seems to be running 
> fine...except for the problem mentioned above.
>
> And what type of architecture do i have to mention with gcc in a kvm ? 
> march ... arch...
>   

Please post the errors.

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

Re: gcc + kvm + 64 bit ? confused :-/

[Andi Kleen]

On Wed, Jul 11, 2007 at 11:43:26AM +0200, Benjamin Budts wrote:
> I'm a bit confused using the -march=xxx options for gcc when using a 
> kernel virtual machine...
> Getting errors on smp when using the -march=nocoma for gcc... possibly 


What errors? 

> because smp is not fully supported yet I suppse.
> 
> I guess my question is, when installing kvm, is the virtual machine 64 
> bit capable or should I install 32 bit OS ?
> 
> I installed an amd64 debian image and it seems to be running 
> fine...except for the problem mentioned above.
> 
> And what type of architecture do i have to mention with gcc in a kvm ? 
> march ... arch...

The same as the host or generic cpu if you're not sure.

-Andi

[PATCH 18/20] KVM: Keep track of which cpus have virtualization enabled

[Avi Kivity]

By keeping track of which cpus have virtualization enabled, we
prevent double-enable or double-disable during hotplug, which is a
very fatal oops.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm_main.c |   45 +++++++++++++++++++++++++++++++++------------
 1 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index ea02719..3226ad4 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -50,8 +50,12 @@ MODULE_LICENSE("GPL");
 static DEFINE_SPINLOCK(kvm_lock);
 static LIST_HEAD(vm_list);
 
+static cpumask_t cpus_hardware_enabled;
+
 struct kvm_arch_ops *kvm_arch_ops;
 
+static void hardware_disable(void *ignored);
+
 #define STAT_OFFSET(x) offsetof(struct kvm_vcpu, stat.x)
 
 static struct kvm_stats_debugfs_item {
@@ -2930,7 +2934,7 @@ static int kvm_reboot(struct notifier_block *notifier, unsigned long val,
 		 * in vmx root mode.
 		 */
 		printk(KERN_INFO "kvm: exiting hardware virtualization\n");
-		on_each_cpu(kvm_arch_ops->hardware_disable, NULL, 0, 1);
+		on_each_cpu(hardware_disable, NULL, 0, 1);
 	}
 	return NOTIFY_OK;
 }
@@ -2973,6 +2977,27 @@ static void decache_vcpus_on_cpu(int cpu)
 	spin_unlock(&kvm_lock);
 }
 
+static void hardware_enable(void *junk)
+{
+	int cpu = raw_smp_processor_id();
+
+	if (cpu_isset(cpu, cpus_hardware_enabled))
+		return;
+	cpu_set(cpu, cpus_hardware_enabled);
+	kvm_arch_ops->hardware_enable(NULL);
+}
+
+static void hardware_disable(void *junk)
+{
+	int cpu = raw_smp_processor_id();
+
+	if (!cpu_isset(cpu, cpus_hardware_enabled))
+		return;
+	cpu_clear(cpu, cpus_hardware_enabled);
+	decache_vcpus_on_cpu(cpu);
+	kvm_arch_ops->hardware_disable(NULL);
+}
+
 static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 			   void *v)
 {
@@ -2985,16 +3010,13 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 	case CPU_UP_CANCELED_FROZEN:
 		printk(KERN_INFO "kvm: disabling virtualization on CPU%d\n",
 		       cpu);
-		decache_vcpus_on_cpu(cpu);
-		smp_call_function_single(cpu, kvm_arch_ops->hardware_disable,
-					 NULL, 0, 1);
+		smp_call_function_single(cpu, hardware_disable, NULL, 0, 1);
 		break;
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
 		printk(KERN_INFO "kvm: enabling virtualization on CPU%d\n",
 		       cpu);
-		smp_call_function_single(cpu, kvm_arch_ops->hardware_enable,
-					 NULL, 0, 1);
+		smp_call_function_single(cpu, hardware_enable, NULL, 0, 1);
 		break;
 	}
 	return NOTIFY_OK;
@@ -3088,14 +3110,13 @@ static void kvm_exit_debug(void)
 
 static int kvm_suspend(struct sys_device *dev, pm_message_t state)
 {
-	decache_vcpus_on_cpu(raw_smp_processor_id());
-	on_each_cpu(kvm_arch_ops->hardware_disable, NULL, 0, 1);
+	on_each_cpu(hardware_disable, NULL, 0, 0);
 	return 0;
 }
 
 static int kvm_resume(struct sys_device *dev)
 {
-	on_each_cpu(kvm_arch_ops->hardware_enable, NULL, 0, 1);
+	on_each_cpu(hardware_disable, NULL, 0, 0);
 	return 0;
 }
 
@@ -3136,7 +3157,7 @@ int kvm_init_arch(struct kvm_arch_ops *ops, struct module *module)
 	if (r < 0)
 		goto out;
 
-	on_each_cpu(kvm_arch_ops->hardware_enable, NULL, 0, 1);
+	on_each_cpu(hardware_enable, NULL, 0, 1);
 	r = register_cpu_notifier(&kvm_cpu_notifier);
 	if (r)
 		goto out_free_1;
@@ -3168,7 +3189,7 @@ out_free_2:
 	unregister_reboot_notifier(&kvm_reboot_notifier);
 	unregister_cpu_notifier(&kvm_cpu_notifier);
 out_free_1:
-	on_each_cpu(kvm_arch_ops->hardware_disable, NULL, 0, 1);
+	on_each_cpu(hardware_disable, NULL, 0, 1);
 	kvm_arch_ops->hardware_unsetup();
 out:
 	kvm_arch_ops = NULL;
@@ -3182,7 +3203,7 @@ void kvm_exit_arch(void)
 	sysdev_class_unregister(&kvm_sysdev_class);
 	unregister_reboot_notifier(&kvm_reboot_notifier);
 	unregister_cpu_notifier(&kvm_cpu_notifier);
-	on_each_cpu(kvm_arch_ops->hardware_disable, NULL, 0, 1);
+	on_each_cpu(hardware_disable, NULL, 0, 1);
 	kvm_arch_ops->hardware_unsetup();
 	kvm_arch_ops = NULL;
 }
-- 
1.5.2.2

[PATCH 19/20] KVM: Tune hotplug/suspend IPIs

[Avi Kivity]

The hotplug IPIs can be called from the cpu on which we are currently
running on, so use on_cpu().  Similarly, drop on_each_cpu() for the
suspend/resume callbacks, as we're in atomic context here and only one
cpu is up anyway.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm_main.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 3226ad4..54cad51 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -3010,13 +3010,13 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 	case CPU_UP_CANCELED_FROZEN:
 		printk(KERN_INFO "kvm: disabling virtualization on CPU%d\n",
 		       cpu);
-		smp_call_function_single(cpu, hardware_disable, NULL, 0, 1);
+		on_cpu(cpu, hardware_disable, NULL, 0, 1);
 		break;
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
 		printk(KERN_INFO "kvm: enabling virtualization on CPU%d\n",
 		       cpu);
-		smp_call_function_single(cpu, hardware_enable, NULL, 0, 1);
+		on_cpu(cpu, hardware_enable, NULL, 0, 1);
 		break;
 	}
 	return NOTIFY_OK;
@@ -3110,13 +3110,13 @@ static void kvm_exit_debug(void)
 
 static int kvm_suspend(struct sys_device *dev, pm_message_t state)
 {
-	on_each_cpu(hardware_disable, NULL, 0, 0);
+	hardware_disable(NULL);
 	return 0;
 }
 
 static int kvm_resume(struct sys_device *dev)
 {
-	on_each_cpu(hardware_disable, NULL, 0, 0);
+	hardware_enable(NULL);
 	return 0;
 }
 
-- 
1.5.2.2

[PATCH 20/20] KVM: Use CPU_DYING for disabling virtualization

[Avi Kivity]

Only at the CPU_DYING stage can we be sure that no user process will
be scheduled onto the cpu and oops when trying to use virtualization
extensions.

Signed-off-by: Avi Kivity <avi@qumranet.com>
---
 drivers/kvm/kvm_main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 54cad51..83bb284 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -3004,8 +3004,8 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val,
 	int cpu = (long)v;
 
 	switch (val) {
-	case CPU_DOWN_PREPARE:
-	case CPU_DOWN_PREPARE_FROZEN:
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
 	case CPU_UP_CANCELED:
 	case CPU_UP_CANCELED_FROZEN:
 		printk(KERN_INFO "kvm: disabling virtualization on CPU%d\n",
-- 
1.5.2.2