kvmctl code refactoring part 1

# HG changeset patch
# User Jerone Young <jyoung5-r/[email protected]> # Date 1193618330 18000
# Node ID 3bf072e498768885ab96b7ccb668b61c96db0e83
# Parent a6f7c585fe76f9563fd061cfe3e772532ab27952
Move x86 kvmcallback structure to kvmctl-x86.h header.

This patch moves the kvmcallback structure that is currently in kvmctl.h into an arch specific header.

Signed-off-by Jerone Young <jyoung5-r/[email protected]>

new file mode 100644

user/kvmctl-x86.h new file +7 -0

new file mode 100644
@@ -0,0 +1,54 @@
+#ifndef KVMCTL_X86_H
+#define KVMCTL_X86_H
+
+/*!
+ * \brief KVM callbacks structure
+ *
+ * This structure holds pointers to various functions that KVM will

call + * when it encounters something that cannot be virtualized, such as + * accessing hardware devices via MMIO or regular IO.
+ */

+ int (*inw)(void *opaque, uint16_t addr, uint16_t *data);

/*!
* \brief Create new KVM context

This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >>
http://get.splunk.com/ _______________________________________________ kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

# HG changeset patch
# User Jerone Young <jyoung5-r/[email protected]> # Date 1193618330 18000
# Node ID 8bf5e4e6a4c9d2dab89062a0ab24a2ae5d144a02
# Parent 3bf072e498768885ab96b7ccb668b61c96db0e83
Move kvm_context structure to kvmctl.h header

This patch moves the kvm_context sturcture declaration to the kvmctl.h header. This way other files are able to identify variables associated with the structure. Also moved are definitions MAX_VCPU & KVM_MAX_NUM_MEM_REGIONS to the kvmctl-x86.h since these are arch specific.

- */

user/kvmctl.c +0 -7

-struct kvm_context {
-	/// Filedescriptor to /dev/kvm
-	int fd;
-	int vm_fd;
-	int vcpu_fd[MAX_VCPUS];
-	struct kvm_run *run[MAX_VCPUS];
-	/// Callbacks that KVM uses to emulate various unvirtualizable

functionality

user/kvmctl.c +0 -3

-	struct kvm_callbacks *callbacks;
-	void *opaque;
-	/// A pointer to the memory used as the physical memory for the

guest

Quoted 29 lines · Jerone Young

# HG changeset patch
# User Jerone Young <jyoung5-r/[email protected]> # Date 1193618330 18000
# Node ID 3bf072e498768885ab96b7ccb668b61c96db0e83
# Parent a6f7c585fe76f9563fd061cfe3e772532ab27952
Move x86 kvmcallback structure to kvmctl-x86.h header.

This patch moves the kvmcallback structure that is currently in kvmctl.h into an arch specific header.

Signed-off-by Jerone Young <jyoung5-r/[email protected]>

new file mode 100644

user/kvmctl-x86.h new file +7 -0

new file mode 100644
@@ -0,0 +1,54 @@
+#ifndef KVMCTL_X86_H
+#define KVMCTL_X86_H
+
+/*!
+ * \brief KVM callbacks structure
+ *
+ * This structure holds pointers to various functions that KVM will

call + * when it encounters something that cannot be virtualized, such as + * accessing hardware devices via MMIO or regular IO.
+ */

user/kvmctl-x86.h

Quoted 2 lines

user/kvmctl-x86.h new file +2 -0

+struct kvm_callbacks {
+	/// For 8bit IO reads from the guest (Usually when executing

'inb')

user/kvmctl-x86.h

Quoted 2 lines

user/kvmctl-x86.h new file +2 -0

+    int (*inb)(void *opaque, uint16_t addr, uint8_t *data);
+	/// For 16bit IO reads from the guest (Usually when executing

'inw')

Quoted 1 line

+ int (*inw)(void *opaque, uint16_t addr, uint16_t *data);

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

# HG changeset patch
# User Jerone Young <jyoung5-r/[email protected]> # Date 1193618330 18000
# Node ID 8bf5e4e6a4c9d2dab89062a0ab24a2ae5d144a02
# Parent 3bf072e498768885ab96b7ccb668b61c96db0e83
Move kvm_context structure to kvmctl.h header

This patch moves the kvm_context sturcture declaration to the kvmctl.h header. This way other files are able to identify variables associated with the structure. Also moved are definitions MAX_VCPU & KVM_MAX_NUM_MEM_REGIONS to the kvmctl-x86.h since these are arch specific.

Signed-off-by Jerone Young <jyoung5-r/[email protected]>

user/kvmctl-x86.h +3 -0

@@ -1,5 +1,8 @@
 #ifndef KVMCTL_X86_H
 #define KVMCTL_X86_H
+
+#define KVM_MAX_NUM_MEM_REGIONS 8u
+#define MAX_VCPUS 4
 
 /*!
  * \brief KVM callbacks structure

user/kvmctl.c +0 -28

@@ -44,36 +44,8 @@ static int kvm_abi = EXPECTED_KVM_API_VE
 
 /* FIXME: share this number with kvm */
 /* FIXME: or dynamically alloc/realloc regions */
-#define KVM_MAX_NUM_MEM_REGIONS 8u
 int free_slots[KVM_MAX_NUM_MEM_REGIONS];
 unsigned long phys_addr_slots[KVM_MAX_NUM_MEM_REGIONS];
-#define MAX_VCPUS 4
-
-/**
- * \brief The KVM context
- *
- * The verbose KVM context
- */
-struct kvm_context {
-	/// Filedescriptor to /dev/kvm
-	int fd;
-	int vm_fd;
-	int vcpu_fd[MAX_VCPUS];
-	struct kvm_run *run[MAX_VCPUS];
-	/// Callbacks that KVM uses to emulate various unvirtualizable functionality
-	struct kvm_callbacks *callbacks;
-	void *opaque;
-	/// A pointer to the memory used as the physical memory for the guest
-	void *physical_memory;
-	/// is dirty pages logging enabled for all regions or not
-	int dirty_pages_log_all;
-	/// memory regions parameters
-	struct kvm_memory_region mem_regions[KVM_MAX_NUM_MEM_REGIONS];
-	/// do not create in-kernel irqchip if set
-	int no_irqchip_creation;
-	/// in-kernel irqchip status
-	int irqchip_in_kernel;
-};
 
 static void init_slots()
 {

user/kvmctl.h +30 -1

@@ -31,6 +31,36 @@ typedef struct kvm_context *kvm_context_
 #if defined(__x86_64__) || defined(__i386__)
 #include "kvmctl-x86.h"
 #endif
+
+
+/**
+ * \brief The KVM context
+ *
+ * The verbose KVM context
+ */
+
+struct kvm_context {
+	/// Filedescriptor to /dev/kvm
+	int fd;
+	int vm_fd;
+	int vcpu_fd[MAX_VCPUS];
+	struct kvm_run *run[MAX_VCPUS];
+	/// Callbacks that KVM uses to emulate various unvirtualizable functionality
+	struct kvm_callbacks *callbacks;
+	void *opaque;
+	/// A pointer to the memory used as the physical memory for the guest
+	void *physical_memory;
+	/// is dirty pages logging enabled for all regions or not
+	int dirty_pages_log_all;
+	/// memory regions parameters
+	struct kvm_memory_region mem_regions[KVM_MAX_NUM_MEM_REGIONS];
+	/// do not create in-kernel irqchip if set
+	int no_irqchip_creation;
+	/// in-kernel irqchip status
+	int irqchip_in_kernel;
+};
+
+
 
 /*!
  * \brief Create new KVM context
 
-------------------------------------------------------------------------

This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

# HG changeset patch
# User Jerone Young <jyoung5-r/[email protected]> # Date 1193618330 18000
# Node ID 3bf072e498768885ab96b7ccb668b61c96db0e83
# Parent a6f7c585fe76f9563fd061cfe3e772532ab27952
Move x86 kvmcallback structure to kvmctl-x86.h header.

This patch moves the kvmcallback structure that is currently in kvmctl.h into an arch specific header.

Signed-off-by Jerone Young <jyoung5-r/[email protected]>

new file mode 100644

user/kvmctl-x86.h new file +54 -0

new file mode 100644
@@ -0,0 +1,54 @@
+#ifndef KVMCTL_X86_H
+#define KVMCTL_X86_H
+
+/*!
+ * \brief KVM callbacks structure
+ *
+ * This structure holds pointers to various functions that KVM will call
+ * when it encounters something that cannot be virtualized, such as
+ * accessing hardware devices via MMIO or regular IO.
+ */
+struct kvm_callbacks {
+	/// For 8bit IO reads from the guest (Usually when executing 'inb')
+    int (*inb)(void *opaque, uint16_t addr, uint8_t *data);
+	/// For 16bit IO reads from the guest (Usually when executing 'inw')
+    int (*inw)(void *opaque, uint16_t addr, uint16_t *data);
+	/// For 32bit IO reads from the guest (Usually when executing 'inl')
+    int (*inl)(void *opaque, uint16_t addr, uint32_t *data);
+	/// For 8bit IO writes from the guest (Usually when executing 'outb')
+    int (*outb)(void *opaque, uint16_t addr, uint8_t data);
+	/// For 16bit IO writes from the guest (Usually when executing 'outw')
+    int (*outw)(void *opaque, uint16_t addr, uint16_t data);
+	/// For 32bit IO writes from the guest (Usually when executing 'outl')
+    int (*outl)(void *opaque, uint16_t addr, uint32_t data);
+	/// For 8bit memory reads from unmapped memory (For MMIO devices)
+    int (*readb)(void *opaque, uint64_t addr, uint8_t *data);
+	/// For 16bit memory reads from unmapped memory (For MMIO devices)
+    int (*readw)(void *opaque, uint64_t addr, uint16_t *data);
+	/// For 32bit memory reads from unmapped memory (For MMIO devices)
+    int (*readl)(void *opaque, uint64_t addr, uint32_t *data);
+	/// For 64bit memory reads from unmapped memory (For MMIO devices)
+    int (*readq)(void *opaque, uint64_t addr, uint64_t *data);
+	/// For 8bit memory writes to unmapped memory (For MMIO devices)
+    int (*writeb)(void *opaque, uint64_t addr, uint8_t data);
+	/// For 16bit memory writes to unmapped memory (For MMIO devices)
+    int (*writew)(void *opaque, uint64_t addr, uint16_t data);
+	/// For 32bit memory writes to unmapped memory (For MMIO devices)
+    int (*writel)(void *opaque, uint64_t addr, uint32_t data);
+	/// For 64bit memory writes to unmapped memory (For MMIO devices)
+    int (*writeq)(void *opaque, uint64_t addr, uint64_t data);
+    int (*debug)(void *opaque, int vcpu);
+	/*!
+	 * \brief Called when the VCPU issues an 'hlt' instruction.
+	 *
+	 * Typically, you should yeild here to prevent 100% CPU utilization
+	 * on the host CPU.
+	 */
+    int (*halt)(void *opaque, int vcpu);
+    int (*shutdown)(void *opaque, int vcpu);
+    int (*io_window)(void *opaque);
+    int (*try_push_interrupts)(void *opaque);
+    void (*post_kvm_run)(void *opaque, int vcpu);
+    int (*pre_kvm_run)(void *opaque, int vcpu);
+};
+#endif

user/kvmctl.h +0 -27

@@ -27,56 +27,10 @@ struct kvm_context;
 
 typedef struct kvm_context *kvm_context_t;
 
-/*!
- * \brief KVM callbacks structure
- *
- * This structure holds pointers to various functions that KVM will call
- * when it encounters something that cannot be virtualized, such as
- * accessing hardware devices via MMIO or regular IO.
- */
-struct kvm_callbacks {
-	/// For 8bit IO reads from the guest (Usually when executing 'inb')
-    int (*inb)(void *opaque, uint16_t addr, uint8_t *data);
-	/// For 16bit IO reads from the guest (Usually when executing 'inw')
-    int (*inw)(void *opaque, uint16_t addr, uint16_t *data);
-	/// For 32bit IO reads from the guest (Usually when executing 'inl')
-    int (*inl)(void *opaque, uint16_t addr, uint32_t *data);
-	/// For 8bit IO writes from the guest (Usually when executing 'outb')
-    int (*outb)(void *opaque, uint16_t addr, uint8_t data);
-	/// For 16bit IO writes from the guest (Usually when executing 'outw')
-    int (*outw)(void *opaque, uint16_t addr, uint16_t data);
-	/// For 32bit IO writes from the guest (Usually when executing 'outl')
-    int (*outl)(void *opaque, uint16_t addr, uint32_t data);
-	/// For 8bit memory reads from unmapped memory (For MMIO devices)
-    int (*readb)(void *opaque, uint64_t addr, uint8_t *data);
-	/// For 16bit memory reads from unmapped memory (For MMIO devices)
-    int (*readw)(void *opaque, uint64_t addr, uint16_t *data);
-	/// For 32bit memory reads from unmapped memory (For MMIO devices)
-    int (*readl)(void *opaque, uint64_t addr, uint32_t *data);
-	/// For 64bit memory reads from unmapped memory (For MMIO devices)

user/kvmctl.h +4 -14

-    int (*debug)(void *opaque, int vcpu);
-	/*!
-	 * \brief Called when the VCPU issues an 'hlt' instruction.
-	 *
-	 * Typically, you should yeild here to prevent 100% CPU utilization
-	 * on the host CPU.
-	 */
-    int (*halt)(void *opaque, int vcpu);
-    int (*shutdown)(void *opaque, int vcpu);
-    int (*io_window)(void *opaque);
-    int (*try_push_interrupts)(void *opaque);
-    void (*post_kvm_run)(void *opaque, int vcpu);
-    int (*pre_kvm_run)(void *opaque, int vcpu);
-};
+/* Add info from arch specific header */
+#if defined(__x86_64__) || defined(__i386__)
+#include "kvmctl-x86.h"
+#endif
 
 /*!

* \brief Create new KVM context

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 6 lines

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

Are you suggesting that kvm_callbacks should be the union of all callbacks used on all architectures, and for any given architecture only a subset are actually used?

Quoted 14 lines

Quoted 8 lines

call + * when it encounters something that cannot be virtualized, such as + * accessing hardware devices via MMIO or regular IO. + */

(quoted diff) +2 -0

+struct kvm_callbacks {
+	/// For 8bit IO reads from the guest (Usually when executing

'inb') + int (*inb)(void *opaque, uint16_t addr, uint8_t *data);

(quoted diff) +1 -0

+	/// For 16bit IO reads from the guest (Usually when executing

'inw') + int (*inw)(void *opaque, uint16_t addr, uint16_t *data);

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

Are you suggesting that kvm_callbacks should be the union of all callbacks used on all architectures, and for any given architecture only a subset are actually used?

Quoted 11 lines · Hollis Blanchard

Quoted 6 lines

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

Are you suggesting that kvm_callbacks should be the union of all callbacks used on all architectures, and for any given architecture only a subset are actually used?

I think two separate callback structures would make more sense.

Quite a few of the callbacks should have common implementations. For instance, all of the io callbacks and the io_window callback should be the same. I would expect most architectures have a concept of a "halt" so that should probably be the same too. That pretty much covers the majority of the callbacks structure :-)

Regards,

Anthony Liguori

Quoted 14 lines

Quoted 13 lines

Quoted 7 lines

call + * when it encounters something that cannot be virtualized, such as + * accessing hardware devices via MMIO or regular IO. + */

(quoted diff) +2 -0

+struct kvm_callbacks {
+	/// For 8bit IO reads from the guest (Usually when executing

'inb') + int (*inb)(void *opaque, uint16_t addr, uint8_t *data);

(quoted diff) +1 -0

+	/// For 16bit IO reads from the guest (Usually when executing

'inw') + int (*inw)(void *opaque, uint16_t addr, uint16_t *data);

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

Quoted 3 lines

Are you suggesting that kvm_callbacks should be the union of all callbacks used on all architectures, and for any given architecture only a subset are actually used?

No, not a union. I think we should keep some common fields for all archs. At least, io_callbacks should be same for all archs.

So, we need to split them into two parts, one common and the other one is arch-specific. Maybe we should add an filed, such as arch to hold arch-specific fields, instead of moving them all to kvmctl-x86.h

Quoted 15 lines · Anthony Liguori

Quoted 11 lines · Hollis Blanchard

Quoted 6 lines

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

Are you suggesting that kvm_callbacks should be the union of all callbacks used on all architectures, and for any given architecture only a subset are actually used?

I think two separate callback structures would make more sense.

Quoted 5 lines

Quite a few of the callbacks should have common implementations. For instance, all of the io callbacks and the io_window callback should be the same. I would expect most architectures have a concept of a "halt" so that should probably be the same too. That pretty much covers the majority of the callbacks structure :-)

Agree.

int (*io_write)(void *opaque, int as, uint64_t addr, uint64_t data, int size);

Where as is a #define representing the address space (on x86, there is the PIO and MMIO address spaces, on PPC, there is just MMIO).

Quoted 6 lines · Anthony Liguori

int (*io_write)(void *opaque, int as, uint64_t addr, uint64_t data, int size);

Where as is a #define representing the address space (on x86, there is the PIO and MMIO address spaces, on PPC, there is just MMIO).

So the implementation would look something like this:

int io_write(void *opaque, int as, uint64_t addr, uint64_t data, int

size) {
        io_handler_t cb;
        
        switch (as) {
        case MMIO:
                cb = io_table_lookup(&mmio_table, ...)
                break;
#ifdef HAS_PIO
        case PIO:
                cb = io_table_lookup(&pio_table, ...)
                break;
#endif
#ifdef HAS_DCR
        case DCR:
                cb = io_table_lookup(&dcr_table, ...)
                break;
#endif
        default:
                cb = NULL;
        }
        
        if (cb)
                return cb(...);
        
        printk("error");
        return -EINVAL;
}

Sounds fine to me.

Quoted 18 lines · Xiantao

Quoted 16 lines · Anthony Liguori

Quoted 7 lines · Hollis Blanchard

Quoted 7 lines

I don't know the privious story about this thread, but now I can't understand the move. Why do we move all the structure to arch-specific ? For IA64 side, almostly we can reuse them directly, and just see some special fields as arch-specific. So, I think, we should keep common fields in kvmctl.h.

Quoted 5 lines

Are you suggesting that kvm_callbacks should be the union of all callbacks used on all architectures, and for any given architecture only a subset are actually used?

I think two separate callback structures would make more sense.

Quoted 8 lines

Quoted 6 lines

Quite a few of the callbacks should have common implementations. For instance, all of the io callbacks and the io_window callback should be the same. I would expect most architectures have a concept of a "halt" so that should probably be the same too. That pretty much covers the majority of the callbacks structure :-)

Agree.

OK, are you changing your position then? Anthony is saying there should be multiple callback data structure definitions, but that the *implementations* of some of those callbacks should be shared. For example:

ia64.h:

        struct kvm_callbacks {
                ...
                .inb = pio_inb,
        };

x86.h:

        struct kvm_callbacks {
                ...
                .inb = pio_inb,
        };

pio.c (built only for ia64 and x86):

        int pio_inb(port) {
                ...
        }

You could certainly get even more clever and have the arch backend register the appropriate tables based on the as type but that's merely an implementation detail. The key observation, that I believe is correct, is that all architectures have one or more IO "address spaces" that have at max a 64-bit address space and support at max 64-bit input/output operations. Once that assumption is made, almost all IO code becomes common.

# HG changeset patch
# User Jerone Young <jyoung5-r/[email protected]> # Date 1193618330 18000
# Node ID 8bf5e4e6a4c9d2dab89062a0ab24a2ae5d144a02
# Parent 3bf072e498768885ab96b7ccb668b61c96db0e83
Move kvm_context structure to kvmctl.h header

This patch moves the kvm_context sturcture declaration to the kvmctl.h header. This way other files are able to identify variables associated with the structure. Also moved are definitions MAX_VCPU & KVM_MAX_NUM_MEM_REGIONS to the kvmctl-x86.h since these are arch specific.

Quoted 8 lines · Anthony Liguori

You could certainly get even more clever and have the arch backend register the appropriate tables based on the as type but that's merely an implementation detail. The key observation, that I believe is correct, is that all architectures have one or more IO "address spaces" that have at max a 64-bit address space and support at max 64-bit input/output operations. Once that assumption is made, almost all IO code becomes common.

Just FYI, some PowerPC have "load/store quad", which are 128-bit memory accesses.

For that matter, I suppose one could do IO loads into Altivec
registers (which are also 128 bits), though that sounds like an extreme case. I wonder if the same is true for x86 vector registers.

Also, can't x86 "rep" instructions go beyond 64 bits? I guess that must be handled in the arch-specific caller of io_write(), which would call it multiple times.

Also, can't x86 "rep" instructions go beyond 64 bits? I guess that must be handled in the arch-specific caller of io_write(), which would call it multiple times.