[Qemu-devel] [PATCH] create kvm-shared-all.c and kvm-shared.h

[Qemu-devel] [PATCH] create kvm-shared-all.c and kvm-shared.h

[Glauber Costa]

Following a suggestion given by Jan, the idea here is to
move shared pieces between qemu and qemu-kvm.git into a common
file, so we can do sharing while avoid clashes.

In the future, this files should disappear.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 Makefile.target  |    3 +-
 kvm-all.c        |   73 ------------------------------------------------------
 kvm-shared-all.c |   58 ++++++++++++++++++++++++++++++++++++++++++
 kvm-shared.h     |   38 ++++++++++++++++++++++++++++
 kvm.h            |   10 +------
 5 files changed, 99 insertions(+), 83 deletions(-)
 create mode 100644 kvm-shared-all.c
 create mode 100644 kvm-shared.h

diff --git a/Makefile.target b/Makefile.target
index 27de4b9..d78db27 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -126,6 +126,7 @@ endif
 
 kvm.o: CFLAGS+=$(KVM_CFLAGS)
 kvm-all.o: CFLAGS+=$(KVM_CFLAGS)
+kvm-shared-all.o: CFLAGS+=$(KVM_CFLAGS)
 
 all: $(PROGS)
 # Dummy command so that make thinks it has done something
@@ -499,7 +500,7 @@ OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o \
 # need to fix this properly
 OBJS+=virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o
 ifdef CONFIG_KVM
-OBJS+=kvm.o kvm-all.o
+OBJS+=kvm.o kvm-all.o kvm-shared-all.o
 endif
 
 LIBS+=-lz
diff --git a/kvm-all.c b/kvm-all.c
index b24d876..258d41a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -39,32 +39,10 @@
     do { } while (0)
 #endif
 
-typedef struct KVMSlot
-{
-    target_phys_addr_t start_addr;
-    ram_addr_t memory_size;
-    ram_addr_t phys_offset;
-    int slot;
-    int flags;
-} KVMSlot;
-
 typedef struct kvm_dirty_log KVMDirtyLog;
 
 int kvm_allowed = 0;
 
-struct KVMState
-{
-    KVMSlot slots[32];
-    int fd;
-    int vmfd;
-    int coalesced_mmio;
-    int broken_set_mem_region;
-    int migration_log;
-#ifdef KVM_CAP_SET_GUEST_DEBUG
-    struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
-#endif
-};
-
 static KVMState *kvm_state;
 
 static KVMSlot *kvm_alloc_slot(KVMState *s)
@@ -803,57 +781,6 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     }
 }
 
