Patchwork [PATCHv2,02/12] kvm: add API to set ioeventfd

login
register
mail settings
Submitter Michael S. Tsirkin
Date Feb. 25, 2010, 6:28 p.m.
Message ID <0de4c8d7d8f9a7e6fdfc1f161f7c95b6a2866f3e.1267122331.git.mst@redhat.com>
Download mbox | patch
Permalink /patch/46263/
State New
Headers show

Comments

Michael S. Tsirkin - Feb. 25, 2010, 6:28 p.m.
Comment on kvm usage: rather than require users to do if (kvm_enabled())
and/or ifdefs, this patch adds an API that, internally, is defined to
stub function on non-kvm build, and checks kvm_enabled for non-kvm
run.

While rest of qemu code still uses if (kvm_enabled()), I think this
approach is cleaner, and we should convert rest of code to it
long term.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Avi, Marcelo, pls review/ack.

 kvm-all.c |   22 ++++++++++++++++++++++
 kvm.h     |   16 ++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)
Anthony Liguori - Feb. 25, 2010, 7:19 p.m.
On 02/25/2010 12:28 PM, Michael S. Tsirkin wrote:
> Comment on kvm usage: rather than require users to do if (kvm_enabled())
> and/or ifdefs, this patch adds an API that, internally, is defined to
> stub function on non-kvm build, and checks kvm_enabled for non-kvm
> run.
>
> While rest of qemu code still uses if (kvm_enabled()), I think this
> approach is cleaner, and we should convert rest of code to it
> long term.
>    

I'm not opposed to that.

> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>
> Avi, Marcelo, pls review/ack.
>
>   kvm-all.c |   22 ++++++++++++++++++++++
>   kvm.h     |   16 ++++++++++++++++
>   2 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 1a02076..9742791 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1138,3 +1138,25 @@ int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset)
>
>       return r;
>   }
> +
> +#ifdef KVM_IOEVENTFD
> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
>    

I think this API could use some love.  You're using a very limited set 
of things that ioeventfd can do and you're multiplexing creation and 
destruction in a single call.

I think:

kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t data);
kvm_unset_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t data);

Would be better.  Alternatively, an API that matched ioeventfd exactly:

kvm_set_ioeventfd(int fd, uint64_t addr, int size, uint64_t data, int 
flags);
kvm_unset_ioeventfd(...);

Could work too.

Regards,

Anthony Liguori
Michael S. Tsirkin - March 2, 2010, 5:41 p.m.
On Thu, Feb 25, 2010 at 01:19:30PM -0600, Anthony Liguori wrote:
> On 02/25/2010 12:28 PM, Michael S. Tsirkin wrote:
>> Comment on kvm usage: rather than require users to do if (kvm_enabled())
>> and/or ifdefs, this patch adds an API that, internally, is defined to
>> stub function on non-kvm build, and checks kvm_enabled for non-kvm
>> run.
>>
>> While rest of qemu code still uses if (kvm_enabled()), I think this
>> approach is cleaner, and we should convert rest of code to it
>> long term.
>>    
>
> I'm not opposed to that.
>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> ---
>>
>> Avi, Marcelo, pls review/ack.
>>
>>   kvm-all.c |   22 ++++++++++++++++++++++
>>   kvm.h     |   16 ++++++++++++++++
>>   2 files changed, 38 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 1a02076..9742791 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1138,3 +1138,25 @@ int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset)
>>
>>       return r;
>>   }
>> +
>> +#ifdef KVM_IOEVENTFD
>> +int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
>>    
>
> I think this API could use some love.  You're using a very limited set  
> of things that ioeventfd can do and you're multiplexing creation and  
> destruction in a single call.
>
> I think:
>
> kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t data);
> kvm_unset_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t data);
>
> Would be better.  Alternatively, an API that matched ioeventfd exactly:
>
> kvm_set_ioeventfd(int fd, uint64_t addr, int size, uint64_t data, int  
> flags);
> kvm_unset_ioeventfd(...);
>
> Could work too.
>
> Regards,
>
> Anthony Liguori
>

So I renamed to kvm_set_ioeventfd_pio_word, but I still left assign
boolean in place because both implementation and usage take up
less code this way.

It's just an internal function, so no biggie to change it later ...

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 1a02076..9742791 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1138,3 +1138,25 @@  int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset)
 
     return r;
 }
+
+#ifdef KVM_IOEVENTFD
+int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
+{
+    struct kvm_ioeventfd kick = {
+        .datamatch = data,
+        .addr = addr,
+        .len = 2,
+        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
+        .fd = fd,
+    };
+    int r;
+    if (!kvm_enabled())
+        return -ENOSYS;
+    if (!assigned)
+        kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+    r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick);
+    if (r < 0)
+        return r;
+    return 0;
+}
+#endif
diff --git a/kvm.h b/kvm.h
index a74dfcb..897efb7 100644
--- a/kvm.h
+++ b/kvm.h
@@ -14,10 +14,16 @@ 
 #ifndef QEMU_KVM_H
 #define QEMU_KVM_H
 
+#include <stdbool.h>
+#include <errno.h>
 #include "config.h"
 #include "qemu-queue.h"
 
 #ifdef CONFIG_KVM
+#include <linux/kvm.h>
+#endif
+
+#ifdef CONFIG_KVM
 extern int kvm_allowed;
 
 #define kvm_enabled() (kvm_allowed)
@@ -135,4 +141,14 @@  static inline void cpu_synchronize_state(CPUState *env)
     }
 }
 
+#if defined(KVM_IOEVENTFD) && defined(CONFIG_KVM)
+int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned);
+#else
+static inline
+int kvm_set_ioeventfd(uint16_t data, uint16_t addr, int fd, bool assigned)
+{
+    return -ENOSYS;
+}
+#endif
+
 #endif