Support for in-kernel mmio handlers

PATCH In review Gregory Haskins 4 participants 17 messages Apr 4, 2007 – Apr 10, 2007 13 messages not in the archive KVM

lore

Share & tools

lore

Gregory Haskins

author

2007-04-04 20:42 UTC #0 lore raw

PATCH 1/1 Support for in-kernel mmio handlers

The MMIO registration code has been broken out as a new patch from the in-kernel APIC work with the following changes per Avi's request:

1) Supports dynamic registration
2) Uses gpa_t addresses
3) Explicit per-cpu mappings

In addition, I have added the concept of distinct VCPU and VM level registrations (where VCPU devices will eclipse competing VM registrations (if any). This will be key down the road where LAPICs should use VCPU registration, but IOAPICs should use VM level.

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

---
 drivers/kvm/kvm.h      |   50 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/kvm_main.c |   53 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 9 deletions(-)

drivers/kvm/kvm.h +50 -0

index fceeb84..3334730 100644
@@ -236,6 +236,54 @@ struct kvm_pio_request {
 	int rep;
 };
 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;
+	struct list_head  link;
+};
+
+/* 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 {
+	struct list_head list;
+};
+
+static inline void 
+kvm_io_bus_init(struct kvm_io_bus *bus)
+{
+	INIT_LIST_HEAD(&bus->list);
+}
+
+static inline struct kvm_io_device* 
+kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	struct kvm_io_device *pos = NULL;
+
+	list_for_each_entry(pos, &bus->list, link) {
+		if(pos->in_range(pos, addr))
+			return pos;
+	}
+
+	return NULL;
+}
+
+static inline void 
+kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	list_add_tail(&dev->link, &bus->list);
+}
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -294,6 +342,7 @@ struct kvm_vcpu {
 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;
 
 	int sigset_active;
 	sigset_t sigset;

@@ -345,6 +394,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct kvm_stat {

drivers/kvm/kvm_main.c +44 -9

98 lines not on this page · raw message

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

A message that is not in the archive (sent off-list or not yet received): [email protected]

Chris Wright

2007-04-04 22:48 UTC #2 lore raw

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

The MMIO registration code has been broken out as a new patch from the in-kernel APIC work with the following changes per Avi's request:
1) Supports dynamic registration
2) Uses gpa_t addresses
3) Explicit per-cpu mappings
In addition, I have added the concept of distinct VCPU and VM level registrations (where VCPU devices will eclipse competing VM registrations (if any). This will be key down the road where LAPICs should use VCPU registration, but IOAPICs should use VM level.

hmm, i'm surprised it makes a difference.

Quoted 16 lines

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

---
 drivers/kvm/kvm.h      |   50 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/kvm_main.c |   53 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 9 deletions(-)

drivers/kvm/kvm.h +1 -0

index fceeb84..3334730 100644
@@ -236,6 +236,54 @@ struct kvm_pio_request {
 	int rep;
 };
 
+struct kvm_io_device {

drivers/kvm/kvm.h struct kvm_pio_request { · line 249

Quoted 10 lines

drivers/kvm/kvm.h +10 -0

+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;

This looks unused, what is it meant for?

Quoted 15 lines

drivers/kvm/kvm.h +15 -0

+	struct list_head  link;
+};
+
+/* 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 {
+	struct list_head list;
+};
+
+static inline void 
+kvm_io_bus_init(struct kvm_io_bus *bus)
+{

drivers/kvm/kvm.h struct kvm_pio_request { · line 274

Quoted 10 lines

drivers/kvm/kvm.h +10 -0

+	INIT_LIST_HEAD(&bus->list);
+}
+
+static inline struct kvm_io_device* 
+kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	struct kvm_io_device *pos = NULL;
+
+	list_for_each_entry(pos, &bus->list, link) {
+		if(pos->in_range(pos, addr))

linux style nit, missing space after if --> if (pos->in_range(pos, addr))

Quoted 36 lines

drivers/kvm/kvm.h +14 -0

+			return pos;
+	}
+
+	return NULL;
+}
+
+static inline void 
+kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	list_add_tail(&dev->link, &bus->list);
+}
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -294,6 +342,7 @@ struct kvm_vcpu {
 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;
 
 	int sigset_active;
 	sigset_t sigset;

@@ -345,6 +394,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct kvm_stat {

drivers/kvm/kvm_main.c +0 -0

index 4473174..da119c0 100644

drivers/kvm/kvm_main.c static struct kvm *kvm_create_vm(void) · line 297

Quoted 4 lines

drivers/kvm/kvm_main.c +1 -0

@@ -294,6 +294,7 @@ static struct kvm *kvm_create_vm(void)
 
 	spin_lock_init(&kvm->lock);
 	INIT_LIST_HEAD(&kvm->active_mmu_pages);
+	kvm_io_bus_init(&kvm->mmio_bus);

I'd just do INIT_LIST_HEAD, unless you havd bigger plans for this wrapper?

drivers/kvm/kvm_main.c static struct kvm *kvm_create_vm(void) · line 306

Quoted 8 lines

drivers/kvm/kvm_main.c +1 -0

 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		struct kvm_vcpu *vcpu = &kvm->vcpus[i];

@@ -302,6 +303,7 @@ static struct kvm *kvm_create_vm(void)
 		vcpu->kvm = kvm;
 		vcpu->mmu.root_hpa = INVALID_PAGE;
 		INIT_LIST_HEAD(&vcpu->free_pages);
+		kvm_io_bus_init(&vcpu->mmio_bus);

ditto

Quoted 5 lines

drivers/kvm/kvm_main.c +0 -0

 		spin_lock(&kvm_lock);
 		list_add(&kvm->vm_list, &vm_list);
 		spin_unlock(&kvm_lock);

@@ -1015,12 +1017,30 @@ static int emulator_write_std(unsigned long addr,
 	return X86EMUL_UNHANDLEABLE;

drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr, · line 1027

Quoted 10 lines

drivers/kvm/kvm_main.c +8 -0

 }
 
+static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
+						gpa_t addr)
+{
+	struct kvm_io_device *mmio_dev;
+
+	/* First check the local CPU addresses */
+	mmio_dev = kvm_io_bus_find_dev(&vcpu->mmio_bus, addr);
+	if(!mmio_dev) {

same style nit. and why do you have local vs global check (or dynamic registration for that matter)?

Quoted 28 lines

drivers/kvm/kvm_main.c +10 -4

+		/* Then check the entire VM */
+		mmio_dev = kvm_io_bus_find_dev(&vcpu->kvm->mmio_bus, addr);
+	}
+	
+	return mmio_dev;
+}
+
 static int emulator_read_emulated(unsigned long addr,
 				  unsigned long *val,
 				  unsigned int bytes,
 				  struct x86_emulate_ctxt *ctxt)
 {
 	struct kvm_vcpu *vcpu = ctxt->vcpu;
+	gpa_t gpa;
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (vcpu->mmio_read_completed) {
 		memcpy(val, vcpu->mmio_data, bytes);

@@ -1029,18 +1049,24 @@ 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;

drivers/kvm/kvm_main.c static int emulator_read_emulated(unsigned long addr, · line 1057

Quoted 10 lines

drivers/kvm/kvm_main.c +4 -5

-		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 vcpu_printf(vcpu, "not present\n"), X86EMUL_PROPAGATE_FAULT;
 
-		return X86EMUL_UNHANDLEABLE;
+	/* Is thie MMIO handled locally? */

s/thie/this/

drivers/kvm/kvm_main.c static int emulator_read_emulated(unsigned long addr, · line 1059

Quoted 2 lines

drivers/kvm/kvm_main.c +2 -0

+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {

style

Quoted 23 lines

drivers/kvm/kvm_main.c +11 -0

+		*val = mmio_dev->read(mmio_dev, gpa, bytes);
+		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,

@@ -1070,6 +1096,8 @@ static int emulator_write_emulated(unsigned long addr,
 {
 	struct kvm_vcpu *vcpu = ctxt->vcpu;
 	gpa_t gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (gpa == UNMAPPED_GVA)
 		return X86EMUL_PROPAGATE_FAULT;

drivers/kvm/kvm_main.c static int emulator_write_emulated(unsigned long addr, · line 1108

Quoted 4 lines

drivers/kvm/kvm_main.c +1 -0

@@ -1077,6 +1105,13 @@ static int emulator_write_emulated(unsigned long addr,
 	if (emulator_write_phys(vcpu, gpa, val, bytes))
 		return X86EMUL_CONTINUE;
 
+	/* Is thie MMIO handled locally? */

s/thie/this/

drivers/kvm/kvm_main.c static int emulator_write_emulated(unsigned long addr, · line 1110

Quoted 2 lines

drivers/kvm/kvm_main.c +2 -0

+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {

style

drivers/kvm/kvm_main.c static int emulator_write_emulated(unsigned long addr, · line 1117

Quoted 7 lines

drivers/kvm/kvm_main.c +4 -0

+		mmio_dev->write(mmio_dev, gpa, bytes, val);
+		return X86EMUL_CONTINUE;
+	}
+
 	vcpu->mmio_needed = 1;
 	vcpu->mmio_phys_addr = gpa;
 	vcpu->mmio_size = bytes;

A message that is not in the archive (sent off-list or not yet received): 20070404224806.GA15078-JyIX8gxvWYPr2PDY2+4mTGD2FQJk+8+b@public.gmane.org

Gregory Haskins

author

2007-04-04 23:04 UTC #4 lore raw

Hi Chris,
Thanks for the feedback. Ive answered inline below.

Quoted 1 line

Quoted 1 line
Quoted 1 line
On Wed, Apr 4, 2007 at 6:48 PM, in message

<20070404224806.GA15078-JyIX8gxvWYPr2PDY2+4mTGD2FQJk+8+b@public.gmane.org>, Chris Wright

Quoted 8 lines · <[email protected]> wrote:

Quoted 1 line · * Gregory Haskins ([email protected]) wrote:
The MMIO registration code has been broken out as a new patch from the
in- kernel APIC work with the following changes per Avi's request:
Quoted 5 lines
1) Supports dynamic registration
2) Uses gpa_t addresses
3) Explicit per- cpu mappings

Quoted 6 lines

Quoted 1 line
In addition, I have added the concept of distinct VCPU and VM level
registrations (where VCPU devices will eclipse competing VM registrations (if any). This will be key down the road where LAPICs should use VCPU registration, but IOAPICs should use VM level.
hmm, i'm surprised it makes a difference.

LAPICs can be remapped on a per-cpu basis via an MSR, whereas something like an IOAPIC is a system-wide resource.

Quoted 16 lines

Quoted 16 lines
Signed- off- by: Gregory Haskins <[email protected]>
---
 drivers/kvm/kvm.h      |   50 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/kvm_main.c |   53 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 9 deletions(- )
diff -- git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h index fceeb84..3334730 100644
drivers/kvm/kvm.h +0 -0
@@ - 236,6 +236,54 @@ struct kvm_pio_request {
int rep;
};

Quoted 13 lines

drivers/kvm/kvm.h struct kvm_pio_request { · line 249

Quoted 11 lines

drivers/kvm/kvm.h +11 -0

+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;

This looks unused, what is it meant for?

Its unused in this patch, because the primary consumer is a follow on patch that is not yet released. The original patch had this logic + the logic that used it all together and it was requested to break them apart.

Quoted 21 lines

drivers/kvm/kvm.h struct kvm_pio_request {

Quoted 6 lines

drivers/kvm/kvm.h +5 -0

+	struct list_head  link;
+};
+
+/* It would be nice to use something smarter than a linear search, TBD...
+   Thankfully we dont expect many devices to register (famous last words

:),

Quoted 14 lines

drivers/kvm/kvm.h +14 -0

+   so until then it will suffice.  At least its abstracted so we can change
+   in one place.
+ */
+struct kvm_io_bus {
+	struct list_head list;
+};
+
+static inline void 
+kvm_io_bus_init(struct kvm_io_bus *bus)
+{
+	INIT_LIST_HEAD(&bus- >list);
+}
+
+static inline struct kvm_io_device*

Quoted 8 lines

drivers/kvm/kvm.h struct kvm_pio_request {
Quoted 6 lines
drivers/kvm/kvm.h +6 -0
+kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	struct kvm_io_device *pos = NULL;
+
+	list_for_each_entry(pos, &bus- >list, link) {
+		if(pos- >in_range(pos, addr))
linux style nit, missing space after if -- > if (pos- >in_range(pos, addr))

Yeah, old habits die hard ;) I will fix all of these.

Quoted 38 lines

Quoted 38 lines
drivers/kvm/kvm.h +12 -0
+			return pos;
+	}
+
+	return NULL;
+}
+
+static inline void 
+kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	list_add_tail(&dev- >link, &bus- >list);
+}
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {
@@ - 294,6 +342,7 @@ struct kvm_vcpu {
drivers/kvm/kvm.h +1 -0
 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;
 
 	int sigset_active;
 	sigset_t sigset;
@@ - 345,6 +394,7 @@ struct kvm {
drivers/kvm/kvm.h +1 -0
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct kvm_stat {
diff -- git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c index 4473174..da119c0 100644
drivers/kvm/kvm_main.c +0 -0
@@ - 294,6 +294,7 @@ static struct kvm *kvm_create_vm(void)

Quoted 5 lines

drivers/kvm/kvm_main.c
Quoted 3 lines
drivers/kvm/kvm_main.c +1 -0
 	spin_lock_init(&kvm- >lock);
 	INIT_LIST_HEAD(&kvm- >active_mmu_pages);
+	kvm_io_bus_init(&kvm- >mmio_bus);
I'd just do INIT_LIST_HEAD, unless you havd bigger plans for this wrapper?

The motivation for wrapping the init is because I want to abstract the fact that its a list. This means I can update the mechanism to do something more intelligent with address lookup (e.g. b-tree, etc) without changing code all over the place. Right now there are only two consumers, put I envision there will be some more. For instance, I would like to get PIOs using this mechanism at some point (so I can snarf accesses to the 8259s at 0x20/0xa0)

Quoted 19 lines

drivers/kvm/kvm_main.c
Quoted 9 lines
drivers/kvm/kvm_main.c +0 -0
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		struct kvm_vcpu *vcpu = &kvm- >vcpus[i];
@@ - 302,6 +303,7 @@ static struct kvm *kvm_create_vm(void)
drivers/kvm/kvm_main.c +1 -0
 		vcpu- >kvm = kvm;
 		vcpu- >mmu.root_hpa = INVALID_PAGE;
 		INIT_LIST_HEAD(&vcpu- >free_pages);
+		kvm_io_bus_init(&vcpu- >mmio_bus);
ditto
Quoted 7 lines
drivers/kvm/kvm_main.c +0 -0
 		spin_lock(&kvm_lock);
 		list_add(&kvm- >vm_list, &vm_list);
 		spin_unlock(&kvm_lock);
@@ - 1015,12 +1017,30 @@ static int emulator_write_std(unsigned long addr,
return X86EMUL_UNHANDLEABLE;
}

Quoted 10 lines

drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr, · line 1027
Quoted 8 lines
drivers/kvm/kvm_main.c +8 -0
+static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
+						gpa_t addr)
+{
+	struct kvm_io_device *mmio_dev;
+
+	/* First check the local CPU addresses */
+	mmio_dev = kvm_io_bus_find_dev(&vcpu- >mmio_bus, addr);
+	if(!mmio_dev) {
same style nit.

ack

Quoted 1 line

and why do you have local vs global check

see my first comment: re LAPIC is a relocateble per-cpu resource, IOAPIC is not

Quoted 1 line

(or dynamic registration for that matter)?

the xAPIC support will be optional, so the ability to register for MMIO handing has to be dynamic.

Quoted 37 lines

drivers/kvm/kvm_main.c +10 -0

+		/* Then check the entire VM */
+		mmio_dev = kvm_io_bus_find_dev(&vcpu- >kvm- >mmio_bus, addr);
+	}
+	
+	return mmio_dev;
+}
+
 static int emulator_read_emulated(unsigned long addr,
 				  unsigned long *val,
 				  unsigned int bytes,
 				  struct x86_emulate_ctxt *ctxt)
 {
 	struct kvm_vcpu *vcpu = ctxt- >vcpu;
+	gpa_t gpa;
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (vcpu- >mmio_read_completed) {
 		memcpy(val, vcpu- >mmio_data, bytes);

@@ - 1029,18 +1049,24 @@ static int emulator_read_emulated(unsigned long addr,

drivers/kvm/kvm_main.c +3 -8

 	} 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 vcpu_printf(vcpu, "not present\n"), X86EMUL_PROPAGATE_FAULT;

Quoted 4 lines

drivers/kvm/kvm_main.c static int emulator_read_emulated(unsigned long addr, · line 1057
Quoted 2 lines
drivers/kvm/kvm_main.c +1 -1
-		return X86EMUL_UNHANDLEABLE;
+	/* Is thie MMIO handled locally? */
s/thie/this/

ack

Quoted 5 lines

drivers/kvm/kvm_main.c
Quoted 3 lines
drivers/kvm/kvm_main.c +2 -0
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {
style

ack

Quoted 27 lines

drivers/kvm/kvm_main.c +9 -0

+		*val = mmio_dev- >read(mmio_dev, gpa, bytes);
+		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,

@@ - 1070,6 +1096,8 @@ static int emulator_write_emulated(unsigned long addr,
{

drivers/kvm/kvm_main.c +2 -0

 	struct kvm_vcpu *vcpu = ctxt- >vcpu;
 	gpa_t gpa = vcpu- >mmu.gva_to_gpa(vcpu, addr);
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (gpa == UNMAPPED_GVA)
 		return X86EMUL_PROPAGATE_FAULT;

@@ - 1077,6 +1105,13 @@ static int emulator_write_emulated(unsigned long addr,

drivers/kvm/kvm_main.c +0 -0

 	if (emulator_write_phys(vcpu, gpa, val, bytes))
 		return X86EMUL_CONTINUE;

Quoted 3 lines

drivers/kvm/kvm_main.c
Quoted 1 line
drivers/kvm/kvm_main.c +1 -0
+	/* Is thie MMIO handled locally? */
s/thie/this/

ack

Quoted 5 lines

drivers/kvm/kvm_main.c
Quoted 3 lines
drivers/kvm/kvm_main.c +2 -0
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {
style

ack

Quoted 8 lines

drivers/kvm/kvm_main.c

Quoted 8 lines

drivers/kvm/kvm_main.c +4 -0

+		mmio_dev- >write(mmio_dev, gpa, bytes, val);
+		return X86EMUL_CONTINUE;
+	}
+
 	vcpu- >mmio_needed = 1;
 	vcpu- >mmio_phys_addr = gpa;
 	vcpu- >mmio_size = bytes;

Regards,
-Greg

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20070405001021.GV10574-JyIX8gxvWYPr2PDY2+4mTGD2FQJk+8+b@public.gmane.org

Gregory Haskins

author

2007-04-05 00:21 UTC #7 lore raw

The attachment contains fixes based on the feedback from Chris.

Thanks Chris!

Regards,
-Greg

drivers/kvm/kvm.h +50 -0

index fceeb84..0e6eb04 100644
@@ -236,6 +236,54 @@ struct kvm_pio_request {
 	int rep;
 };
 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;
+	struct list_head  link;
+};
+
+/* 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 {
+	struct list_head list;
+};
+
+static inline void 
+kvm_io_bus_init(struct kvm_io_bus *bus)
+{
+	INIT_LIST_HEAD(&bus->list);
+}
+
+static inline struct kvm_io_device* 
+kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	struct kvm_io_device *pos = NULL;
+
+	list_for_each_entry(pos, &bus->list, link) {
+		if (pos->in_range(pos, addr))
+			return pos;
+	}
+
+	return NULL;
+}
+
+static inline void 
+kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	list_add_tail(&dev->link, &bus->list);
+}
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -294,6 +342,7 @@ struct kvm_vcpu {
 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;
 
 	int sigset_active;
 	sigset_t sigset;

@@ -345,6 +394,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct kvm_stat {

drivers/kvm/kvm_main.c +44 -9

98 lines not on this page · raw message

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Chris Wright

2007-04-05 00:49 UTC #9 lore raw

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

LAPICs can be remapped on a per-cpu basis via an MSR, whereas something like an IOAPIC is a system-wide resource.

Yes, I see now, no vcpu in kvm_io_device callbacks' context (admittedly, I'm used to the Xen implementation ;-)

Quoted 14 lines

Quoted 5 lines

drivers/kvm/kvm.h +5 -0

+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,

Quoted 8 lines

drivers/kvm/kvm.h struct kvm_pio_request { · line 249
Quoted 6 lines
drivers/kvm/kvm.h +6 -0
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;
This looks unused, what is it meant for?

Quoted 3 lines

Its unused in this patch, because the primary consumer is a follow on patch that is not yet released. The original patch had this logic + the logic that used it all together and it was requested to break them apart.

Makes sense, I'll wait to see a user to understand how it'w used.

Quoted 9 lines

Quoted 8 lines
Review
Quoted 6 lines
+++ b/drivers/kvm/kvm_main.c
@@ - 294,6 +294,7 @@ static struct kvm *kvm_create_vm(void)
(quoted diff) +1 -0
 	spin_lock_init(&kvm- >lock);
 	INIT_LIST_HEAD(&kvm- >active_mmu_pages);
+	kvm_io_bus_init(&kvm- >mmio_bus);
I'd just do INIT_LIST_HEAD, unless you havd bigger plans for this wrapper?

Quoted 7 lines

The motivation for wrapping the init is because I want to abstract
the fact that its a list. This means I can update the mechanism to
do something more intelligent with address lookup (e.g. b-tree, etc) without changing code all over the place. Right now there are only two consumers, put I envision there will be some more. For instance, I would like to get PIOs using this mechanism at some point (so I can snarf accesses to the 8259s at 0x20/0xa0)

Right, you even alluded to that in your comments. I didn't expect a list to really become that long where it needed a more complex data structure.

thanks,
-chris

Avi Kivity

2007-04-05 07:07 UTC #10 lore raw

In reply to a message not in the archive

Quoted 22 lines · Gregory Haskins

The MMIO registration code has been broken out as a new patch from the in-kernel APIC work with the following changes per Avi's request:
1) Supports dynamic registration
2) Uses gpa_t addresses
3) Explicit per-cpu mappings
In addition, I have added the concept of distinct VCPU and VM level registrations (where VCPU devices will eclipse competing VM registrations (if any). This will be key down the road where LAPICs should use VCPU registration, but IOAPICs should use VM level.
Signed-off-by Gregory Haskins <[email protected]>
---
 drivers/kvm/kvm.h      |   50 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/kvm_main.c |   53 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 9 deletions(-)
drivers/kvm/kvm.h +0 -0
index fceeb84..3334730 100644
@@ -236,6 +236,54 @@ struct kvm_pio_request {
 	int rep;
 };

drivers/kvm/kvm.h struct kvm_pio_request { · line 247

Quoted 10 lines

drivers/kvm/kvm.h +8 -0

 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);

length could be just an int.

drivers/kvm/kvm.h struct kvm_pio_request { · line 249

Quoted 2 lines

drivers/kvm/kvm.h +1 -0

+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);

Do you see any reason to have this as a callback and not a pair of gpas?

drivers/kvm/kvm.h struct kvm_pio_request {

Quoted 4 lines

drivers/kvm/kvm.h +3 -0

+
+	void             *private;
+	struct list_head  link;

Having these in an array would be much more efficient. A fixed size array of moderate size should suffice.

drivers/kvm/kvm.h struct kvm_pio_request { · line 257

Quoted 8 lines

drivers/kvm/kvm.h +7 -0

+};
+
+/* 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.
+ */

/*
* kernel comments look
* like this
*/

drivers/kvm/kvm.h struct kvm_pio_request { · line 263

Quoted 7 lines

drivers/kvm/kvm.h +6 -0

+struct kvm_io_bus {
+	struct list_head list;
+};
+
+static inline void 
+kvm_io_bus_init(struct kvm_io_bus *bus)

function declarations on one line please.

drivers/kvm/kvm.h struct kvm_pio_request { · line 268

Quoted 6 lines

drivers/kvm/kvm.h +5 -0

+{
+	INIT_LIST_HEAD(&bus->list);
+}
+
+static inline struct kvm_io_device*

C style pointers:
struct blah *somthing();

drivers/kvm/kvm.h struct kvm_pio_request { · line 276

Quoted 9 lines

drivers/kvm/kvm.h +8 -0

+kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	struct kvm_io_device *pos = NULL;
+
+	list_for_each_entry(pos, &bus->list, link) {
+		if(pos->in_range(pos, addr))
+			return pos;
+	}

space after if. avoid redundant {}. Have Documentaion/CodingStyle tattooed somewhere easily accessible.

Quoted 22 lines

drivers/kvm/kvm.h +10 -0

+
+	return NULL;
+}
+
+static inline void 
+kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	list_add_tail(&dev->link, &bus->list);
+}
+

struct kvm_vcpu {

drivers/kvm/kvm.h +1 -0

 	struct kvm *kvm;
 	union {

@@ -294,6 +342,7 @@ struct kvm_vcpu {
 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;
 
 	int sigset_active;
 	sigset_t sigset;

drivers/kvm/kvm.h struct kvm { · line 398

Quoted 5 lines

drivers/kvm/kvm.h +1 -0

@@ -345,6 +394,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;

The per-vcpu I/O bus is special in that it has exactly one component, and one which can change its address. I think we can special case it and just check for apic addresses explicitly when searching the bus.

Quoted 29 lines

drivers/kvm/kvm.h +0 -0

};

struct kvm_stat {

drivers/kvm/kvm_main.c +4 -0

index 4473174..da119c0 100644
@@ -294,6 +294,7 @@ static struct kvm *kvm_create_vm(void)
 
 	spin_lock_init(&kvm->lock);
 	INIT_LIST_HEAD(&kvm->active_mmu_pages);
+	kvm_io_bus_init(&kvm->mmio_bus);
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		struct kvm_vcpu *vcpu = &kvm->vcpus[i];

@@ -302,6 +303,7 @@ static struct kvm *kvm_create_vm(void)
 		vcpu->kvm = kvm;
 		vcpu->mmu.root_hpa = INVALID_PAGE;
 		INIT_LIST_HEAD(&vcpu->free_pages);
+		kvm_io_bus_init(&vcpu->mmio_bus);
 		spin_lock(&kvm_lock);
 		list_add(&kvm->vm_list, &vm_list);
 		spin_unlock(&kvm_lock);

@@ -1015,12 +1017,30 @@ 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)

drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr, · line 1031

Quoted 10 lines

drivers/kvm/kvm_main.c +9 -0

+{
+	struct kvm_io_device *mmio_dev;
+
+	/* First check the local CPU addresses */
+	mmio_dev = kvm_io_bus_find_dev(&vcpu->mmio_bus, addr);
+	if(!mmio_dev) {
+		/* Then check the entire VM */
+		mmio_dev = kvm_io_bus_find_dev(&vcpu->kvm->mmio_bus, addr);
+	}

space, comment, braces

Quoted 23 lines

drivers/kvm/kvm_main.c +7 -0

+	
+	return mmio_dev;
+}
+
 static int emulator_read_emulated(unsigned long addr,
 				  unsigned long *val,
 				  unsigned int bytes,
 				  struct x86_emulate_ctxt *ctxt)
 {
 	struct kvm_vcpu *vcpu = ctxt->vcpu;
+	gpa_t gpa;
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (vcpu->mmio_read_completed) {

drivers/kvm/kvm_main.c +0 -2

 		memcpy(val, vcpu->mmio_data, bytes);

@@ -1029,18 +1049,24 @@ 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);

drivers/kvm/kvm_main.c static int emulator_read_emulated(unsigned long addr, · line 1056

Quoted 10 lines

drivers/kvm/kvm_main.c +3 -6

-		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 vcpu_printf(vcpu, "not present\n"), X86EMUL_PROPAGATE_FAULT;

The vcpu_printf() snuck in somehow.

Quoted 28 lines

drivers/kvm/kvm_main.c +14 -1

-		return X86EMUL_UNHANDLEABLE;
+	/* Is thie MMIO handled locally? */
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {
+		*val = mmio_dev->read(mmio_dev, gpa, bytes);
+		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,

@@ -1070,6 +1096,8 @@ static int emulator_write_emulated(unsigned long addr,
 {
 	struct kvm_vcpu *vcpu = ctxt->vcpu;
 	gpa_t gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (gpa == UNMAPPED_GVA)
 		return X86EMUL_PROPAGATE_FAULT;

drivers/kvm/kvm_main.c static int emulator_write_emulated(unsigned long addr, · line 1109

Quoted 5 lines

drivers/kvm/kvm_main.c +1 -0

@@ -1077,6 +1105,13 @@ static int emulator_write_emulated(unsigned long addr,
 	if (emulator_write_phys(vcpu, gpa, val, bytes))
 		return X86EMUL_CONTINUE;
 
+	/* Is thie MMIO handled locally? */

spelling

Quoted 8 lines

drivers/kvm/kvm_main.c +6 -0

+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {
+		mmio_dev->write(mmio_dev, gpa, bytes, val);
+		return X86EMUL_CONTINUE;
+	}
+
 	vcpu->mmio_needed = 1;
 	vcpu->mmio_phys_addr = gpa;

Quoted 4 lines

vcpu->mmio_size = bytes;

Please fix and *test*. Boot at least 32-bit Windows with ACPI HAL and 64-bit Linux, the more the better of course.

Signature · 8 lines

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

A message that is not in the archive (sent off-list or not yet received): 4614A03C.2050707-atKUWr5tajBWk0Htik3J/[email protected]

Dor Laor

2007-04-05 07:29 UTC #12 lore raw

Quoted 6 lines

Quoted 4 lines
Signed-off-by Gregory Haskins <[email protected]>
---
 drivers/kvm/kvm.h      |   50
+++++++++++++++++++++++++++++++++++++++++++++
Quoted 1 line
 drivers/kvm/kvm_main.c |   53

+++++++++++++++++++++++++++++++++++++++---

Quoted 23 lines

-----

Quoted 9 lines

 2 files changed, 94 insertions(+), 9 deletions(-)

drivers/kvm/kvm.h +0 -0

index fceeb84..3334730 100644
@@ -236,6 +236,54 @@ struct kvm_pio_request {
 	int rep;
 };

drivers/kvm/kvm.h struct kvm_pio_request { · line 247

Quoted 10 lines

drivers/kvm/kvm.h +8 -0

 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);

length could be just an int.

Quoted 4 lines

drivers/kvm/kvm.h struct kvm_pio_request { · line 249
Quoted 2 lines
drivers/kvm/kvm.h +1 -0
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
 
Do you see any reason to have this as a callback and not a pair of

gpas?

This way a device can register for a range that contains holes, this would allow partial implementation in the kernel and part in user space. Might be usefull for some VT optimization support (haven't though of a specific one yet, but there's a good chance to be used.)

Quoted 8 lines

drivers/kvm/kvm.h struct kvm_pio_request {
Quoted 5 lines
drivers/kvm/kvm.h +3 -0
+
+	void             *private;
+	struct list_head  link;
 
Having these in an array would be much more efficient. A fixed size array of moderate size should suffice.

You can take it from my old apic code.

Quoted 1 line

Gregory Haskins

author

2007-04-05 14:58 UTC #13 lore raw

In reply to a message not in the archive

Hi Avi,

I have addressed your comments and re-attached the fixed up patch. Most of the things you suggested I implemented, but a few I didnt so I will comment inline...

Quoted 1 line

Quoted 1 line
Quoted 1 line
On Thu, Apr 5, 2007 at 3:07 AM, in message <4614A03C.2050707-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 28 lines · Avi Kivity

Quoted 1 line · Gregory Haskins
The MMIO registration code has been broken out as a new patch from the
in- kernel APIC work with the following changes per Avi's request:
Quoted 6 lines
1) Supports dynamic registration
2) Uses gpa_t addresses
3) Explicit per- cpu mappings
In addition, I have added the concept of distinct VCPU and VM level
registrations (where VCPU devices will eclipse competing VM registrations (if any). This will be key down the road where LAPICs should use VCPU registration, but IOAPICs should use VM level.
Quoted 16 lines
Signed- off- by: Gregory Haskins <[email protected]>
---
 drivers/kvm/kvm.h      |   50 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/kvm/kvm_main.c |   53 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 9 deletions(- )
diff -- git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h index fceeb84..3334730 100644
drivers/kvm/kvm.h +0 -0
@@ - 236,6 +236,54 @@ struct kvm_pio_request {
int rep;
};

Quoted 11 lines

drivers/kvm/kvm.h

Quoted 9 lines

drivers/kvm/kvm.h +8 -0

+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      unsigned long length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      unsigned long length,
+		      unsigned long val);

length could be just an int.

Done

Quoted 5 lines

drivers/kvm/kvm.h
Quoted 3 lines
drivers/kvm/kvm.h +1 -0
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
Do you see any reason to have this as a callback and not a pair of gpas?

I believe Dor replied earlier stating the reason of being able to support holes. Another reason that I can think of that I particularly like about this design (which I am not claiming as my own) is that the device can relocate (e.g. LAPIC base addr) without worrying about reprogramming the bus.

Quoted 8 lines

drivers/kvm/kvm.h
Quoted 5 lines
drivers/kvm/kvm.h +3 -0
+
+	void             *private;
+	struct list_head  link;
Having these in an array would be much more efficient. A fixed size array of moderate size should suffice.

Done. Maximum # devices is currently 6, because anything beyond that and I think we need to revisit the linear alg ;)

Quoted 11 lines

drivers/kvm/kvm.h

Quoted 5 lines

drivers/kvm/kvm.h +4 -0

+};
+
+/* It would be nice to use something smarter than a linear search, TBD...
+   Thankfully we dont expect many devices to register (famous last words

:),

drivers/kvm/kvm.h

Quoted 4 lines

drivers/kvm/kvm.h +3 -0

+   so until then it will suffice.  At least its abstracted so we can change
+   in one place.
+ */

Quoted 4 lines

/*
* kernel comments look
* like this
*/

Done

Quoted 10 lines

drivers/kvm/kvm.h
Quoted 8 lines
drivers/kvm/kvm.h +6 -0
+struct kvm_io_bus {
+	struct list_head list;
+};
+
+static inline void 
+kvm_io_bus_init(struct kvm_io_bus *bus)
function declarations on one line please.

Done (though I hate lines that runneth over 80 ;)

Quoted 10 lines

drivers/kvm/kvm.h
Quoted 7 lines
drivers/kvm/kvm.h +5 -0
+{
+	INIT_LIST_HEAD(&bus- >list);
+}
+
+static inline struct kvm_io_device* 
C style pointers:
struct blah *somthing();

Done.

Quoted 13 lines

drivers/kvm/kvm.h
Quoted 10 lines
drivers/kvm/kvm.h +8 -0
+kvm_io_bus_find_dev(struct kvm_io_bus *bus, gpa_t addr)
+{
+	struct kvm_io_device *pos = NULL;
+
+	list_for_each_entry(pos, &bus- >list, link) {
+		if(pos- >in_range(pos, addr))
+			return pos;
+	}
space after if. avoid redundant {}. Have Documentaion/CodingStyle tattooed somewhere easily accessible.

Done

Quoted 20 lines

drivers/kvm/kvm.h +10 -0

+
+	return NULL;
+}
+
+static inline void 
+kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
+{
+	list_add_tail(&dev- >link, &bus- >list);
+}
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ - 294,6 +342,7 @@ struct kvm_vcpu {

drivers/kvm/kvm.h +1 -0

 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;

Quoted 12 lines

drivers/kvm/kvm.h
Quoted 8 lines
drivers/kvm/kvm.h +0 -0
 	int sigset_active;
 	sigset_t sigset;
@@ - 345,6 +394,7 @@ struct kvm {
drivers/kvm/kvm.h +1 -0
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
The per- vcpu I/O bus is special in that it has exactly one component, and one which can change its address. I think we can special case it and just check for apic addresses explicitly when searching the bus.

I am loath to make special cases if they can be avoided. I think performance wise a kvm_io_bus with one device wont be much different than having a special case check against apicbase. And the advantage that this buys us is future platforms (e.g. IA64?) may have more than one per-cpu MMIO address. I also realize that future platforms may be divergent from the entire in-kernel code base altogether, but I think the general and flexible way is better if there are no compromising tradeoffs, even if its only for example/reference. In this case I dont think there are any tradeoffs, so I left it. If you insist, I will pull it ;)

Quoted 28 lines

Quoted 28 lines
};
struct kvm_stat {
diff -- git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c index 4473174..da119c0 100644
drivers/kvm/kvm_main.c +0 -0
@@ - 294,6 +294,7 @@ static struct kvm *kvm_create_vm(void)
drivers/kvm/kvm_main.c +1 -0
 	spin_lock_init(&kvm- >lock);
 	INIT_LIST_HEAD(&kvm- >active_mmu_pages);
+	kvm_io_bus_init(&kvm- >mmio_bus);
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		struct kvm_vcpu *vcpu = &kvm- >vcpus[i];
@@ - 302,6 +303,7 @@ static struct kvm *kvm_create_vm(void)
drivers/kvm/kvm_main.c +1 -0
 		vcpu- >kvm = kvm;
 		vcpu- >mmu.root_hpa = INVALID_PAGE;
 		INIT_LIST_HEAD(&vcpu- >free_pages);
+		kvm_io_bus_init(&vcpu- >mmio_bus);
 		spin_lock(&kvm_lock);
 		list_add(&kvm- >vm_list, &vm_list);
 		spin_unlock(&kvm_lock);
@@ - 1015,12 +1017,30 @@ static int emulator_write_std(unsigned long addr,
return X86EMUL_UNHANDLEABLE;
}

Quoted 14 lines

drivers/kvm/kvm_main.c

Quoted 12 lines

drivers/kvm/kvm_main.c +11 -0

+static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
+						gpa_t addr)
+{
+	struct kvm_io_device *mmio_dev;
+
+	/* First check the local CPU addresses */
+	mmio_dev = kvm_io_bus_find_dev(&vcpu- >mmio_bus, addr);
+	if(!mmio_dev) {
+		/* Then check the entire VM */
+		mmio_dev = kvm_io_bus_find_dev(&vcpu- >kvm- >mmio_bus, addr);
+	}

space, comment, braces

I believe I fixed this, but I am a little confused about what you were pointing out. The space is obvious. I believe you were pointing out that the braces weren't needed because its technically a single-line, and that the comment is fine. If I needed to change the comment too, let me know.

Quoted 24 lines

drivers/kvm/kvm_main.c +7 -0

+	
+	return mmio_dev;
+}
+
 static int emulator_read_emulated(unsigned long addr,
 				  unsigned long *val,
 				  unsigned int bytes,
 				  struct x86_emulate_ctxt *ctxt)
 {
 	struct kvm_vcpu *vcpu = ctxt- >vcpu;
+	gpa_t gpa;
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (vcpu- >mmio_read_completed) {
 		memcpy(val, vcpu- >mmio_data, bytes);

@@ - 1029,18 +1049,24 @@ static int emulator_read_emulated(unsigned long addr,

drivers/kvm/kvm_main.c +0 -2

 	} 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);

Quoted 13 lines

drivers/kvm/kvm_main.c

Quoted 10 lines

drivers/kvm/kvm_main.c +3 -6

-		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 vcpu_printf(vcpu, "not present\n"), X86EMUL_PROPAGATE_FAULT;

The vcpu_printf() snuck in somehow.

Opps. Carry over from the apic branch merge. Fixed.

Quoted 32 lines

drivers/kvm/kvm_main.c +12 -1

-		return X86EMUL_UNHANDLEABLE;
+	/* Is thie MMIO handled locally? */
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {
+		*val = mmio_dev- >read(mmio_dev, gpa, bytes);
+		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,

@@ - 1070,6 +1096,8 @@ static int emulator_write_emulated(unsigned long addr,
{

drivers/kvm/kvm_main.c +2 -0

 	struct kvm_vcpu *vcpu = ctxt- >vcpu;
 	gpa_t gpa = vcpu- >mmu.gva_to_gpa(vcpu, addr);
+	int i;
+	struct kvm_io_device *mmio_dev;
 
 	if (gpa == UNMAPPED_GVA)
 		return X86EMUL_PROPAGATE_FAULT;

@@ - 1077,6 +1105,13 @@ static int emulator_write_emulated(unsigned long addr,

drivers/kvm/kvm_main.c +0 -0

 	if (emulator_write_phys(vcpu, gpa, val, bytes))
 		return X86EMUL_CONTINUE;

Quoted 4 lines

drivers/kvm/kvm_main.c
Quoted 2 lines
drivers/kvm/kvm_main.c +1 -0
+	/* Is thie MMIO handled locally? */
spelling

Done

Quoted 16 lines

drivers/kvm/kvm_main.c
Quoted 13 lines
drivers/kvm/kvm_main.c +6 -0
+	mmio_dev = vcpu_find_mmio_dev(vcpu, gpa);
+	if(mmio_dev) {
+		mmio_dev- >write(mmio_dev, gpa, bytes, val);
+		return X86EMUL_CONTINUE;
+	}
+
 	vcpu- >mmio_needed = 1;
 	vcpu- >mmio_phys_addr = gpa;
 	vcpu- >mmio_size = bytes;
Please fix and *test*. Boot at least 32- bit Windows with ACPI HAL and 64- bit Linux, the more the better of course.

I have confirmed that my 64 bit linux guest boots fine. I don't currently have any other guests. Careful review of the code leads me to believe this should be an inert change, so I wont go through the effort of finding an XP CD to install unless you insist ;)

Regards,
-Greg

drivers/kvm/kvm.h +52 -0

index fceeb84..c1923df 100644
@@ -236,6 +236,56 @@ struct kvm_pio_request {
 	int rep;
 };
 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      int length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      int length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;
+};
+
+/* 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];
+};
+
+static inline void kvm_io_bus_init(struct kvm_io_bus *bus)
+{
+	memset(bus, 0, sizeof(*bus));
+}
+
+static inline 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;
+}
+
+static inline 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;
+}
+
 struct kvm_vcpu {
 	struct kvm *kvm;
 	union {

@@ -294,6 +344,7 @@ struct kvm_vcpu {
 	gpa_t mmio_phys_addr;
 	struct kvm_pio_request pio;
 	void *pio_data;
+	struct kvm_io_bus mmio_bus;
 
 	int sigset_active;
 	sigset_t sigset;

@@ -345,6 +396,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct kvm_stat {

drivers/kvm/kvm_main.c +44 ~~-12~~

100 lines not on this page · raw message

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2007-04-08 07:38 UTC #15 lore raw

Quoted 10 lines · Gregory Haskins

Quoted 6 lines
drivers/kvm/kvm.h struct kvm_pio_request { · line 247
Quoted 4 lines
drivers/kvm/kvm.h +1 -0
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
Do you see any reason to have this as a callback and not a pair of gpas?
I believe Dor replied earlier stating the reason of being able to support holes. Another reason that I can think of that I particularly like about this design (which I am not claiming as my own) is that the device can relocate (e.g. LAPIC base addr) without worrying about reprogramming the bus.

I don't like either reasons much, but okay. We can address any performance concerns later (I doubt we'll see any with current hardware).

Quoted 9 lines

Quoted 8 lines
drivers/kvm/kvm.h struct kvm_pio_request {
Quoted 5 lines
drivers/kvm/kvm.h +3 -0
+
+	void             *private;
+	struct list_head  link;
Having these in an array would be much more efficient. A fixed size array of moderate size should suffice.

Quoted 3 lines

Done. Maximum # devices is currently 6, because anything beyond that and I think we need to revisit the linear alg ;)

You'll be surprised. Processors are so efficient at processing arrays that you'll need a much longer list before a better algorithm starts to gain.

Anyway 6 is as good a number as any.

Quoted 6 lines

Quoted 2 lines
function declarations on one line please.
Done (though I hate lines that runneth over 80 ;)

A newline usually answers :)

Quoted 8 lines

Quoted 4 lines
The per- vcpu I/O bus is special in that it has exactly one component, and one which can change its address. I think we can special case it and just check for apic addresses explicitly when searching the bus.
I am loath to make special cases if they can be avoided. I think performance wise a kvm_io_bus with one device wont be much different than having a special case check against apicbase. And the advantage that this buys us is future platforms (e.g. IA64?) may have more than one per-cpu MMIO address. I also realize that future platforms may be divergent from the entire in-kernel code base altogether, but I think the general and flexible way is better if there are no compromising tradeoffs, even if its only for example/reference. In this case I dont think there are any tradeoffs, so I left it. If you insist, I will pull it ;)

I think it unlikely that we'll see another local mmio device, it's so counter to the spirit of mmio (which is global by its nature).

Quoted 17 lines

Quoted 16 lines
Quoted 4 lines
drivers/kvm/kvm_main.c +1 -0
+static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
drivers/kvm/kvm_main.c static int emulator_write_std(unsigned long addr, · line 1032
Quoted 10 lines
drivers/kvm/kvm_main.c +10 -0
+						gpa_t addr)
+{
+	struct kvm_io_device *mmio_dev;
+
+	/* First check the local CPU addresses */
+	mmio_dev = kvm_io_bus_find_dev(&vcpu- >mmio_bus, addr);
+	if(!mmio_dev) {
+		/* Then check the entire VM */
+		mmio_dev = kvm_io_bus_find_dev(&vcpu- >kvm- >mmio_bus, addr);
+	}
space, comment, braces

Quoted 2 lines

I believe I fixed this, but I am a little confused about what you were pointing out. The space is obvious. I believe you were pointing out that the braces weren't needed because its technically a single-line, and that the comment is fine. If I needed to change the comment too, let me know.

/*
* comment
*/

Do re-read Documentation/CodingStyle. Coding practices die hard, and the kernel is especially sensitive to coding style. There are some instances of nonconforming comments in the updated patches too.

Quoted 10 lines

Quoted 5 lines
Quoted 2 lines
Please fix and *test*. Boot at least 32- bit Windows with ACPI HAL and 64- bit Linux, the more the better of course.
I have confirmed that my 64 bit linux guest boots fine. I don't currently have any other guests. Careful review of the code leads me to believe this should be an inert change, so I wont go through the effort of finding an XP CD to install unless you insist ;)

Please do test. Even if the changes have no effect, you might expose some latent bug. In any case you'll need Windows to do the apic stuff -- it's much more sensitive to apic problems than Linux.

Signature · 8 lines

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Avi Kivity

2007-04-08 08:49 UTC #16 lore raw

In reply to a message not in the archive

This has significant changes, so merits a review.

Quoted 17 lines · Gregory Haskins

drivers/kvm/kvm.h +9 -0

index fceeb84..c1923df 100644
@@ -236,6 +236,56 @@ struct kvm_pio_request {
 	int rep;
 };
 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      int length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      int length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);

drivers/kvm/kvm.h struct kvm_pio_request { · line 257

Quoted 10 lines

drivers/kvm/kvm.h +9 -0

+
+	void             *private;
+};
+
+/* 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.
+ */

/*
* comment
*/

Quoted 7 lines

drivers/kvm/kvm.h +7 -0

+struct kvm_io_bus {
+	int                   dev_count;
+#define NR_IOBUS_DEVS 6
+	struct kvm_io_device *devs[NR_IOBUS_DEVS];
+};
+
+static inline void kvm_io_bus_init(struct kvm_io_bus *bus)

drivers/kvm/kvm.h struct kvm_pio_request {

Quoted 10 lines

drivers/kvm/kvm.h +9 -0

+{
+	memset(bus, 0, sizeof(*bus));
+}
+
+static inline 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++) {

spaces, after for and around operators

drivers/kvm/kvm.h struct kvm_pio_request {

Quoted 9 lines

drivers/kvm/kvm.h +8 -0

+		struct kvm_io_device *pos = bus->devs[i];
+
+		if (pos->in_range(pos, addr))
+			return pos;
+	}
+
+	return NULL;
+}

this is too long for an inline function. the others can also be made out-of-line.

drivers/kvm/kvm.h struct kvm_pio_request { · line 288

Quoted 9 lines

drivers/kvm/kvm.h +8 -0

+
+static inline 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;
+}
+

I actually meant an array of objects, not pointers, but in the interest of reducing the amount of churn we'll change it if and when we see a problem there.

drivers/kvm/kvm.h

Quoted 4 lines

drivers/kvm/kvm.h +2 -0

+static struct kvm_io_device* vcpu_find_mmio_dev(struct kvm_vcpu *vcpu, 
+						gpa_t addr)

"struct kvm_io_device *..."

Signature · 8 lines

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

A message that is not in the archive (sent off-list or not yet received): 4618AC94.3040700-atKUWr5tajBWk0Htik3J/[email protected]

Gregory Haskins

author

2007-04-09 14:14 UTC #18 lore raw

Hi Avi,

I believe I have incorporated all of the changes requested. Please find the result of that patch inline.

Note that I finally understand what you were getting at with the array of objects thing. I didn't change it yet for the same reason that you mentioned: reduction of churn. However, now that I understand it, I see why you wanted it. Perhaps I will send a follow-on to this patch that uses your idea. But for now...

---

KVM: Add support for in-kernel mmio handlers

There is a near-term need for moving some of the emulation from userspace to the kernel (e.g. interrupt handling). This patch adds a construct for registering in-kernel MMIO handlers. The consumers of this interface will appear in a follow-on patch.

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

---

 drivers/kvm/kvm.h      |   31 ++++++++++++++++++
 drivers/kvm/kvm_main.c |   82 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 101 insertions(+), 12 deletions(-)

drivers/kvm/kvm.h +31 -0

index fceeb84..181099f 100644
@@ -236,6 +236,36 @@ struct kvm_pio_request {
 	int rep;
 };
 
+struct kvm_io_device {
+	unsigned long (*read)(struct kvm_io_device *this,
+			      gpa_t addr,
+			      int length);
+	void (*write)(struct kvm_io_device *this,
+		      gpa_t addr,
+		      int length,
+		      unsigned long val);
+	int (*in_range)(struct kvm_io_device *this, gpa_t addr);
+
+	void             *private;
+};
+
+/*
+ * 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);
+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 {

@@ -345,6 +375,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };
 
 struct kvm_stat {

drivers/kvm/kvm_main.c +70 ~~-12~~

126 lines not on this page · raw message

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2007-04-10 07:56 UTC #20 lore raw

Quoted 6 lines · Gregory Haskins

Hi Avi,
I believe I have incorporated all of the changes requested. Please find the result of that patch inline.
Note that I finally understand what you were getting at with the array of objects thing. I didn't change it yet for the same reason that you mentioned: reduction of churn. However, now that I understand it, I see why you wanted it. Perhaps I will send a follow-on to this patch that uses your idea. But for now...

I will try to be more explicit in the future.

Quoted 9 lines

---
KVM: Add support for in-kernel mmio handlers
There is a near-term need for moving some of the emulation from userspace to the kernel (e.g. interrupt handling). This patch adds a construct for registering in-kernel MMIO handlers. The consumers of this interface will appear in a follow-on patch.

Given that the first consumer is the local apic, it makes sense to add the vcpu-local bus first, no?

Signature · 8 lines

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

A message that is not in the archive (sent off-list or not yet received): 461B4319.80608-atKUWr5tajBWk0Htik3J/[email protected]

Gregory Haskins

author

2007-04-10 11:49 UTC #22 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
On Tue, Apr 10, 2007 at 3:56 AM, in message <461B4319.80608-atKUWr5tajBWk0Htik3J/[email protected]>, Avi

Quoted 11 lines · Kivity

Quoted 2 lines · Gregory Haskins
Note that I finally understand what you were getting at with the array of
objects thing. I didn't change it yet for the same reason that you
mentioned: reduction of churn. However, now that I understand it, I see why you wanted it. Perhaps I will send a follow- on to this patch that uses your idea. But for now...
Quoted 1 line
I will try to be more explicit in the future.

Nah, I think you explained it fine, I was just being too obtuse ;) When you first brought it up, I figured there was some secret deficiency in the linked-list implementation compared to arrays that I wasn't aware of and didn't question it. I see now that what you were getting at is that an array of objects with the range criteria in-line can be made to fit in a handful of cache-lines. Having an indirection to find the kvm_io_device* and another to find the in_range() function do not have this benefit (which both the list and the pointer-array as I have implemented suffer from this).

Quoted 11 lines

Quoted 9 lines
---
KVM: Add support for in- kernel mmio handlers
There is a near- term need for moving some of the emulation from userspace to the kernel (e.g. interrupt handling). This patch adds a construct for registering in- kernel MMIO handlers. The consumers of this interface will appear in a follow- on patch.

Quoted 2 lines

Given that the first consumer is the local apic, it makes sense to add the vcpu- local bus first, no?

I'm confused. I thought you didn't like the vpcu-local bus? I pulled it based on your feedback :) Please advise.

Regards,
-Greg

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2007-04-10 12:02 UTC #24 lore raw

Quoted 7 lines · Gregory Haskins

Quoted 3 lines
Given that the first consumer is the local apic, it makes sense to add the vcpu- local bus first, no?
I'm confused. I thought you didn't like the vpcu-local bus? I pulled it based on your feedback :) Please advise.

Sorry, my mistake.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Avi Kivity

2007-04-05 07:46 UTC #25 lore raw

In reply to a message not in the archive

Quoted 10 lines · Gregory Haskins

The MMIO registration code has been broken out as a new patch from the in-kernel APIC work with the following changes per Avi's request:
1) Supports dynamic registration
2) Uses gpa_t addresses
3) Explicit per-cpu mappings
In addition, I have added the concept of distinct VCPU and VM level registrations (where VCPU devices will eclipse competing VM registrations (if any). This will be key down the road where LAPICs should use VCPU registration, but IOAPICs should use VM level.

drivers/kvm/kvm.h struct kvm { · line 399

Quoted 6 lines

drivers/kvm/kvm.h +1 -0

@@ -345,6 +394,7 @@ struct kvm {
 	unsigned long rmap_overflow;
 	struct list_head vm_list;
 	struct file *filp;
+	struct kvm_io_bus mmio_bus;
 };

Additionally:

The pit/pic are pio devices, not mmio, so they need their own bus.

Signature · 8 lines

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

A message that is not in the archive (sent off-list or not yet received): 4614A973.6020102-atKUWr5tajBWk0Htik3J/[email protected]

Gregory Haskins

author

2007-04-05 13:23 UTC #27 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
On Thu, Apr 5, 2007 at 3:46 AM, in message <4614A973.6020102-atKUWr5tajBWk0Htik3J/[email protected]>,

Quoted 2 lines · Avi Kivity

The pit/pic are pio devices, not mmio, so they need their own bus.

Good morning!

Yeah, I knew that but I haven't coded any support for anything but mmio yet. I explicitly chose the "kvm_io_XX" name instead of kvm_mmio_XXX in hopes that I can use the same structure for a pio_bus object in addition to the existing mmio_bus objects. Since I haven't really spent much time thinking about the pio side yet, im not sure if it will translate or require its own.

Thanks for your feedback on the patch. I will make the changes you requested and re-submit (note quite a few of them have been fixed in the follow up email I sent after talking with Chris Wright).

Regards,
-Greg

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2007-04-05 13:45 UTC #29 lore raw

Quoted 5 lines · Gregory Haskins

Quoted 2 lines
The pit/pic are pio devices, not mmio, so they need their own bus.
Yeah, I knew that but I haven't coded any support for anything but mmio yet. I explicitly chose the "kvm_io_XX" name instead of kvm_mmio_XXX in hopes that I can use the same structure for a pio_bus object in addition to the existing mmio_bus objects. Since I haven't really spent much time thinking about the pio side yet, im not sure if it will translate or require its own.

It should work. A minor nit is that gpa_t is a bit wide for pio, but that does no real harm.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV