| Message ID | 0de4c8d7d8f9a7e6fdfc1f161f7c95b6a2866f3e.1267122331.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 ...
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
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(-)