Re: [Qemu-devel] [PATCH] Initial support for Ilumos build and Illumos-kvm

[Jan Kiszka]

On 2012-03-16 10:23, Lee Essen wrote:
> This fixes a number of issues with the build process (namely ensuring the use 
> of bash), adds specific support for the Illumos port of KVM and fixes a few 
> general Solaris compatibility issues.
> 
> There are still some things outstanding:
> 
> - there's a duplicate smb_wmb() definition in qemu-barrier.h and the illumos 
> kvm_x86.h which generates some warnings.
> - there's a repeated call to page_size() that should probably be fixed.
> - dtrace support needs to be fixed (-m64/32 option, reserved words and 
> linking issues)
> - vnics need to be added
> - the original illumos code added another timer source (multiticks)
> - the issue with Linux needs to be resolved
> 
> Other than that, this gets it to the point where it will build and run with 
> illumos kvm, and works fine for Windows.
> 
> It's my first patch to qemu, and most of the real kvm stuff has come from the 
> original illumos-kvm-cmd tree, so be gentle with me!
> 
> 
> Signed-off-by: Lee Essen <address@hidden>
> 
> --
>  Makefile.objs              |    6 +-
>  Makefile.target            |    6 +-
>  configure                  |    8 +++-
>  cpus.c                     |    4 +-
>  exec.c                     |   88 
> ++++++++++++++++++++++++++++++++++++++++++++
>  fpu/softfloat-specialize.h |    4 ++
>  hw/kvm/clock.c             |    4 ++
>  kvm-all.c                  |   81 ++++++++++++++++++++++++++++++++++++++++-
>  kvm.h                      |   15 +++++++
>  qemu-timer.c               |   10 ++--
>  qga/channel-posix.c        |   16 ++++++++
>  qga/commands-posix.c       |    9 ++++
>  target-i386/hyperv.h       |    4 ++
>  target-i386/kvm.c          |   53 +++++++++++++++++++++++++-
>  14 files changed, 291 insertions(+), 17 deletions(-)
> 

...

> diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> index 446bd62..3fd5e1e 100644
> --- a/hw/kvm/clock.c
> +++ b/hw/kvm/clock.c
> @@ -19,8 +19,12 @@
>  #include "hw/sysbus.h"
>  #include "hw/kvm/clock.h"
>  
> +#ifdef __sun__
> +#include <sys/kvm.h>
> +#else
>  #include <linux/kvm.h>
>  #include <linux/kvm_para.h>
> +#endif

As Paolo already said, this should somehow be centralized.

Also, CONFIG_SOLARIS vs. __sun__: please use a consistent pattern.

