| Message ID | 4BB606BE.9000508@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/02/2010 10:01 AM, Paolo Bonzini wrote: > On 04/01/2010 10:27 PM, Blue Swirl wrote: >> It will not be safe to use kvm_enabled() in vl.c, so there needs to be >> a target dependent helper. The call to kvm_init could remain in vl.c, >> likewise it's not strictly needed to move kvm_allowed to arch_init.c. > > Not really, because kvm_allowed _can_ be used from once-compiled > files. The attached patch makes kvm-all.c be compiled always, even if > !CONFIG_KVM; functions that require kvm are almost always omitted for > now (so checking kvm_enabled() is required to call them). However, > kvm_init is stubbed so that vl.c can call it. > > In the future we could add more stubbing and ultimately do this > unconditionally: > > #define kvm_enabled() kvm_allowed > > With this patch vl.c can already be compiled once, but I did not > include it because it would conflict with my balloon.c series; I'm > doing enough rebasing these days. Also, qemu-kvm is a bit behind qemu > and all these patches are nightmares for the merges, so it's better > IMO if things are left to calm down a bit. Having kvm-all.c compile with and without CONFIG_KVM is pretty ugly IMHO. Is compiling vl.c once really that important of a goal? Wouldn't it be better to split bits out of vl.c and have those compile once? Ideally, vl.c should be so small that compiling per-target shouldn't matter. Regards, Anthony Liguori >> They just caused problems with the poisoning check in patch 2/2. > > Poisoning kvm_enabled is right, but poisoning kvm_allowed is overkill. > > Paolo
On 04/02/2010 05:08 PM, Anthony Liguori wrote: > Having kvm-all.c compile with and without CONFIG_KVM is pretty ugly IMHO. > > Is compiling vl.c once really that important of a goal? I didn't do this to compile vl.c once. I don't care about that. I did this as an initial step towards having kvm functions stubbed out for !CONFIG_KVM, instead of relying on GCC performing dead-code-elimination on kvm_enabled(). Paolo
On 04/02/2010 10:11 AM, Paolo Bonzini wrote: > On 04/02/2010 05:08 PM, Anthony Liguori wrote: >> Having kvm-all.c compile with and without CONFIG_KVM is pretty ugly >> IMHO. >> >> Is compiling vl.c once really that important of a goal? > > I didn't do this to compile vl.c once. I don't care about that. > > I did this as an initial step towards having kvm functions stubbed out > for !CONFIG_KVM, instead of relying on GCC performing > dead-code-elimination on kvm_enabled(). I'd prefer a kvm-stub.c implementation as opposed to mixing in CONFIG_KVM in kvm-all.c Regards, Anthony Liguori > Paolo
On 4/2/10, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 04/02/2010 10:01 AM, Paolo Bonzini wrote: > > > On 04/01/2010 10:27 PM, Blue Swirl wrote: > > > > > It will not be safe to use kvm_enabled() in vl.c, so there needs to be > > > a target dependent helper. The call to kvm_init could remain in vl.c, > > > likewise it's not strictly needed to move kvm_allowed to arch_init.c. > > > > > > > Not really, because kvm_allowed _can_ be used from once-compiled files. > The attached patch makes kvm-all.c be compiled always, even if !CONFIG_KVM; > functions that require kvm are almost always omitted for now (so checking > kvm_enabled() is required to call them). However, kvm_init is stubbed so > that vl.c can call it. > > > > In the future we could add more stubbing and ultimately do this > unconditionally: > > > > #define kvm_enabled() kvm_allowed > > > > With this patch vl.c can already be compiled once, but I did not include > it because it would conflict with my balloon.c series; I'm doing enough > rebasing these days. Also, qemu-kvm is a bit behind qemu and all these > patches are nightmares for the merges, so it's better IMO if things are left > to calm down a bit. > > > > Having kvm-all.c compile with and without CONFIG_KVM is pretty ugly IMHO. > > Is compiling vl.c once really that important of a goal? Wouldn't it be > better to split bits out of vl.c and have those compile once? Ideally, vl.c > should be so small that compiling per-target shouldn't matter. But the only thing preventing compiling whole vl.c once is just KVM init. I'll send a new patch, hopefully it's better.
On 04/02/2010 05:20 PM, Anthony Liguori wrote: >> >> I didn't do this to compile vl.c once. I don't care about that. >> >> I did this as an initial step towards having kvm functions stubbed out >> for !CONFIG_KVM, instead of relying on GCC performing >> dead-code-elimination on kvm_enabled(). > > I'd prefer a kvm-stub.c implementation as opposed to mixing in > CONFIG_KVM in kvm-all.c I tried it, but our build system makes it a mess because compile-once-only can only be done using what once were static libraries. All files from there are added blindly: obj-y += $(addprefix ../, $(common-obj-y)) obj-y += $(addprefix ../libdis/, $(libdis-y)) obj-y += $(libobj-y) obj-y += $(addprefix $(HWDIR)/, $(hw-obj-y)) I'll try something. Paolo
diff --git a/Makefile.target b/Makefile.target index 167fc8d..3943de1 100644 --- a/Makefile.target +++ b/Makefile.target @@ -168,8 +168,8 @@ obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o virtio-serial-b obj-y += event_notifier.o obj-y += vhost_net.o obj-$(CONFIG_VHOST_NET) += vhost.o -obj-y += rwhandler.o -obj-$(CONFIG_KVM) += kvm.o kvm-all.o +obj-y += rwhandler.o kvm-all.o +obj-$(CONFIG_KVM) += kvm.o LIBS+=-lz QEMU_CFLAGS += $(VNC_TLS_CFLAGS) diff --git a/kvm-all.c b/kvm-all.c index 7aa5e57..53f58a6 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -13,14 +13,16 @@ * */ +#include "qemu-common.h" #include <sys/types.h> #include <sys/ioctl.h> #include <sys/mman.h> #include <stdarg.h> +#ifdef CONFIG_KVM #include <linux/kvm.h> +#endif -#include "qemu-common.h" #include "qemu-barrier.h" #include "sysemu.h" #include "hw/hw.h" @@ -73,6 +75,7 @@ struct KVMState static KVMState *kvm_state; +#ifdef CONFIG_KVM static KVMSlot *kvm_alloc_slot(KVMState *s) { int i; @@ -282,7 +285,7 @@ static int kvm_set_migration_log(int enable) return 0; } -static int test_le_bit(unsigned long nr, unsigned char *addr) +static inline int test_le_bit(unsigned long nr, unsigned char *addr) { return (addr[nr >> 3] >> (nr & 7)) & 1; } @@ -561,9 +564,11 @@ static CPUPhysMemoryClient kvm_cpu_phys_memory_client = { .sync_dirty_bitmap = kvm_client_sync_dirty_bitmap, .migration_log = kvm_client_migration_log, }; +#endif int kvm_init(int smp_cpus) { +#ifdef CONFIG_KVM static const char upgrade_note[] = "Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n" "(see http://sourceforge.net/projects/kvm).\n"; @@ -683,8 +688,12 @@ err: qemu_free(s); return ret; +#else + return -ENOSYS; +#endif } +#ifdef CONFIG_KVM static int kvm_handle_io(uint16_t port, void *data, int direction, int size, uint32_t count) { @@ -866,6 +875,7 @@ int kvm_cpu_exec(CPUState *env) return ret; } +#endif int kvm_ioctl(KVMState *s, int type, ...) { @@ -1139,6 +1149,7 @@ void kvm_remove_all_breakpoints(CPUState *current_env) } #endif /* !KVM_CAP_SET_GUEST_DEBUG */ +#ifdef CONFIG_KVM int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset) { struct kvm_signal_mask *sigmask; @@ -1156,6 +1167,7 @@ int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset) return r; } +#endif #ifdef KVM_IOEVENTFD int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val, bool assign) diff --git a/kvm.h b/kvm.h index 1e5be27..2477cfd 100644 --- a/kvm.h +++ b/kvm.h @@ -18,13 +18,15 @@ #include <errno.h> #include "config-host.h" #include "qemu-queue.h" +#include "cpu-common.h" #ifdef CONFIG_KVM #include <linux/kvm.h> #endif -#ifdef CONFIG_KVM extern int kvm_allowed; + +#ifdef CONFIG_KVM #define kvm_enabled() (kvm_allowed) #else #define kvm_enabled() (0) diff --git a/vl.c b/vl.c index 6768cf1..9fe4682 100644 --- a/vl.c +++ b/vl.c @@ -3235,10 +3235,6 @@ int main(int argc, char **argv, char **envp) do_smbios_option(optarg); break; case QEMU_OPTION_enable_kvm: - if (!(kvm_available())) { - printf("Option %s not supported for this target\n", popt->name); - exit(1); - } kvm_allowed = 1; break; case QEMU_OPTION_usb: @@ -3585,12 +3581,14 @@ int main(int argc, char **argv, char **envp) exit(1); } - if (kvm_enabled()) { - int ret; - - ret = kvm_init(smp_cpus); + if (kvm_allowed) { + int ret = kvm_init(smp_cpus); if (ret < 0) { - fprintf(stderr, "failed to initialize KVM\n"); + if (!kvm_available()) { + printf("KVM not supported for this target\n"); + } else { + fprintf(stderr, "failed to initialize KVM: %s\n", strerror(-ret)); + } exit(1); } }