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

[Alexander Graf]

On 23.12.2013, at 19:03, Andreas Färber <address@hidden> wrote:

> 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)

I agree to the others, but that particular one is very consistent with the code 
around it. I don't want two different error reporting paths in the same 
function.

> 
>> +        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.

Fixed them inline.


Alex