-int kvm_ioctl(KVMState *s, int type, ...)
-{
-    int ret;
-    void *arg;
-    va_list ap;
-
-    va_start(ap, type);
-    arg = va_arg(ap, void *);
-    va_end(ap);
-
-    ret = ioctl(s->fd, type, arg);
-    if (ret == -1)
-        ret = -errno;
-
-    return ret;
-}
-
-int kvm_vm_ioctl(KVMState *s, int type, ...)
-{
-    int ret;
-    void *arg;
-    va_list ap;
-
-    va_start(ap, type);
-    arg = va_arg(ap, void *);
-    va_end(ap);
-
-    ret = ioctl(s->vmfd, type, arg);
-    if (ret == -1)
-        ret = -errno;
-
-    return ret;
-}
-
-int kvm_vcpu_ioctl(CPUState *env, int type, ...)
-{
-    int ret;
-    void *arg;
-    va_list ap;
-
-    va_start(ap, type);
-    arg = va_arg(ap, void *);
-    va_end(ap);
-
-    ret = ioctl(env->kvm_fd, type, arg);
-    if (ret == -1)
-        ret = -errno;
-
-    return ret;
-}
-
 int kvm_has_sync_mmu(void)
 {
 #ifdef KVM_CAP_SYNC_MMU
diff --git a/kvm-shared-all.c b/kvm-shared-all.c
new file mode 100644
index 0000000..ca94155
--- /dev/null
+++ b/kvm-shared-all.c
@@ -0,0 +1,58 @@
+
+#include <sys/ioctl.h>
+
+#include "sysemu.h"
+#include "kvm.h"
+
+int kvm_ioctl(KVMState *s, int type, ...)
+{
+    int ret;
+    void *arg;
+    va_list ap;
+
+    va_start(ap, type);
+    arg = va_arg(ap, void *);
+    va_end(ap);
+
+    ret = ioctl(s->fd, type, arg);
+    if (ret == -1)
+        ret = -errno;
+
+    return ret;
+}
+
+int kvm_vm_ioctl(KVMState *s, int type, ...)
+{
+    int ret;
+    void *arg;
+    va_list ap;
+
+    va_start(ap, type);
+    arg = va_arg(ap, void *);
+    va_end(ap);
+
+    ret = ioctl(s->vmfd, type, arg);
+    if (ret == -1)
+        ret = -errno;
+
+    return ret;
+}
+
+int kvm_vcpu_ioctl(CPUState *env, int type, ...)
+{
+    int ret;
+    void *arg;
+    va_list ap;
+
+    va_start(ap, type);
+    arg = va_arg(ap, void *);
+    va_end(ap);
+
+    ret = ioctl(env->kvm_fd, type, arg);
+    if (ret == -1)
+        ret = -errno;
+
+    return ret;
+}
+
+
diff --git a/kvm-shared.h b/kvm-shared.h
new file mode 100644
index 0000000..c8364ff
--- /dev/null
+++ b/kvm-shared.h
@@ -0,0 +1,38 @@
+/* This file is temporary by nature. It exists to aid merging of
+ * qemu-kvm.git, and should go away once it is in
+ */
+#ifndef QEMU_KVM_SHARED_H
+#define QEMU_KVM_SHARED_H
+
+typedef struct KVMSlot
+{
+    target_phys_addr_t start_addr;
+    ram_addr_t memory_size;
+    ram_addr_t phys_offset;
+    int slot;
+    int flags;
+} KVMSlot;
+
+struct kvm_sw_breakpoint {
+    target_ulong pc;
+    target_ulong saved_insn;
+    int use_count;
+    TAILQ_ENTRY(kvm_sw_breakpoint) entry;
+};
+
+TAILQ_HEAD(kvm_sw_breakpoint_head, kvm_sw_breakpoint);
+
+struct KVMState
+{
+    KVMSlot slots[32];
+    int fd;
+    int vmfd;
+    int coalesced_mmio;
+    int broken_set_mem_region;
+    int migration_log;
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+    struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
+#endif
+};
+
+#endif
diff --git a/kvm.h b/kvm.h
index 560aef3..9cc64ac 100644
--- a/kvm.h
+++ b/kvm.h
@@ -16,6 +16,7 @@
 
 #include "config.h"
 #include "sys-queue.h"
+#include "kvm-shared.h"
 
 #ifdef CONFIG_KVM
 extern int kvm_allowed;
@@ -94,15 +95,6 @@ int kvm_arch_init_vcpu(CPUState *env);
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
-struct kvm_sw_breakpoint {
-    target_ulong pc;
-    target_ulong saved_insn;
-    int use_count;
-    TAILQ_ENTRY(kvm_sw_breakpoint) entry;
-};
-
-TAILQ_HEAD(kvm_sw_breakpoint_head, kvm_sw_breakpoint);
-
 int kvm_arch_debug(struct kvm_debug_exit_arch *arch_info);
 
 struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *env,
-- 
1.5.6.6

[Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Jan Kiszka]

Glauber Costa wrote:
> Following a suggestion given by Jan, the idea here is to
> move shared pieces between qemu and qemu-kvm.git into a common
> file, so we can do sharing while avoid clashes.
> 
> In the future, this files should disappear.
> 

OK for the header - but why do we have to push the ioctl services into a
separate module? Will all functions qemu-kvm start to use from upstream
have to be pushed around? Or what is special about the ioctls?

I rather think qemu-kvm should build kvm-all.c and #ifdef out those
parts which collide with its own implementation. Moreover, when we morph
qemu-kvm services for upstream, this could already happen where they
shall once be located: in kvm-all.c or target-*/kvm.c.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Avi Kivity]

Jan Kiszka wrote:
> Glauber Costa wrote:
>   
>> Following a suggestion given by Jan, the idea here is to
>> move shared pieces between qemu and qemu-kvm.git into a common
>> file, so we can do sharing while avoid clashes.
>>
>> In the future, this files should disappear.
>>
>>     
>
> OK for the header - but why do we have to push the ioctl services into a
> separate module? Will all functions qemu-kvm start to use from upstream
> have to be pushed around? Or what is special about the ioctls?
>
> I rather think qemu-kvm should build kvm-all.c and #ifdef out those
> parts which collide with its own implementation. Moreover, when we morph
> qemu-kvm services for upstream, this could already happen where they
> shall once be located: in kvm-all.c or target-*/kvm.c.
>   

Yes, we could simply append libkvm-all.c and qemu-kvm.c to kvm-all.c, 
and gradually include more of kvm-all.c as we delete parts of libkvm.c 
and qemu-kvm.c.

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

[Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Glauber Costa]

On Tue, Jun 09, 2009 at 06:17:25PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > Following a suggestion given by Jan, the idea here is to
> > move shared pieces between qemu and qemu-kvm.git into a common
> > file, so we can do sharing while avoid clashes.
> > 
> > In the future, this files should disappear.
> > 
> 
> OK for the header - but why do we have to push the ioctl services into a
> separate module? Will all functions qemu-kvm start to use from upstream
> have to be pushed around? Or what is special about the ioctls?
> 
> I rather think qemu-kvm should build kvm-all.c and #ifdef out those
> parts which collide with its own implementation. Moreover, when we morph
> qemu-kvm services for upstream, this could already happen where they
> shall once be located: in kvm-all.c or target-*/kvm.c.
I also think qemu-kvm should build kvm-all.c. But right now, it is so full
of conflicts, that it would be impossible. My go with the module, is, whenever
we move all the code in kvm-all.c away, it should mean it is safe to go back to
it: qemu-kvm will be granted not to conflict with it.

Re: [Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Glauber Costa]

On Tue, Jun 09, 2009 at 07:26:58PM +0300, Avi Kivity wrote:
> Jan Kiszka wrote:
>> Glauber Costa wrote:
>>   
>>> Following a suggestion given by Jan, the idea here is to
>>> move shared pieces between qemu and qemu-kvm.git into a common
>>> file, so we can do sharing while avoid clashes.
>>>
>>> In the future, this files should disappear.
>>>
>>>     
>>
>> OK for the header - but why do we have to push the ioctl services into a
>> separate module? Will all functions qemu-kvm start to use from upstream
>> have to be pushed around? Or what is special about the ioctls?
>>
>> I rather think qemu-kvm should build kvm-all.c and #ifdef out those
>> parts which collide with its own implementation. Moreover, when we morph
>> qemu-kvm services for upstream, this could already happen where they
>> shall once be located: in kvm-all.c or target-*/kvm.c.
>>   
>
> Yes, we could simply append libkvm-all.c and qemu-kvm.c to kvm-all.c,  
> and gradually include more of kvm-all.c as we delete parts of libkvm.c  
> and qemu-kvm.c.
I tried it myself, and it generates tons of conflicts. So scary.
I'd prefer to do it in the way I propose, until there is nothing left on kvm-all.c

Re: [Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Avi Kivity]

Glauber Costa wrote:
>> Yes, we could simply append libkvm-all.c and qemu-kvm.c to kvm-all.c,  
>> and gradually include more of kvm-all.c as we delete parts of libkvm.c  
>> and qemu-kvm.c.
>>     
> I tried it myself, and it generates tons of conflicts. So scary.
> I'd prefer to do it in the way I propose, until there is nothing left on kvm-all.c
>   

There shouldn't be any conflicts, kvm-all is ifdefed out.

We'd enable bits one by one until everything is enabled.

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

Re: [Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Jan Kiszka]

Glauber Costa wrote:
> On Tue, Jun 09, 2009 at 07:26:58PM +0300, Avi Kivity wrote:
>> Jan Kiszka wrote:
>>> Glauber Costa wrote:
>>>   
>>>> Following a suggestion given by Jan, the idea here is to
>>>> move shared pieces between qemu and qemu-kvm.git into a common
>>>> file, so we can do sharing while avoid clashes.
>>>>
>>>> In the future, this files should disappear.
>>>>
>>>>     
>>> OK for the header - but why do we have to push the ioctl services into a
>>> separate module? Will all functions qemu-kvm start to use from upstream
>>> have to be pushed around? Or what is special about the ioctls?
>>>
>>> I rather think qemu-kvm should build kvm-all.c and #ifdef out those
>>> parts which collide with its own implementation. Moreover, when we morph
>>> qemu-kvm services for upstream, this could already happen where they
>>> shall once be located: in kvm-all.c or target-*/kvm.c.
>>>   
>> Yes, we could simply append libkvm-all.c and qemu-kvm.c to kvm-all.c,  
>> and gradually include more of kvm-all.c as we delete parts of libkvm.c  
>> and qemu-kvm.c.
> I tried it myself, and it generates tons of conflicts. So scary.
> I'd prefer to do it in the way I propose, until there is nothing left on kvm-all.c
> 

kvm-all.c:

#ifdef DONT_USE_UPSTREAM_YET
<upstream code>
#endif

int kvm_ioctl(KVMState *s, int type, ...)
...

#ifdef DONT_USE_UPSTREAM_YET
<some more upstream code>
#endif

<libkvm code>


Can't imagine that this is infeasible. And it would all happily live in
qemu-kvm, so upstream would never see these hacks.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Glauber Costa]

On Tue, Jun 09, 2009 at 06:42:44PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Tue, Jun 09, 2009 at 07:26:58PM +0300, Avi Kivity wrote:
> >> Jan Kiszka wrote:
> >>> Glauber Costa wrote:
> >>>   
> >>>> Following a suggestion given by Jan, the idea here is to
> >>>> move shared pieces between qemu and qemu-kvm.git into a common
> >>>> file, so we can do sharing while avoid clashes.
> >>>>
> >>>> In the future, this files should disappear.
> >>>>
> >>>>     
> >>> OK for the header - but why do we have to push the ioctl services into a
> >>> separate module? Will all functions qemu-kvm start to use from upstream
> >>> have to be pushed around? Or what is special about the ioctls?
> >>>
> >>> I rather think qemu-kvm should build kvm-all.c and #ifdef out those
> >>> parts which collide with its own implementation. Moreover, when we morph
> >>> qemu-kvm services for upstream, this could already happen where they
> >>> shall once be located: in kvm-all.c or target-*/kvm.c.
> >>>   
> >> Yes, we could simply append libkvm-all.c and qemu-kvm.c to kvm-all.c,  
> >> and gradually include more of kvm-all.c as we delete parts of libkvm.c  
> >> and qemu-kvm.c.
> > I tried it myself, and it generates tons of conflicts. So scary.
> > I'd prefer to do it in the way I propose, until there is nothing left on kvm-all.c
> > 
> 
> kvm-all.c:
> 
> #ifdef DONT_USE_UPSTREAM_YET
> <upstream code>
> #endif
> 
> int kvm_ioctl(KVMState *s, int type, ...)
> ...
> 
> #ifdef DONT_USE_UPSTREAM_YET
> <some more upstream code>
> #endif
> 
> <libkvm code>
> 
> 
> Can't imagine that this is infeasible. And it would all happily live in
> qemu-kvm, so upstream would never see these hacks.
yeah, if it is in fact ifdef'd this way, it of course works.

I can do that, definitely.
But by doing that, we probably does not even need the header part here too.

Re: [Qemu-devel] Re: [PATCH] create kvm-shared-all.c and kvm-shared.h

[Jan Kiszka]

Glauber Costa wrote:
> On Tue, Jun 09, 2009 at 06:42:44PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Tue, Jun 09, 2009 at 07:26:58PM +0300, Avi Kivity wrote:
>>>> Jan Kiszka wrote:
>>>>> Glauber Costa wrote:
>>>>>   
>>>>>> Following a suggestion given by Jan, the idea here is to
>>>>>> move shared pieces between qemu and qemu-kvm.git into a common
>>>>>> file, so we can do sharing while avoid clashes.
>>>>>>
>>>>>> In the future, this files should disappear.
>>>>>>
>>>>>>     
>>>>> OK for the header - but why do we have to push the ioctl services into a
>>>>> separate module? Will all functions qemu-kvm start to use from upstream
>>>>> have to be pushed around? Or what is special about the ioctls?
>>>>>
>>>>> I rather think qemu-kvm should build kvm-all.c and #ifdef out those
>>>>> parts which collide with its own implementation. Moreover, when we morph
>>>>> qemu-kvm services for upstream, this could already happen where they
>>>>> shall once be located: in kvm-all.c or target-*/kvm.c.
>>>>>   
>>>> Yes, we could simply append libkvm-all.c and qemu-kvm.c to kvm-all.c,  
>>>> and gradually include more of kvm-all.c as we delete parts of libkvm.c  
>>>> and qemu-kvm.c.
>>> I tried it myself, and it generates tons of conflicts. So scary.
>>> I'd prefer to do it in the way I propose, until there is nothing left on kvm-all.c
>>>
>> kvm-all.c:
>>
>> #ifdef DONT_USE_UPSTREAM_YET
>> <upstream code>
>> #endif
>>
>> int kvm_ioctl(KVMState *s, int type, ...)
>> ...
>>
>> #ifdef DONT_USE_UPSTREAM_YET
>> <some more upstream code>
>> #endif
>>
>> <libkvm code>
>>
>>
>> Can't imagine that this is infeasible. And it would all happily live in
>> qemu-kvm, so upstream would never see these hacks.
> yeah, if it is in fact ifdef'd this way, it of course works.
> 
> I can do that, definitely.
> But by doing that, we probably does not even need the header part here too.
> 

I think we need it as long as qemu-kvm-<arch> fiddles with state fields
directly. If that is easily resolvable, I'm all for doing that first and
skipping the kvm-shared.h part in upstream!

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux