Skip to contentSkip to conversation

mmio/pio cleanup

PATCH Superseded Gregory Haskins 3 participants 7 messages May 27, 2009 – May 31, 2009

Subsystems (3)
x86 KVM KVM x86

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Gregory Haskins

author
#0 lore raw

v2 cover mmio/pio cleanup

This is v2 of the series, and applies to kvm.git/master:1a6a35a1)

This is in prep for some more substantial mmio/pio work for iosignalfd, coming later.

---

Gregory Haskins (3):
kvm: do not register i8254 PIO regions until we are initialized
kvm: cleanup io_device code
kvm: fix potential coalesced_mmio leak on shutdown

 arch/x86/kvm/i8254.c      |   53 +++++++++++++++++++++++++++++----------------
 arch/x86/kvm/i8259.c      |   20 ++++++++++++-----
 arch/x86/kvm/lapic.c      |   22 +++++++++++++------
 arch/x86/kvm/x86.c        |    2 +-
 virt/kvm/coalesced_mmio.c |   26 ++++++++++++++--------
 virt/kvm/ioapic.c         |   22 +++++++++++++------
 virt/kvm/iodev.h          |   25 +++++++++++++++------
 virt/kvm/kvm_main.c       |    2 +-
 8 files changed, 115 insertions(+), 57 deletions(-)
Signature · 1 line
Signature
author
#1 lore raw

PATCH v2 1/3 kvm: fix potential coalesced_mmio leak on shutdown

It would appear that we are invoking kfree() on the wrong pointer in the destructor for the coalesced_mmio device. This could result in a potential leak during shutdown. This works today because the kvm_io_device is the first element of the private structure, but this could change in the future, so lets clean this up.

Signed-off-by Gregory Haskins <[email protected]>

---

 virt/kvm/coalesced_mmio.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
virt/kvm/coalesced_mmio.c +4 -1
index 5ae620d..03ea280 100644
@@ -80,7 +80,10 @@ static void coalesced_mmio_write(struct kvm_io_device *this,
 
 static void coalesced_mmio_destructor(struct kvm_io_device *this)
 {
-	kfree(this);
+	struct kvm_coalesced_mmio_dev *dev =
+		(struct kvm_coalesced_mmio_dev *)this->private;
+
+	kfree(dev);
 }
 
 int kvm_coalesced_mmio_init(struct kvm *kvm)
author
#2 lore raw

PATCH v2 2/3 kvm: cleanup io_device code

In reply to Gregory Haskins

We modernize the io_device code so that we use container_of() instead of dev->private, and move the vtable to a separate ops structure (theoretically allows better caching for multiple instances of the same ops structure)

Signed-off-by Gregory Haskins <[email protected]>

---

 arch/x86/kvm/i8254.c      |   40 ++++++++++++++++++++++++++++------------
 arch/x86/kvm/i8259.c      |   20 ++++++++++++++------
 arch/x86/kvm/lapic.c      |   22 +++++++++++++++-------
 arch/x86/kvm/x86.c        |    2 +-
 virt/kvm/coalesced_mmio.c |   25 +++++++++++++++----------
 virt/kvm/ioapic.c         |   22 +++++++++++++++-------
 virt/kvm/iodev.h          |   25 ++++++++++++++++++-------
 virt/kvm/kvm_main.c       |    2 +-
 8 files changed, 107 insertions(+), 51 deletions(-)
arch/x86/kvm/i8254.c +28 -12
index 584e3d3..d6a2b7d 100644
@@ -347,10 +347,20 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val)
 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
 }
 
+static inline struct kvm_pit *dev_to_pit(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_pit, dev);
+}
+
+static inline struct kvm_pit *speaker_to_pit(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_pit, speaker_dev);
+}
+
 static void pit_ioport_write(struct kvm_io_device *this,
 			     gpa_t addr, int len, const void *data)
 {
-	struct kvm_pit *pit = (struct kvm_pit *)this->private;
+	struct kvm_pit *pit = dev_to_pit(this);
 	struct kvm_kpit_state *pit_state = &pit->pit_state;
 	struct kvm *kvm = pit->kvm;
 	int channel, access;

@@ -423,7 +433,7 @@ static void pit_ioport_write(struct kvm_io_device *this,
 static void pit_ioport_read(struct kvm_io_device *this,
 			    gpa_t addr, int len, void *data)
 {
-	struct kvm_pit *pit = (struct kvm_pit *)this->private;
+	struct kvm_pit *pit = dev_to_pit(this);
 	struct kvm_kpit_state *pit_state = &pit->pit_state;
 	struct kvm *kvm = pit->kvm;
 	int ret, count;

@@ -494,7 +504,7 @@ static int pit_in_range(struct kvm_io_device *this, gpa_t addr,
 static void speaker_ioport_write(struct kvm_io_device *this,
 				 gpa_t addr, int len, const void *data)
 {
-	struct kvm_pit *pit = (struct kvm_pit *)this->private;
+	struct kvm_pit *pit = speaker_to_pit(this);
 	struct kvm_kpit_state *pit_state = &pit->pit_state;
 	struct kvm *kvm = pit->kvm;
 	u32 val = *(u32 *) data;

@@ -508,7 +518,7 @@ static void speaker_ioport_write(struct kvm_io_device *this,
 static void speaker_ioport_read(struct kvm_io_device *this,
 				gpa_t addr, int len, void *data)
 {
-	struct kvm_pit *pit = (struct kvm_pit *)this->private;
+	struct kvm_pit *pit = speaker_to_pit(this);
 	struct kvm_kpit_state *pit_state = &pit->pit_state;
 	struct kvm *kvm = pit->kvm;
 	unsigned int refresh_clock;

@@ -560,6 +570,18 @@ static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask)
 	}
 }
 
+static struct kvm_io_device_ops pit_dev_ops = {
+	.read     = pit_ioport_read,
+	.write    = pit_ioport_write,
+	.in_range = pit_in_range,
+};
+
+static struct kvm_io_device_ops speaker_dev_ops = {
+	.read     = speaker_ioport_read,
+	.write    = speaker_ioport_write,
+	.in_range = speaker_in_range,
+};
+
 struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 {
 	struct kvm_pit *pit;

@@ -580,17 +602,11 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 	spin_lock_init(&pit->pit_state.inject_lock);
 
 	/* Initialize PIO device */
-	pit->dev.read = pit_ioport_read;
-	pit->dev.write = pit_ioport_write;
-	pit->dev.in_range = pit_in_range;
-	pit->dev.private = pit;
+	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
 	kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
 
 	if (flags & KVM_PIT_SPEAKER_DUMMY) {
-		pit->speaker_dev.read = speaker_ioport_read;
-		pit->speaker_dev.write = speaker_ioport_write;
-		pit->speaker_dev.in_range = speaker_in_range;
-		pit->speaker_dev.private = pit;
+		kvm_iodevice_init(&pit->speaker_dev, &speaker_dev_ops);
 		kvm_io_bus_register_dev(&kvm->pio_bus, &pit->speaker_dev);
 	}
arch/x86/kvm/i8259.c +14 -6
index 1ccb50c..63d9e5e 100644
@@ -444,10 +444,15 @@ static int picdev_in_range(struct kvm_io_device *this, gpa_t addr,
 	}
 }
 
+static inline struct kvm_pic *to_pic(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_pic, dev);
+}
+
 static void picdev_write(struct kvm_io_device *this,
 			 gpa_t addr, int len, const void *val)
 {
-	struct kvm_pic *s = this->private;
+	struct kvm_pic *s = to_pic(this);
 	unsigned char data = *(unsigned char *)val;
 
 	if (len != 1) {

@@ -474,7 +479,7 @@ static void picdev_write(struct kvm_io_device *this,
 static void picdev_read(struct kvm_io_device *this,
 			gpa_t addr, int len, void *val)
 {
-	struct kvm_pic *s = this->private;
+	struct kvm_pic *s = to_pic(this);
 	unsigned char data = 0;
 
 	if (len != 1) {

@@ -516,6 +521,12 @@ static void pic_irq_request(void *opaque, int level)
 	}
 }
 
+static struct kvm_io_device_ops picdev_ops = {
+	.read = picdev_read,
+	.write = picdev_write,
+	.in_range = picdev_in_range,
+};
+
 struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 {
 	struct kvm_pic *s;

@@ -534,10 +545,7 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 	/*
 	 * Initialize PIO device
 	 */
-	s->dev.read = picdev_read;
-	s->dev.write = picdev_write;
-	s->dev.in_range = picdev_in_range;
-	s->dev.private = s;
+	kvm_iodevice_init(&s->dev, &picdev_ops);
 	kvm_io_bus_register_dev(&kvm->pio_bus, &s->dev);
 	return s;
 }
arch/x86/kvm/lapic.c +15 -7
index ae99d83..496d2ad 100644
@@ -522,10 +522,15 @@ static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
 	return val;
 }
 
+static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_lapic, dev);
+}
+
 static void apic_mmio_read(struct kvm_io_device *this,
 			   gpa_t address, int len, void *data)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)this->private;
+	struct kvm_lapic *apic = to_lapic(this);
 	unsigned int offset = address - apic->base_address;
 	unsigned char alignment = offset & 0xf;
 	u32 result;

@@ -606,7 +611,7 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
 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;
+	struct kvm_lapic *apic = to_lapic(this);
 	unsigned int offset = address - apic->base_address;
 	unsigned char alignment = offset & 0xf;
 	u32 val;

@@ -723,7 +728,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
 static int apic_mmio_range(struct kvm_io_device *this, gpa_t addr,
 			   int len, int size)
 {
-	struct kvm_lapic *apic = (struct kvm_lapic *)this->private;
+	struct kvm_lapic *apic = to_lapic(this);
 	int ret = 0;
 
 

@@ -917,6 +922,12 @@ static struct kvm_timer_ops lapic_timer_ops = {
 	.is_periodic = lapic_is_periodic,
 };
 
+static struct kvm_io_device_ops apic_mmio_ops = {
+	.read     = apic_mmio_read,
+	.write    = apic_mmio_write,
+	.in_range = apic_mmio_range,
+};
+
 int kvm_create_lapic(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic;

@@ -951,10 +962,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 	vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE;
 
 	kvm_lapic_reset(vcpu);
-	apic->dev.read = apic_mmio_read;
-	apic->dev.write = apic_mmio_write;
-	apic->dev.in_range = apic_mmio_range;
-	apic->dev.private = apic;
+	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
 
 	return 0;
 nomem_free_apic:
arch/x86/kvm/x86.c +1 -1
index 6d44dd5..b54159e 100644
@@ -2227,7 +2227,7 @@ static struct kvm_io_device *vcpu_find_pervcpu_dev(struct kvm_vcpu *vcpu,
 
 	if (vcpu->arch.apic) {
 		dev = &vcpu->arch.apic->dev;
-		if (dev->in_range(dev, addr, len, is_write))
+		if (dev->ops->in_range(dev, addr, len, is_write))
 			return dev;
 	}
 	return NULL;
virt/kvm/coalesced_mmio.c +15 -10
index 03ea280..f61a1d4 100644
@@ -14,11 +14,15 @@
 
 #include "coalesced_mmio.h"
 
+static inline struct kvm_coalesced_mmio_dev *to_mmio(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_coalesced_mmio_dev, dev);
+}
+
 static int coalesced_mmio_in_range(struct kvm_io_device *this,
 				   gpa_t addr, int len, int is_write)
 {
-	struct kvm_coalesced_mmio_dev *dev =
-				(struct kvm_coalesced_mmio_dev*)this->private;
+	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
 	struct kvm_coalesced_mmio_zone *zone;
 	int next;
 	int i;

@@ -63,8 +67,7 @@ static int coalesced_mmio_in_range(struct kvm_io_device *this,
 static void coalesced_mmio_write(struct kvm_io_device *this,
 				 gpa_t addr, int len, const void *val)
 {
-	struct kvm_coalesced_mmio_dev *dev =
-				(struct kvm_coalesced_mmio_dev*)this->private;
+	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
 	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
 
 	/* kvm->lock must be taken by caller before call to in_range()*/

@@ -80,12 +83,17 @@ static void coalesced_mmio_write(struct kvm_io_device *this,
 
 static void coalesced_mmio_destructor(struct kvm_io_device *this)
 {
-	struct kvm_coalesced_mmio_dev *dev =
-		(struct kvm_coalesced_mmio_dev *)this->private;
+	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
 
 	kfree(dev);
 }
 
+static struct kvm_io_device_ops coalesced_mmio_ops = {
+	.write = coalesced_mmio_write,
+	.in_range = coalesced_mmio_in_range,
+	.destructor = coalesced_mmio_destructor,
+};
+
 int kvm_coalesced_mmio_init(struct kvm *kvm)
 {
 	struct kvm_coalesced_mmio_dev *dev;

@@ -93,10 +101,7 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
 	dev = kzalloc(sizeof(struct kvm_coalesced_mmio_dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
-	dev->dev.write  = coalesced_mmio_write;
-	dev->dev.in_range  = coalesced_mmio_in_range;
-	dev->dev.destructor  = coalesced_mmio_destructor;
-	dev->dev.private  = dev;
+	kvm_iodevice_init(&dev->dev, &coalesced_mmio_ops);
 	dev->kvm = kvm;
 	kvm->coalesced_mmio_dev = dev;
 	kvm_io_bus_register_dev(&kvm->mmio_bus, &dev->dev);
virt/kvm/ioapic.c +15 -7
index 1eddae9..0c17270 100644
@@ -220,10 +220,15 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode)
 			__kvm_ioapic_update_eoi(ioapic, i, trigger_mode);
 }
 
+static inline struct kvm_ioapic *to_ioapic(struct kvm_io_device *dev)
+{
+	return container_of(dev, struct kvm_ioapic, dev);
+}
+
 static int ioapic_in_range(struct kvm_io_device *this, gpa_t addr,
 			   int len, int is_write)
 {
-	struct kvm_ioapic *ioapic = (struct kvm_ioapic *)this->private;
+	struct kvm_ioapic *ioapic = to_ioapic(this);
 
 	return ((addr >= ioapic->base_address &&
 		 (addr < ioapic->base_address + IOAPIC_MEM_LENGTH)));

@@ -232,7 +237,7 @@ static int ioapic_in_range(struct kvm_io_device *this, gpa_t addr,
 static void ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 			     void *val)
 {
-	struct kvm_ioapic *ioapic = (struct kvm_ioapic *)this->private;
+	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 result;
 
 	ioapic_debug("addr %lx\n", (unsigned long)addr);

@@ -269,7 +274,7 @@ static void ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 static void ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 			      const void *val)
 {
-	struct kvm_ioapic *ioapic = (struct kvm_ioapic *)this->private;
+	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 data;
 
 	ioapic_debug("ioapic_mmio_write addr=%p len=%d val=%p\n",

@@ -314,6 +319,12 @@ void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
 	ioapic->id = 0;
 }
 
+static struct kvm_io_device_ops ioapic_mmio_ops = {
+	.read = ioapic_mmio_read,
+	.write = ioapic_mmio_write,
+	.in_range = ioapic_in_range,
+};
+
 int kvm_ioapic_init(struct kvm *kvm)
 {
 	struct kvm_ioapic *ioapic;

@@ -323,10 +334,7 @@ int kvm_ioapic_init(struct kvm *kvm)
 		return -ENOMEM;
 	kvm->arch.vioapic = ioapic;
 	kvm_ioapic_reset(ioapic);
-	ioapic->dev.read = ioapic_mmio_read;
-	ioapic->dev.write = ioapic_mmio_write;
-	ioapic->dev.in_range = ioapic_in_range;
-	ioapic->dev.private = ioapic;
+	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
 	ioapic->kvm = kvm;
 	kvm_io_bus_register_dev(&kvm->mmio_bus, &ioapic->dev);
 	return 0;
virt/kvm/iodev.h +18 -7
index 55e8846..0e1d60a 100644
@@ -18,7 +18,9 @@
 
 #include <linux/kvm_types.h>
 
-struct kvm_io_device {
+struct kvm_io_device;
+
+struct kvm_io_device_ops {
 	void (*read)(struct kvm_io_device *this,
 		     gpa_t addr,
 		     int len,

@@ -30,16 +32,25 @@ struct kvm_io_device {
 	int (*in_range)(struct kvm_io_device *this, gpa_t addr, int len,
 			int is_write);
 	void (*destructor)(struct kvm_io_device *this);
+};
+
 
-	void             *private;
+struct kvm_io_device {
+	struct kvm_io_device_ops *ops;
 };
 
+static inline void kvm_iodevice_init(struct kvm_io_device *dev,
+				     struct kvm_io_device_ops *ops)
+{
+	dev->ops = ops;
+}
+
 static inline void kvm_iodevice_read(struct kvm_io_device *dev,
 				     gpa_t addr,
 				     int len,
 				     void *val)
 {
-	dev->read(dev, addr, len, val);
+	dev->ops->read(dev, addr, len, val);
 }
 
 static inline void kvm_iodevice_write(struct kvm_io_device *dev,

@@ -47,19 +58,19 @@ static inline void kvm_iodevice_write(struct kvm_io_device *dev,
 				      int len,
 				      const void *val)
 {
-	dev->write(dev, addr, len, val);
+	dev->ops->write(dev, addr, len, val);
 }
 
 static inline int kvm_iodevice_inrange(struct kvm_io_device *dev,
 				       gpa_t addr, int len, int is_write)
 {
-	return dev->in_range(dev, addr, len, is_write);
+	return dev->ops->in_range(dev, addr, len, is_write);
 }
 
 static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
 {
-	if (dev->destructor)
-		dev->destructor(dev);
+	if (dev->ops->destructor)
+		dev->ops->destructor(dev);
 }
 
 #endif /* __KVM_IODEV_H__ */
virt/kvm/kvm_main.c +1 -1
index de042cb..ca2034a 100644
@@ -2456,7 +2456,7 @@ struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
 	for (i = 0; i < bus->dev_count; i++) {
 		struct kvm_io_device *pos = bus->devs[i];
 
-		if (pos->in_range(pos, addr, len, is_write))
+		if (kvm_iodevice_inrange(pos, addr, len, is_write))
 			return pos;
 	}
#3 lore raw

Re: [KVM PATCH v2 2/3] kvm: cleanup io_device code

Quoted 4 lines · * Gregory Haskins ([email protected]) wrote:

We modernize the io_device code so that we use container_of() instead of dev->private, and move the vtable to a separate ops structure (theoretically allows better caching for multiple instances of the same ops structure)

Looks like a nice cleanup. Couple minor nits.

arch/x86/kvm/i8254.c static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask) · line 583

Quoted 11 lines
arch/x86/kvm/i8254.c +11 -0
+static struct kvm_io_device_ops pit_dev_ops = {
+	.read     = pit_ioport_read,
+	.write    = pit_ioport_write,
+	.in_range = pit_in_range,
+};
+
+static struct kvm_io_device_ops speaker_dev_ops = {
+	.read     = speaker_ioport_read,
+	.write    = speaker_ioport_write,
+	.in_range = speaker_in_range,
+};

kvm_io_device_ops instances could be made const.

Quoted 24 lines
arch/x86/kvm/x86.c +1 -1
@@ -2227,7 +2227,7 @@ static struct kvm_io_device *vcpu_find_pervcpu_dev(struct kvm_vcpu *vcpu,
 
 	if (vcpu->arch.apic) {
 		dev = &vcpu->arch.apic->dev;
-		if (dev->in_range(dev, addr, len, is_write))
+		if (dev->ops->in_range(dev, addr, len, is_write))
 			return dev;
virt/kvm/iodev.h +3 -1
@@ -18,7 +18,9 @@
 
 #include <linux/kvm_types.h>
 
-struct kvm_io_device {
+struct kvm_io_device;
+
+struct kvm_io_device_ops {
 	void (*read)(struct kvm_io_device *this,
 		     gpa_t addr,
 		     int len,

virt/kvm/iodev.h struct kvm_io_device { · line 40

Quoted 10 lines
virt/kvm/iodev.h +4 -1
@@ -30,16 +32,25 @@ struct kvm_io_device {
 	int (*in_range)(struct kvm_io_device *this, gpa_t addr, int len,
 			int is_write);
 	void (*destructor)(struct kvm_io_device *this);
+};
+
 
-	void             *private;
+struct kvm_io_device {
+	struct kvm_io_device_ops *ops;
 };

Did you plan to extend kvm_io_device struct?

virt/kvm/iodev.h struct kvm_io_device { · line 46

Quoted 5 lines
virt/kvm/iodev.h +5 -0
+static inline void kvm_iodevice_init(struct kvm_io_device *dev,
+				     struct kvm_io_device_ops *ops)
+{
+	dev->ops = ops;
+}

And similarly, did you have a plan to do more with kvm_iodevice_init()? Otherwise looking a bit like overkill to me.

virt/kvm/kvm_main.c struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus, · line 2461

Quoted 10 lines
virt/kvm/kvm_main.c +1 -1
@@ -2456,7 +2456,7 @@ struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
 	for (i = 0; i < bus->dev_count; i++) {
 		struct kvm_io_device *pos = bus->devs[i];
 
-		if (pos->in_range(pos, addr, len, is_write))
+		if (kvm_iodevice_inrange(pos, addr, len, is_write))
 			return pos;
 	}

You converted this to the helper but not vcpu_find_pervcpu_dev() (not convinced it actually helps readability, but consistency is good). BTW, while there, s/kvm_iodevice_inrange/kvm_iodevice_in_range/ would be nice.

thanks,
-chris

author
#4 lore raw

Re: [KVM PATCH v2 2/3] kvm: cleanup io_device code

Quoted 11 lines · Chris Wright
Quoted 5 lines · * Gregory Haskins ([email protected]) wrote:

We modernize the io_device code so that we use container_of() instead of dev->private, and move the vtable to a separate ops structure (theoretically allows better caching for multiple instances of the same ops structure)

Looks like a nice cleanup. Couple minor nits.

Quoted 15 lines

arch/x86/kvm/i8254.c static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask) · line 583

Quoted 12 lines
arch/x86/kvm/i8254.c +11 -0
+static struct kvm_io_device_ops pit_dev_ops = {
+	.read     = pit_ioport_read,
+	.write    = pit_ioport_write,
+	.in_range = pit_in_range,
+};
+
+static struct kvm_io_device_ops speaker_dev_ops = {
+	.read     = speaker_ioport_read,
+	.write    = speaker_ioport_write,
+	.in_range = speaker_in_range,
+};

kvm_io_device_ops instances could be made const.

Ack

Quoted 42 lines
Quoted 28 lines
arch/x86/kvm/x86.c +1 -1
@@ -2227,7 +2227,7 @@ static struct kvm_io_device *vcpu_find_pervcpu_dev(struct kvm_vcpu *vcpu,
 
 	if (vcpu->arch.apic) {
 		dev = &vcpu->arch.apic->dev;
-		if (dev->in_range(dev, addr, len, is_write))
+		if (dev->ops->in_range(dev, addr, len, is_write))
 			return dev;
virt/kvm/iodev.h +3 -1
@@ -18,7 +18,9 @@
 
 #include <linux/kvm_types.h>
 
-struct kvm_io_device {
+struct kvm_io_device;
+
+struct kvm_io_device_ops {
 	void (*read)(struct kvm_io_device *this,
 		     gpa_t addr,
 		     int len,

@@ -30,16 +32,25 @@ struct kvm_io_device {
 	int (*in_range)(struct kvm_io_device *this, gpa_t addr, int len,

virt/kvm/iodev.h struct kvm_io_device { · line 41

Quoted 10 lines
virt/kvm/iodev.h +4 -1
 			int is_write);
 	void (*destructor)(struct kvm_io_device *this);
+};
+
 
-	void             *private;
+struct kvm_io_device {
+	struct kvm_io_device_ops *ops;
 };

Did you plan to extend kvm_io_device struct?

Quoted 10 lines

virt/kvm/iodev.h struct kvm_io_device { · line 55

Quoted 6 lines
virt/kvm/iodev.h +5 -0
+static inline void kvm_iodevice_init(struct kvm_io_device *dev,
+				     struct kvm_io_device_ops *ops)
+{
+	dev->ops = ops;
+}

And similarly, did you have a plan to do more with kvm_iodevice_init()? Otherwise looking a bit like overkill to me.

Yeah. As of right now my plan is to wait for Marcelo's lock cleanup to go in and integrate with that, and then convert the MMIO/PIO code to use RCU to acquire a reference to the io_device (so we run as fine-graned and lockless as possible). When that happens, you will see an atomic_t in the struct/init as well.

Even if that doesn't make the cut after review, I am thinking that we may be making the structure more complex in the future (for instance, to use a rbtree/hlist instead of the array, or to do tricks with caching the MRU device, etc.) and this will simplify that effort by already having all users call the abstracted init.

That said, we could just defer these hunks until needed. I just figured "while Im in here" but its nbd either way.

Quoted 7 lines
Quoted 7 lines
virt/kvm/kvm_main.c +0 -0
@@ -2456,7 +2456,7 @@ struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
 	for (i = 0; i < bus->dev_count; i++) {
 		struct kvm_io_device *pos = bus->devs[i];
Quoted 8 lines

virt/kvm/kvm_main.c struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus, · line 2462

Quoted 5 lines
virt/kvm/kvm_main.c +1 -1
-		if (pos->in_range(pos, addr, len, is_write))
+		if (kvm_iodevice_inrange(pos, addr, len, is_write))
 			return pos;
 	}

You converted this to the helper but not vcpu_find_pervcpu_dev() (not convinced it actually helps readability, but consistency is good).

Oops..oversight. Will fix.

Quoted 3 lines

BTW,
while there, s/kvm_iodevice_inrange/kvm_iodevice_in_range/ would be nice.

Yeah, good idea. Will fix.

Thanks Chris,
-Greg

author
#5 lore raw

PATCH v2 3/3 kvm: do not register i8254 PIO regions until we are initialized

In reply to Gregory Haskins

We currently publish the i8254 resources to the pio_bus before the devices are fully initialized. Since we hold the pit_lock, its probably not a real issue. But lets clean this up anyway.

Found-by Avi Kivity <[email protected]>
Signed-off-by Gregory Haskins <[email protected]>

---

 arch/x86/kvm/i8254.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)
arch/x86/kvm/i8254.c +8 -9
index d6a2b7d..0667700 100644
@@ -601,15 +601,6 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 	mutex_lock(&pit->pit_state.lock);
 	spin_lock_init(&pit->pit_state.inject_lock);
 
-	/* Initialize PIO device */
-	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
-	kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
-
-	if (flags & KVM_PIT_SPEAKER_DUMMY) {
-		kvm_iodevice_init(&pit->speaker_dev, &speaker_dev_ops);
-		kvm_io_bus_register_dev(&kvm->pio_bus, &pit->speaker_dev);
-	}
-
 	kvm->arch.vpit = pit;
 	pit->kvm = kvm;
 

@@ -628,6 +619,14 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 	pit->mask_notifier.func = pit_mask_notifer;
 	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
 
+	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
+	kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
+
+	if (flags & KVM_PIT_SPEAKER_DUMMY) {
+		kvm_iodevice_init(&pit->speaker_dev, &speaker_dev_ops);
+		kvm_io_bus_register_dev(&kvm->pio_bus, &pit->speaker_dev);
+	}
+
 	return pit;
 }
#6 lore raw
In reply to Gregory Haskins
Quoted 5 lines · Gregory Haskins

This is v2 of the series, and applies to kvm.git/master:1a6a35a1)

This is in prep for some more substantial mmio/pio work for iosignalfd, coming later.

Looks good, will apply once Chris' comments are addressed.

Signature · 1 line
error compiling committee.c: too many arguments to function