>  
>  typedef struct KVMClockState {
>      SysBusDevice busdev;
> diff --git a/kvm-all.c b/kvm-all.c
> index 42e5e23..27f3177 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -18,7 +18,11 @@
>  #include <sys/mman.h>
>  #include <stdarg.h>
>  
> +#ifdef __sun__
> +#include <sys/kvm.h>
> +#else
>  #include <linux/kvm.h>
> +#endif
>  
>  #include "qemu-common.h"
>  #include "qemu-barrier.h"
> @@ -176,12 +180,23 @@ int kvm_physical_memory_addr_from_host(KVMState *s, 
> void *ram,
>  static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>  {
>      struct kvm_userspace_memory_region mem;
> +#ifdef CONFIG_SOLARIS
> +    caddr_t p;
> +    char c;
> +#endif
>  
>      mem.slot = slot->slot;
>      mem.guest_phys_addr = slot->start_addr;
>      mem.memory_size = slot->memory_size;
>      mem.userspace_addr = (unsigned long)slot->ram;
>      mem.flags = slot->flags;
> +#ifdef CONFIG_SOLARIS
> +    for (p = (caddr_t)mem.userspace_addr;
> +      p < (caddr_t)mem.userspace_addr + mem.memory_size;
> +      p += PAGE_SIZE)
> +        c = *p;
> +#endif /* CONFIG_SOLARIS */
> +

I bet gcc will like this write-only pattern and bark at you.

>      if (s->migration_log) {
>          mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
>      }
> @@ -200,6 +215,31 @@ int kvm_pit_in_kernel(void)
>      return kvm_state->pit_in_kernel;
>  }
>  
> +#ifdef CONFIG_SOLARIS
> +static int kvm_vm_clone(KVMState *s)
> +{
> +    struct stat stat;
> +    int fd;
> +
> +    if (fstat(s->fd, &stat) != 0) {
> +        return -errno;
> +    }
> +
> +    fd = qemu_open("/dev/kvm", O_RDWR);
> +
> +    if (fd == -1) {
> +        return -errno;
> +    }
> +
> +    if (ioctl(fd, KVM_CLONE, stat.st_rdev) == -1) {
> +        close(fd);
> +        return -errno;
> +    }
> +
> +    return fd;
> +}
> +#endif
> +
>  int kvm_init_vcpu(CPUArchState *env)
>  {
>      KVMState *s = kvm_state;
> @@ -208,14 +248,29 @@ int kvm_init_vcpu(CPUArchState *env)
>  
>      DPRINTF("kvm_init_vcpu\n");
>  
> +#ifdef CONFIG_SOLARIS
> +    ret = kvm_vm_clone(kvm_state);
> +
> +    if (ret < 0) {
> +        fprintf(stderr, "kvm_init_vcpu could not clone fd: %m\n");
> +        goto err;
> +    }
> +    env->kvm_fd = ret;
> +    env->kvm_state = kvm_state;
> +
> +    ret = ioctl(env->kvm_fd, KVM_CREATE_VCPU, env->cpu_index);

kvm_vcpu_ioctl

> +#else
>      ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
> +#endif

There is no chance to fix the Solaris KVM to do fd cloning in the kernel
and implement the same KVM_CREATE_VCPU ABI?

>      if (ret < 0) {
>          DPRINTF("kvm_create_vcpu failed\n");
>          goto err;
>      }
>  
> +#ifndef CONFIG_SOLARIS
>      env->kvm_fd = ret;
>      env->kvm_state = s;
> +#endif
>      env->kvm_vcpu_dirty = 1;
>  
>      mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
> @@ -1021,6 +1076,9 @@ int kvm_init(void)
>          ret = s->vmfd;
>          goto err;
>      }
> +#ifdef CONFIG_SOLARIS
> +    s->vmfd = s->fd;
> +#endif
>  
>      missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
>      if (!missing_cap) {
> @@ -1287,6 +1345,19 @@ int kvm_cpu_exec(CPUArchState *env)
>              DPRINTF("irq_window_open\n");
>              ret = EXCP_INTERRUPT;
>              break;
> +#ifdef CONFIG_SOLARIS
> +        /*
> +         * In the case of an external interrupt we can get a zero
> +         * return from the ioctl, with a KVM_EXIT_INTR. This doesn't
> +         * happen on linux
> +         *
> +         * Not entirely sure what to do here.

Fix the kernel?

> +         */
> +        case KVM_EXIT_INTR:
> +            DPRINTF("exit_intr (run_ret is %d)\n", run_ret);
> +            ret = EXCP_INTERRUPT;
> +            break;
> +#endif
>          case KVM_EXIT_SHUTDOWN:
>              DPRINTF("shutdown\n");
>              qemu_system_reset_request();
> @@ -1631,7 +1702,7 @@ int kvm_set_signal_mask(CPUArchState *env, const 
> sigset_t *sigset)
>  
>      sigmask = g_malloc(sizeof(*sigmask) + sizeof(*sigset));
>  
> -    sigmask->len = 8;
> +    sigmask->len = sizeof(sigset_t);
>      memcpy(sigmask->sigset, sigset, sizeof(*sigset));
>      r = kvm_vcpu_ioctl(env, KVM_SET_SIGNAL_MASK, sigmask);
>      g_free(sigmask);
> @@ -1641,6 +1712,7 @@ int kvm_set_signal_mask(CPUArchState *env, const 
> sigset_t *sigset)
>  
>  int kvm_set_ioeventfd_mmio_long(int fd, uint32_t addr, uint32_t val, bool 
> assign)
>  {
> +#ifdef CONFIG_EVENTFD
>      int ret;
>      struct kvm_ioeventfd iofd;
>  
> @@ -1665,10 +1737,14 @@ int kvm_set_ioeventfd_mmio_long(int fd, uint32_t 
> addr, uint32_t val, bool assign
>      }
>  
>      return 0;
> +#else
> +    return -ENOSYS;
> +#endif
>  }
>  
>  int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val, bool 
> assign)
>  {
> +#ifdef CONFIG_EVENTFD
>      struct kvm_ioeventfd kick = {
>          .datamatch = val,
>          .addr = addr,
> @@ -1688,6 +1764,9 @@ int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, 
> uint16_t val, bool assign)
>          return r;
>      }
>      return 0;
> +#else
> +    return -ENOSYS;
> +#endif
>  }
>  
>  int kvm_on_sigbus_vcpu(CPUArchState *env, int code, void *addr)
> diff --git a/kvm.h b/kvm.h
> index 330f17b..8960b4e 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -19,8 +19,23 @@
>  #include "qemu-queue.h"
>  
>  #ifdef CONFIG_KVM
> +#ifdef __sun__
> +#include <sys/kvm.h>
> +/*
> + * it's a bit horrible to include these here, but the kvm_para.h include file
> + * isn't public with the illumos kvm implementation

Just provide a package of properly fixed kernel headers and let us carry
them in solaris-headers or so, analogously to linux-headers.

> + */
> +#define KVM_CPUID_SIGNATURE       0x40000000
> +#define KVM_CPUID_FEATURES        0x40000001
> +#define KVM_FEATURE_CLOCKSOURCE   0
> +#define KVM_FEATURE_NOP_IO_DELAY  1
> +#define KVM_FEATURE_MMU_OP        2
> +#define KVM_FEATURE_CLOCKSOURCE2  3
> +#define HYPERV_CPUID_MIN          0x40000005
> +#else
>  #include <linux/kvm.h>
>  #endif
> +#endif
>  
>  extern int kvm_allowed;
>  extern bool kvm_kernel_irqchip;

...

> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index e74a9e4..6d007cc 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -17,8 +17,12 @@
>  #include <sys/mman.h>
>  #include <sys/utsname.h>
>  
> +#ifdef __sun__
> +#include <sys/kvm.h>
> +#else
>  #include <linux/kvm.h>
>  #include <linux/kvm_para.h>
> +#endif
>  
>  #include "qemu-common.h"
>  #include "sysemu.h"
> @@ -61,7 +65,9 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>  static bool has_msr_star;
>  static bool has_msr_hsave_pa;
>  static bool has_msr_tsc_deadline;
> +#ifdef KVM_CAP_ASYNC_PF
>  static bool has_msr_async_pf_en;
> +#endif

NACK. With proper kernel headers, all these KVM_CAP reintroductions
become obsolete again.

Note that KVM changes need to CC the kvm mailing list and will likely
flow via uq/master of qemu-kvm.git.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux