Re: [Qemu-ppc] [Qemu-devel] [PATCH -V5] kvm: Add a new machine property kvm-type

[Andreas Färber]

Am 23.12.2013 16:40, schrieb Aneesh Kumar K.V:
> From: "Aneesh Kumar K.V" <address@hidden>
> 
> Targets like ppc64 support different typed of KVM, one which use

"types" - Alex, please fix. :)

> hypervisor mode and the other which doesn't. Add a new machine
> property kvm-type that helps in selecting the respective ones

There is no property being added in this patch, it's an "option". Please
edit, also in subject.

> We also add a new QEMUMachine callback get_vm_type that helps
> in mapping the string representation of kvm type specified.
> 
> Signed-off-by: Aneesh Kumar K.V <address@hidden>
> ---
> Changes from V4:
> * Fix build failure for ppc{,64}-linux-user
> 
>  hw/ppc/spapr.c         | 19 +++++++++++++++++++
>  include/hw/boards.h    |  3 +++
>  include/hw/xen/xen.h   |  3 ++-
>  include/sysemu/kvm.h   |  4 ++--
>  include/sysemu/qtest.h |  3 ++-
>  kvm-all.c              | 16 +++++++++++++---
>  kvm-stub.c             |  3 ++-
>  qtest.c                |  2 +-
>  vl.c                   | 14 +++++++++-----
>  xen-all.c              |  2 +-
>  xen-stub.c             |  2 +-
>  11 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7e53a5f97781..267a47d6cc4d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1357,6 +1357,24 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      assert(spapr->fdt_skel != NULL);
>  }
>  
> +static int spapr_kvm_type(const char *vm_type)
> +{
> +    if (!vm_type) {
> +        return 0;
> +    }
> +
> +    if (!strcmp(vm_type, "HV")) {
> +        return 1;
> +    }
> +
> +    if (!strcmp(vm_type, "PR")) {
> +        return 2;
> +    }
> +
> +    hw_error("Unknown kvm-type specified '%s'", vm_type);

error_report() - hw_error() would need \n IIRC and dumps CPU state,
which seems useless at that point.

> +    exit(1);
> +}
> +
>  static QEMUMachine spapr_machine = {
>      .name = "pseries",
>      .desc = "pSeries Logical Partition (PAPR compliant)",
> @@ -1367,6 +1385,7 @@ static QEMUMachine spapr_machine = {
>      .max_cpus = MAX_CPUS,
>      .no_parallel = 1,
>      .default_boot_order = NULL,
> +    .kvm_type = spapr_kvm_type,
>  };
>  
>  static void spapr_machine_init(void)
[...]
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 3b25f27a7cc5..400a2682923e 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -151,8 +151,8 @@ typedef struct KVMState KVMState;
>  extern KVMState *kvm_state;
>  
>  /* external API */
> -

Line dropped accidentally? I guess external API doesn't just apply to
the two lines added...

> -int kvm_init(void);
> +typedef struct QEMUMachine QEMUMachine;
> +int kvm_init(QEMUMachine *machine);
>  
>  int kvm_has_sync_mmu(void);
>  int kvm_has_vcpu_events(void);
> diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
> index 112a661ac4b0..7185174a39b2 100644
> --- a/include/sysemu/qtest.h
> +++ b/include/sysemu/qtest.h
> @@ -23,7 +23,8 @@ static inline bool qtest_enabled(void)
>      return qtest_allowed;
>  }
>  
> -int qtest_init_accel(void);
> +typedef struct QEMUMachine QEMUMachine;
> +int qtest_init_accel(QEMUMachine *machine);
>  void qtest_init(const char *qtest_chrdev, const char *qtest_log);
>  
>  static inline int qtest_available(void)
> diff --git a/kvm-all.c b/kvm-all.c
> index 393775459d9f..57472804fe44 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -35,6 +35,8 @@
>  #include "qemu/event_notifier.h"
>  #include "trace.h"
>  
> +#include "hw/boards.h"
> +
>  /* This check must be after config-host.h is included */
>  #ifdef CONFIG_EVENTFD
>  #include <sys/eventfd.h>
> @@ -1352,7 +1354,7 @@ static int kvm_max_vcpus(KVMState *s)
>      return (ret) ? ret : kvm_recommended_vcpus(s);
>  }
>  
> -int kvm_init(void)
> +int kvm_init(QEMUMachine *machine)
>  {
>      static const char upgrade_note[] =
>          "Please upgrade to at least kernel 2.6.29 or recent kvm-kmod\n"
> @@ -1369,7 +1371,8 @@ int kvm_init(void)
>      KVMState *s;
>      const KVMCapabilityInfo *missing_cap;
>      int ret;
> -    int i;
> +    int i, type = 0;
> +    const char *kvm_type;
>  
>      s = g_malloc0(sizeof(KVMState));
>  
> @@ -1442,7 +1445,14 @@ int kvm_init(void)
>          nc++;
>      }
>  
> -    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> +    kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
> +    if (machine->kvm_type) {
> +        type = machine->kvm_type(kvm_type);
> +    } else if (kvm_type) {
> +        fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);

error_report()? (without \n then)

> +        goto err;
> +    }
> +    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, type);
>      if (s->vmfd < 0) {
>  #ifdef TARGET_S390X
>          fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
[snip]

Almost all remarks could be edited into the patch file on the same line
to avoid yet another respin.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg