Skip to contentSkip to conversation

try to reduce kvm impact in core qemu code.

RFC PATCH In review Glauber Costa 3 participants 5 messages Apr 29, 2008 – Apr 30, 2008 KVM

lore
Share & tools
lore

Glauber Costa

author
#0 lore raw

RFC PATCH 1/1 try to reduce kvm impact in core qemu code.

Hi. This is a proposal for reducing the impact of kvm functions in core qemu code. This is by all means not ready, but I felt like posting it, so a discussion on it could follow.

The idea in this patch is to replace the specific kvm details from core qemu files like vl.c, with driver_yyy() functions. When kvm is not running, those functions would just return (most of time), absolutely reducing the impact of kvm code.

As I wanted to test it, in this patch I changed the kvm functions to be called driver_yyy(), but that's not my final goal. I intend to use a function pointer schema, similar to what the linux kernel already do for a lot of its subsystem, to isolate the changes.

Comments deeply welcome.

---
 qemu/exec.c      |   11 +--
 qemu/gdbstub.c   |    8 +-
 qemu/hw/vmport.c |    6 +-
 qemu/monitor.c   |    3 +-
 qemu/qemu-kvm.c  |  210 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu/qemu-kvm.h  |    1 +
 qemu/vl.c        |  187 +++++-------------------------------------------
 7 files changed, 239 insertions(+), 187 deletions(-)
qemu/exec.c +4 -7
index b82d26d..7a16c78 100644
@@ -1150,8 +1150,7 @@ int cpu_breakpoint_insert(CPUState *env, target_ulong pc)
         return -1;
     env->breakpoints[env->nb_breakpoints++] = pc;
 
-    if (kvm_enabled())
-	kvm_update_debugger(env);
+    driver_update_debugger(env);
 
     breakpoint_invalidate(env, pc);
     return 0;

@@ -1175,8 +1174,7 @@ int cpu_breakpoint_remove(CPUState *env, target_ulong pc)
     if (i < env->nb_breakpoints)
       env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
 
-    if (kvm_enabled())
-	kvm_update_debugger(env);
+    driver_update_debugger(env);
     
     breakpoint_invalidate(env, pc);
     return 0;

@@ -1196,8 +1194,7 @@ void cpu_single_step(CPUState *env, int enabled)
         /* XXX: only flush what is necessary */
         tb_flush(env);
     }
-    if (kvm_enabled())
-	kvm_update_debugger(env);
+    driver_update_debugger(env);
 #endif
 }
 

@@ -1246,7 +1243,7 @@ void cpu_interrupt(CPUState *env, int mask)
 
     env->interrupt_request |= mask;
     if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())
-	kvm_update_interrupt_request(env);
+        kvm_update_interrupt_request(env);
 
     /* if the cpu is currently executing code, we must unlink it and
        all the potentially executing TB */
qemu/gdbstub.c +4 -4
index 2252084..c574686 100644
@@ -895,7 +895,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
 #if defined(TARGET_I386)
             env->eip = addr;
 	    if (kvm_enabled())
-		kvm_load_registers(env);
+		    driver_load_registers(env);
 #elif defined (TARGET_PPC)
             env->nip = addr;
 #elif defined (TARGET_SPARC)

@@ -923,7 +923,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
 #if defined(TARGET_I386)
             env->eip = addr;
 	    if (kvm_enabled())
-		kvm_load_registers(env);
+		    driver_load_registers(env);
 #elif defined (TARGET_PPC)
             env->nip = addr;
 #elif defined (TARGET_SPARC)

@@ -976,7 +976,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
         break;
     case 'g':
 	if (kvm_enabled())
-	    kvm_save_registers(env);
+	    driver_save_registers(env);
         reg_size = cpu_gdb_read_registers(env, mem_buf);
         memtohex(buf, mem_buf, reg_size);
         put_packet(s, buf);

@@ -987,7 +987,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
         hextomem((uint8_t *)registers, p, len);
         cpu_gdb_write_registers(env, mem_buf, len);
 	if (kvm_enabled())
-	    kvm_load_registers(env);
+	    driver_load_registers(env);
         put_packet(s, "OK");
         break;
     case 'm':
qemu/hw/vmport.c +2 -4
index c09227d..a519152 100644
@@ -59,8 +59,7 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
     uint32_t eax;
     uint32_t ret;
 
-    if (kvm_enabled())
-	kvm_save_registers(s->env);
+	driver_save_registers(s->env);
 
     eax = s->env->regs[R_EAX];
     if (eax != VMPORT_MAGIC)

@@ -77,8 +76,7 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
 
     ret = s->func[command](s->opaque[command], addr);
 
-    if (kvm_enabled())
-	kvm_load_registers(s->env);
+	driver_load_registers(s->env);
 
     return ret;
 }
qemu/monitor.c +1 -2
index 4ee0b19..bd538d9 100644
@@ -286,8 +286,7 @@ static CPUState *mon_get_cpu(void)
         mon_set_cpu(0);
     }
 
-    if (kvm_enabled())
-	kvm_save_registers(mon_cpu);
+    driver_save_registers(mon_cpu);
 
     return mon_cpu;
 }
qemu/qemu-kvm.c +207 -3

238 lines not on this page · raw message

qemu/qemu-kvm.h +1 -0
index 8e45f30..7953f4a 100644
@@ -81,6 +81,7 @@ int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
 
 extern int kvm_allowed;
 extern kvm_context_t kvm_context;
+extern unsigned int kvm_shadow_memory;
 
 #define kvm_enabled() (kvm_allowed)
 #define qemu_kvm_irqchip_in_kernel() kvm_irqchip_in_kernel(kvm_context)
qemu/vl.c +20 -167

291 lines not on this page · raw message

Signature · 8 lines
1.5.0.6


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
#1 lore raw
Quoted 8 lines · Glauber Costa

Hi. This is a proposal for reducing the impact of kvm functions in core qemu code. This is by all means not ready, but I felt like posting it, so a discussion on it could follow.

The idea in this patch is to replace the specific kvm details from core qemu files like vl.c, with driver_yyy() functions. When kvm is not running, those functions would just return (most of time), absolutely reducing the impact of kvm code.

Quoted 6 lines

As I wanted to test it, in this patch I changed the kvm functions to be called driver_yyy(), but that's not my final goal. I intend to use a function pointer schema, similar to what the linux kernel already do for a lot of its subsystem, to isolate the changes.

Comments deeply welcome.

While I would be very annoyed if someone referred to kvm as a qemu accelerator, I think accelerator_yyy() is more descriptive than driver_yyy().

I did not see any references to kqemu, but I imagine you mean this to abstract kqemu support as well.

Other than that, looks really good.

Signature · 8 lines
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
author
#2 lore raw
Quoted 19 lines · Avi Kivity
Quoted 18 lines · Glauber Costa

Hi. This is a proposal for reducing the impact of kvm functions in core qemu
code. This is by all means not ready, but I felt like posting it, so a discussion
on it could follow.

The idea in this patch is to replace the specific kvm details from core qemu files
like vl.c, with driver_yyy() functions. When kvm is not running, those functions would
just return (most of time), absolutely reducing the impact of kvm code.

As I wanted to test it, in this patch I changed the kvm functions to be called driver_yyy(),
but that's not my final goal. I intend to use a function pointer schema, similar to what the linux
kernel already do for a lot of its subsystem, to isolate the changes.

Quoted 6 lines
Quoted 2 lines

Comments deeply welcome.

While I would be very annoyed if someone referred to kvm as a qemu accelerator, I think accelerator_yyy() is more descriptive than driver_yyy().

How about booster? ;-)

Quoted 2 lines

I did not see any references to kqemu, but I imagine you mean this to abstract kqemu support as well.

Yeah, even the kvm part is not complete. As I said, just wanted to get it going.

Quoted 3 lines

Other than that, looks really good.

thanks

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

#3 lore raw
Quoted 22 lines · Glauber Costa
Quoted 20 lines · Avi Kivity
Quoted 18 lines · Glauber Costa

Hi. This is a proposal for reducing the impact of kvm functions in core qemu
code. This is by all means not ready, but I felt like posting it, so a discussion
on it could follow.

The idea in this patch is to replace the specific kvm details from core qemu files
like vl.c, with driver_yyy() functions. When kvm is not running, those functions would
just return (most of time), absolutely reducing the impact of kvm code.

As I wanted to test it, in this patch I changed the kvm functions to be called driver_yyy(),
but that's not my final goal. I intend to use a function pointer schema, similar to what the linux
kernel already do for a lot of its subsystem, to isolate the changes.

Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

Comments deeply welcome.

While I would be very annoyed if someone referred to kvm as a qemu accelerator, I think accelerator_yyy() is more descriptive than driver_yyy().

How about booster? ;-)

I don't think the concern from a QEMU perspective is that QEMU is too intimately tied to KVM. The concern is that overtime, it will be very difficult to make changes to QEMU without breaking KVM support because of the shear number of hooks we require. Fabrice had actually suggested merging libkvm into QEMU. We just need to reduce the overall number of if (kvm_enabled()) blocks.

You have to understand a lot about KVM to know why it's necessary to do an register reload call-out in the vmport hw for instance. Instead of introducing generic hooks in vmport, a more appropriate solution may be to add wrappers to read individual register values within the QEMU
device code. We can then add our hooks to that. I think a good
argument can be made that devices should never access env-> directly.

Regards,

Anthony Liguori

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

author
#4 lore raw
Quoted 32 lines · Anthony Liguori
Quoted 22 lines · Glauber Costa
Quoted 20 lines · Avi Kivity
Quoted 18 lines · Glauber Costa

Hi. This is a proposal for reducing the impact of kvm functions in core qemu
code. This is by all means not ready, but I felt like posting it, so a discussion
on it could follow.

The idea in this patch is to replace the specific kvm details from core qemu files
like vl.c, with driver_yyy() functions. When kvm is not running, those functions would
just return (most of time), absolutely reducing the impact of kvm code.

As I wanted to test it, in this patch I changed the kvm functions to be called driver_yyy(),
but that's not my final goal. I intend to use a function pointer schema, similar to what the linux
kernel already do for a lot of its subsystem, to isolate the changes.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Comments deeply welcome.

While I would be very annoyed if someone referred to kvm as a qemu accelerator, I think accelerator_yyy() is more descriptive than driver_yyy().

How about booster? ;-)

Quoted 6 lines

I don't think the concern from a QEMU perspective is that QEMU is too intimately tied to KVM. The concern is that overtime, it will be very difficult to make changes to QEMU without breaking KVM support because of the shear number of hooks we require. Fabrice had actually suggested merging libkvm into QEMU. We just need to reduce the overall number of if (kvm_enabled()) blocks.

They are not mutually exclusive. Even if we do merge libkvm into qemu, having it more modular and separated will be way better than having kvm hooks all over.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone