| Message ID | 1430146411-34632-16-git-send-email-mimu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote: [...] > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c > index 4d75ff0..94fede5 100644 > --- a/target-s390x/kvm.c > +++ b/target-s390x/kvm.c > @@ -276,12 +276,59 @@ static int cpu_model_set(KVMState *s, uint64_t attr, void *addr) > return rc; > } > > -static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop) This seems to duplicate lots of the existing KVM code. (See additional comment below about possible ways to avoid it). > +static int get_machine_props_fallback(S390MachineProps *prop) > +{ > + struct kvm_device_attr dev_attr; > + int rc, kvmfd = -1, vmfd = -1; > + > + rc = qemu_open("/dev/kvm", O_RDWR); > + if (rc < 0) { > + goto out_err; > + } > + kvmfd = rc; > + > + rc = ioctl(kvmfd, KVM_CREATE_VM, 0); > + if (rc < 0) { > + goto out_err; > + } > + vmfd = rc; > + > + rc = ioctl(vmfd, KVM_CHECK_EXTENSION, KVM_CAP_VM_ATTRIBUTES); > + if (rc < 0) { > + rc = -ENOSYS; > + goto out_err; > + } > + > + dev_attr.group = KVM_S390_VM_CPU_MODEL; > + dev_attr.attr = KVM_S390_VM_CPU_MACHINE; > + rc = ioctl(vmfd, KVM_HAS_DEVICE_ATTR, &dev_attr); > + if (rc < 0) { > + rc = -EFAULT; > + goto out_err; > + } > + > + dev_attr.addr = (uint64_t) prop; > + rc = ioctl(vmfd, KVM_GET_DEVICE_ATTR, &dev_attr); > + > +out_err: > + if (vmfd >= 0) { > + close(vmfd); > + } > + if (kvmfd >= 0) { > + close(kvmfd); > + } > + > + return rc; > +} > + > +int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop) > { > int rc = -EFAULT; > > if (s) { > rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, prop); > + } else { > + rc = get_machine_props_fallback(prop); > } The comments below are just suggestions, not something which should block the patch, in my opinion: First, if s is always NULL inside arch_query_cpu_definitions(), and is always non-NULL inside kvm_setup_cpu_classes(), why don't you just call keep the original kvm_s390_get_machine_props() function, and call and get_machine_props_fallback() inside arch_query_cpu_definitions()? The only thing common to both cases is the tracing point, but if we are running two completely different code paths I assume it would be a good thing to have a different tracing point for get_machine_props_fallback(). Second, you shouldn't even need to duplicate code in get_machine_props_fallback() if you are able to create an accel object and do just basic initialization so that cpu_model_get() works. Allowing accel objects to be created on the fly was one of the main purposes of the accel QOM work. For example, if we do something like this: https://github.com/ehabkost/qemu-hacks/commit/36a250e34c5fd0d43a25271f5bc9b04681fdd56a [1] https://github.com/ehabkost/qemu-hacks/commits/work/accel-open-func Then the code could look like this: accel.c: /* configure_accelerator() would be changed to reuse this function: */ AccelState *accel_create(const char *accel_name) { AccelClass *acc = accel_find(accel_name); /*TODO: error handling, checking acc->available() */ return ACCEL(object_new(object_class_get_name(OBJECT_CLASS(acc)))); } /* Do basic accel initialization without affecting global QEMU state */ /* accel_init_machine() would be changed to reuse this function: */ void accel_open(AccelState *s, Error **errp) { object_property_set_bool(OBJECT(s), true, "open", errp); } target-s390/kvm.c: /* Using a different function name would be interesting, as it would be * the main arch_query_cpu_definitions() code path, not a fallback. */ int get_machine_props_fallback(S390MachineProps *prop) { int r; AccelState *ac = accel_create("kvm"); /*TODO: error handling */ accel_open(ac, &err); r = cpu_model_get(ac, prop); object_unref(OBJECT(ac)); return r; } [1] I only moved the /dev/kvm opening to the open method, but maybe the whole code up to KVM_CREATE_VM and capabitlity checking could be moved. (But I don't know how to handle kvm_type, as it is currently provided by MachineClass. Maybe kvm_type() belongs to CPUClass instead of MachineClass?) > trace_kvm_get_machine_props(rc, prop->cpuid, prop->ibc); > > -- > 1.8.3.1 >
On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote: [...] > #ifndef CONFIG_USER_ONLY > +static CpuDefinitionInfoList *qmp_query_cpu_definition_host(void) > +{ > + CpuDefinitionInfoList *host = NULL; > + CpuDefinitionInfo *info; > + > + info = g_try_new0(CpuDefinitionInfo, 1); > + if (!info) { > + goto out; > + } > + info->name = g_strdup("host"); > + > + host = g_try_new0(CpuDefinitionInfoList, 1); > + if (!host) { > + g_free(info->name); > + g_free(info); > + goto out; > + } > + host->value = info; > +out: > + return host; > +} [...] > CpuDefinitionInfoList *arch_query_cpu_definitions(bool has_machine, > const char *machine, > bool has_accel, > AccelId accel, > Error **errp) > { > - CpuDefinitionInfoList *entry; > - CpuDefinitionInfo *info; > + S390MachineProps mach; > + GSList *classes; > + uint64_t *mask = NULL; > + CpuDefinitionInfoList *list = NULL; > + > + if (has_machine) { > + mask = s390_fac_list_mask_by_machine(machine); > + if (!mask) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "machine", > + "a valid machine type"); > + return NULL; > + } > + } I would like to understand better the meaning of "runnable" when machine is omitted. Is it really possible to tell if a CPU model is runnable if no machine/mask info is provided as input? If machine is omitted and the command returns runnable=true, does that mean the CPU model is runnable using any machine? Does it mean it is runnable using some of the available machines? If so, which ones? Does it mean something else? > > - info = g_malloc0(sizeof(*info)); > - info->name = g_strdup("host"); > + memset(&mach, 0, sizeof(mach)); > + if (has_accel) { > + switch (accel) { > + case ACCEL_ID_KVM: > + kvm_s390_get_machine_props(NULL, &mach); > + break; > + default: > + return qmp_query_cpu_definition_host(); This will return only a single element. I don't think that's correct. If machine or accel is omitted, I believe we should just omit the "runnable" field, but always return the full list of CPU models. > + } > + } > > - entry = g_malloc0(sizeof(*entry)); > - entry->value = info; > + s390_setup_cpu_classes(ACCEL_TEMP, &mach, mask); > + > + classes = object_class_get_list(TYPE_S390_CPU, false); > + classes = g_slist_sort(classes, s390_cpu_class_asc_order_compare); > + g_slist_foreach(classes, qmp_query_cpu_definition_entry, &list); > + g_slist_free(classes); > > - return entry; > + return list; > } > #endif > [...]
On Wed, 6 May 2015 09:37:41 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote: > [...] > > #ifndef CONFIG_USER_ONLY > > +static CpuDefinitionInfoList *qmp_query_cpu_definition_host(void) > > +{ > > + CpuDefinitionInfoList *host = NULL; > > + CpuDefinitionInfo *info; > > + > > + info = g_try_new0(CpuDefinitionInfo, 1); > > + if (!info) { > > + goto out; > > + } > > + info->name = g_strdup("host"); > > + > > + host = g_try_new0(CpuDefinitionInfoList, 1); > > + if (!host) { > > + g_free(info->name); > > + g_free(info); > > + goto out; > > + } > > + host->value = info; > > +out: > > + return host; > > +} > [...] > > CpuDefinitionInfoList *arch_query_cpu_definitions(bool has_machine, > > const char *machine, > > bool has_accel, > > AccelId accel, > > Error **errp) > > { > > - CpuDefinitionInfoList *entry; > > - CpuDefinitionInfo *info; > > + S390MachineProps mach; > > + GSList *classes; > > + uint64_t *mask = NULL; > > + CpuDefinitionInfoList *list = NULL; > > + > > + if (has_machine) { > > + mask = s390_fac_list_mask_by_machine(machine); > > + if (!mask) { > > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "machine", > > + "a valid machine type"); > > + return NULL; > > + } > > + } > > I would like to understand better the meaning of "runnable" when machine > is omitted. Is it really possible to tell if a CPU model is runnable if > no machine/mask info is provided as input? Yes it is. The list of runnable CPU models is derived from the set of S390 CPU facilities available. One subset of facilities depends on the accelerator and the second on the capabilities of QEMU which is represented by the mask. In the case the no machine is specified only those of the accelerator are taken into account for the calculation. It is even possible to omit both, the accelerator and the machine, in that case only the model none is runnable and becomes the default: {"return":[{"order":3345,"name":"2964-ga1","live-migration-safe":true},{"name":"zBC12"}, {"name":"2828"},{"order":3105,"name":"2828-ga1","live-migration-safe":true},{"name":"host"}, {"name":"zEC12"},{"name":"2827"},{"order":3090,"name":"2827-ga2","live-migration-safe":true}, {"order":3089,"name":"2827-ga1","live-migration-safe":true},{"name":"z114"},{"name":"2818"}, {"order":2849,"name":"2818-ga1","live-migration-safe":true},{"name":"z196"},{"name":"2817"}, {"order":2834,"name":"2817-ga2","live-migration-safe":true},{"order":2833,"name":"2817-ga1", "live-migration-safe":true},{"name":"z10-bc"},{"name":"2098"},{"order":2594,"name":"2098-ga2", "live-migration-safe":true},{"order":2593,"name":"2098-ga1","live-migration-safe":true}, {"name":"z10-ec"},{"name":"z10"},{"name":"2097"},{"order":2579,"name":"2097-ga3", "live-migration-safe":true},{"order":2578,"name":"2097-ga2","live-migration-safe":true}, {"order":2577,"name":"2097-ga1","live-migration-safe":true},{"name":"z9-bc"},{"name":"2096"}, {"order":2338,"name":"2096-ga2","live-migration-safe":true},{"order":2337,"name":"2096-ga1", "live-migration-safe":true},{"name":"z9-ec"},{"name":"z9"},{"name":"2094"},{"order":2323, "name":"2094-ga3","live-migration-safe":true},{"order":2322,"name":"2094-ga2", "live-migration-safe":true},{"name":"z9-109"},{"order":2321,"name":"2094-ga1", "live-migration-safe":true},{"name":"z890"},{"name":"2086"},{"order":2083,"name":"2086-ga3", "live-migration-safe":true},{"order":2082,"name":"2086-ga2","live-migration-safe":true}, {"order":2081,"name":"2086-ga1","live-migration-safe":true},{"name":"z990"},{"name":"2084"}, {"order":2069,"name":"2084-ga5","live-migration-safe":true},{"order":2068,"name":"2084-ga4", "live-migration-safe":true},{"order":2067,"name":"2084-ga3","live-migration-safe":true}, {"order":2066,"name":"2084-ga2","live-migration-safe":true},{"order":2065,"name":"2084-ga1", "live-migration-safe":true},{"name":"z800"},{"name":"2066"},{"order":1825,"name":"2066-ga1", "live-migration-safe":true},{"name":"z900"},{"name":"2064"},{"order":1811,"name":"2064-ga3", "live-migration-safe":true},{"order":1810,"name":"2064-ga2","live-migration-safe":true}, {"order":1809,"name":"2064-ga1","live-migration-safe":true},{"name":"none","runnable":true, "is-default":true}],"id":"libvirt-15"} > > If machine is omitted and the command returns runnable=true, does that > mean the CPU model is runnable using any machine? Does it mean it is > runnable using some of the available machines? If so, which ones? Does > it mean something else? As it is a lower limit all machines shall be able to run it. Although a somewhat clever management interface should iterate through all accel/machine combinations. > > > > > - info = g_malloc0(sizeof(*info)); > > - info->name = g_strdup("host"); > > + memset(&mach, 0, sizeof(mach)); > > + if (has_accel) { > > + switch (accel) { > > + case ACCEL_ID_KVM: > > + kvm_s390_get_machine_props(NULL, &mach); > > + break; > > + default: > > + return qmp_query_cpu_definition_host(); > > This will return only a single element. I don't think that's correct. If > machine or accel is omitted, I believe we should just omit the > "runnable" field, but always return the full list of CPU models. That is the !KVM case where I keep the behavior currently unchanged to the existing implementation. But right for the TCG case there has to be a comparable get_machine_props() call returning whatever TCG is implementing. But that is not part of this patch series. > > > + } > > + } > > > > - entry = g_malloc0(sizeof(*entry)); > > - entry->value = info; > > + s390_setup_cpu_classes(ACCEL_TEMP, &mach, mask); > > + > > + classes = object_class_get_list(TYPE_S390_CPU, false); > > + classes = g_slist_sort(classes, s390_cpu_class_asc_order_compare); > > + g_slist_foreach(classes, qmp_query_cpu_definition_entry, &list); > > + g_slist_free(classes); > > > > - return entry; > > + return list; > > } > > #endif > > > [...] >
On Tue, 5 May 2015 15:40:34 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote: > [...] > > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c > > index 4d75ff0..94fede5 100644 > > --- a/target-s390x/kvm.c > > +++ b/target-s390x/kvm.c > > @@ -276,12 +276,59 @@ static int cpu_model_set(KVMState *s, uint64_t attr, void *addr) > > return rc; > > } > > > > -static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop) > > This seems to duplicate lots of the existing KVM code. (See additional > comment below about possible ways to avoid it). some :-) > > > +static int get_machine_props_fallback(S390MachineProps *prop) > > +{ > > + struct kvm_device_attr dev_attr; > > + int rc, kvmfd = -1, vmfd = -1; > > + > > + rc = qemu_open("/dev/kvm", O_RDWR); > > + if (rc < 0) { > > + goto out_err; > > + } > > + kvmfd = rc; > > + > > + rc = ioctl(kvmfd, KVM_CREATE_VM, 0); > > + if (rc < 0) { > > + goto out_err; > > + } > > + vmfd = rc; > > + > > + rc = ioctl(vmfd, KVM_CHECK_EXTENSION, KVM_CAP_VM_ATTRIBUTES); > > + if (rc < 0) { > > + rc = -ENOSYS; > > + goto out_err; > > + } > > + > > + dev_attr.group = KVM_S390_VM_CPU_MODEL; > > + dev_attr.attr = KVM_S390_VM_CPU_MACHINE; > > + rc = ioctl(vmfd, KVM_HAS_DEVICE_ATTR, &dev_attr); > > + if (rc < 0) { > > + rc = -EFAULT; > > + goto out_err; > > + } > > + > > + dev_attr.addr = (uint64_t) prop; > > + rc = ioctl(vmfd, KVM_GET_DEVICE_ATTR, &dev_attr); > > + > > +out_err: > > + if (vmfd >= 0) { > > + close(vmfd); > > + } > > + if (kvmfd >= 0) { > > + close(kvmfd); > > + } > > + > > + return rc; > > +} > > + > > +int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop) > > { > > int rc = -EFAULT; > > > > if (s) { > > rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, prop); > > + } else { > > + rc = get_machine_props_fallback(prop); > > } > > The comments below are just suggestions, not something which should > block the patch, in my opinion: > > First, if s is always NULL inside arch_query_cpu_definitions(), and is > always non-NULL inside kvm_setup_cpu_classes(), why don't you just call > keep the original kvm_s390_get_machine_props() function, and call and > get_machine_props_fallback() inside arch_query_cpu_definitions()? My reason for pulling both paths through the same internal interface call is to have just single call for the same purpose. > > The only thing common to both cases is the tracing point, but if we are > running two completely different code paths I assume it would be a good > thing to have a different tracing point for > get_machine_props_fallback(). > > > Second, you shouldn't even need to duplicate code in > get_machine_props_fallback() if you are able to create an accel object > and do just basic initialization so that cpu_model_get() works. > Allowing accel objects to be created on the fly was one of the main > purposes of the accel QOM work. > > For example, if we do something like this: > https://github.com/ehabkost/qemu-hacks/commit/36a250e34c5fd0d43a25271f5bc9b04681fdd56a [1] > https://github.com/ehabkost/qemu-hacks/commits/work/accel-open-func I had a look at your qemu-hacks before writing the _fallback() routine but did not wanted to base on some not yet published code. Once your part goes upstream my intend is to provide a cleanup patch... And I was missing the KVM_CREATE_VM actually. > > Then the code could look like this: > > accel.c: > > /* configure_accelerator() would be changed to reuse this function: */ > AccelState *accel_create(const char *accel_name) > { > AccelClass *acc = accel_find(accel_name); > /*TODO: error handling, checking acc->available() */ > return ACCEL(object_new(object_class_get_name(OBJECT_CLASS(acc)))); > } > > /* Do basic accel initialization without affecting global QEMU state */ > /* accel_init_machine() would be changed to reuse this function: */ > void accel_open(AccelState *s, Error **errp) > { > object_property_set_bool(OBJECT(s), true, "open", errp); > } > > target-s390/kvm.c: > > /* Using a different function name would be interesting, as it would be > * the main arch_query_cpu_definitions() code path, not a fallback. > */ > int get_machine_props_fallback(S390MachineProps *prop) > { > int r; > AccelState *ac = accel_create("kvm"); > /*TODO: error handling */ > accel_open(ac, &err); > r = cpu_model_get(ac, prop); > object_unref(OBJECT(ac)); > return r; > } > > > [1] I only moved the /dev/kvm opening to the open method, but maybe the > whole code up to KVM_CREATE_VM and capabitlity checking could be > moved. Yes as mentioned above. > (But I don't know how to handle kvm_type, as it is currently > provided by MachineClass. Maybe kvm_type() belongs to CPUClass > instead of MachineClass?) > > > trace_kvm_get_machine_props(rc, prop->cpuid, prop->ibc); > > > > -- > > 1.8.3.1 > > >
On Wed, May 06, 2015 at 05:31:06PM +0200, Michael Mueller wrote: > On Tue, 5 May 2015 15:40:34 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > Second, you shouldn't even need to duplicate code in > > get_machine_props_fallback() if you are able to create an accel object > > and do just basic initialization so that cpu_model_get() works. > > Allowing accel objects to be created on the fly was one of the main > > purposes of the accel QOM work. > > > > For example, if we do something like this: > > https://github.com/ehabkost/qemu-hacks/commit/36a250e34c5fd0d43a25271f5bc9b04681fdd56a [1] > > https://github.com/ehabkost/qemu-hacks/commits/work/accel-open-func > > I had a look at your qemu-hacks before writing the _fallback() routine > but did not wanted to base on some not yet published code. Once your part goes > upstream my intend is to provide a cleanup patch... And I was missing the > KVM_CREATE_VM actually. No problem to me if you prefer to keep it this way and change it to use AccelState as a follow-up, after appropriate methods are provided by TYPE_KVM_ACCEL. Your patch was helpful to show that just extracting the open("/dev/kvm") part isn't enough and more work is needed.
On Wed, 6 May 2015 13:00:32 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, May 06, 2015 at 05:31:06PM +0200, Michael Mueller wrote: > > On Tue, 5 May 2015 15:40:34 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > [...] > > > Second, you shouldn't even need to duplicate code in > > > get_machine_props_fallback() if you are able to create an accel object > > > and do just basic initialization so that cpu_model_get() works. > > > Allowing accel objects to be created on the fly was one of the main > > > purposes of the accel QOM work. > > > > > > For example, if we do something like this: > > > https://github.com/ehabkost/qemu-hacks/commit/36a250e34c5fd0d43a25271f5bc9b04681fdd56a [1] > > > https://github.com/ehabkost/qemu-hacks/commits/work/accel-open-func > > > > I had a look at your qemu-hacks before writing the _fallback() routine > > but did not wanted to base on some not yet published code. Once your part goes > > upstream my intend is to provide a cleanup patch... And I was missing the > > KVM_CREATE_VM actually. > > No problem to me if you prefer to keep it this way and change it to use > AccelState as a follow-up, after appropriate methods are provided by > TYPE_KVM_ACCEL. Your patch was helpful to show that just extracting the > open("/dev/kvm") part isn't enough and more work is needed. promised :-) >
On Wed, May 06, 2015 at 04:48:57PM +0200, Michael Mueller wrote: > On Wed, 6 May 2015 09:37:41 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > > > > > > > - info = g_malloc0(sizeof(*info)); > > > - info->name = g_strdup("host"); > > > + memset(&mach, 0, sizeof(mach)); > > > + if (has_accel) { > > > + switch (accel) { > > > + case ACCEL_ID_KVM: > > > + kvm_s390_get_machine_props(NULL, &mach); > > > + break; > > > + default: > > > + return qmp_query_cpu_definition_host(); > > > > This will return only a single element. I don't think that's correct. If > > machine or accel is omitted, I believe we should just omit the > > "runnable" field, but always return the full list of CPU models. > > That is the !KVM case where I keep the behavior currently unchanged to > the existing implementation. But right for the TCG case there has to be > a comparable get_machine_props() call returning whatever TCG is implementing. > But that is not part of this patch series. I believe the right thing to do is to return the full list of CPU models, but report them as not runnable if accel is TCG, and simply omit the runnable field if no accel argument is provided. This way, existing management code that already runs query-cpu-models (e.g. libvirt, that does that with "-machine none") will keep working.
diff --git a/target-s390x/cpu-models.c b/target-s390x/cpu-models.c index dfdfb7b..7efb3e7 100644 --- a/target-s390x/cpu-models.c +++ b/target-s390x/cpu-models.c @@ -208,6 +208,21 @@ int set_s390_cpu_alias(const char *name, const char *model) return 0; } +/* compare order of two cpu classes for descending sort */ +gint s390_cpu_class_desc_order_compare(gconstpointer a, gconstpointer b) +{ + S390CPUClass *cc_a = S390_CPU_CLASS((ObjectClass *) a); + S390CPUClass *cc_b = S390_CPU_CLASS((ObjectClass *) b); + + if (cc_a->mach.order < cc_b->mach.order) { + return 1; + } + if (cc_a->mach.order > cc_b->mach.order) { + return -1; + } + return 0; +} + /* return machine class for specific machine type */ static void s390_machine_class_test_cpu_class(gpointer data, gpointer user_data) { diff --git a/target-s390x/cpu-models.h b/target-s390x/cpu-models.h index c24550b..76673f4 100644 --- a/target-s390x/cpu-models.h +++ b/target-s390x/cpu-models.h @@ -87,6 +87,7 @@ void s390_setup_cpu_classes(S390AccelMode mode, S390MachineProps *prop, uint64_t *fac_list_mask); void s390_setup_cpu_aliases(void); gint s390_cpu_class_asc_order_compare(gconstpointer a, gconstpointer b); +gint s390_cpu_class_desc_order_compare(gconstpointer a, gconstpointer b); void s390_cpu_list_entry(gpointer data, gpointer user_data); bool s390_cpu_classes_initialized(void); uint64_t *s390_fac_list_mask_by_machine(const char *name); @@ -94,10 +95,16 @@ uint64_t *s390_current_fac_list_mask(void); void s390_cpu_model_init(S390CPUClass *cc); #ifdef CONFIG_KVM +int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop); int kvm_s390_get_processor_props(S390ProcessorProps *prop); int kvm_s390_set_processor_props(S390ProcessorProps *prop); bool kvm_s390_cpu_classes_initialized(void); #else +static inline int kvm_s390_get_machine_props(KVMState *s, + S390MachineProps *prop) +{ + return -ENOSYS; +} static inline int kvm_s390_get_processor_props(S390ProcessorProps *prop) { return -ENOSYS; diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c index 771acbd..a006b93 100644 --- a/target-s390x/cpu.c +++ b/target-s390x/cpu.c @@ -30,7 +30,10 @@ #include "hw/hw.h" #include "trace.h" #include "cpu-models.h" +#include "sysemu/accel.h" +#include "qapi/qmp/qerror.h" #ifndef CONFIG_USER_ONLY +#include "hw/boards.h" #include "sysemu/arch_init.h" #endif @@ -61,22 +64,131 @@ void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf) } #ifndef CONFIG_USER_ONLY +static CpuDefinitionInfoList *qmp_query_cpu_definition_host(void) +{ + CpuDefinitionInfoList *host = NULL; + CpuDefinitionInfo *info; + + info = g_try_new0(CpuDefinitionInfo, 1); + if (!info) { + goto out; + } + info->name = g_strdup("host"); + + host = g_try_new0(CpuDefinitionInfoList, 1); + if (!host) { + g_free(info->name); + g_free(info); + goto out; + } + host->value = info; +out: + return host; +} + +static void qmp_query_cpu_definition_entry(gpointer data, gpointer user_data) +{ + ObjectClass *oc = (ObjectClass *) data; + S390CPUClass *cc = S390_CPU_CLASS(oc); + CpuDefinitionInfoList *entry, **list = user_data; + CpuDefinitionInfo *info; + GSList *item; + S390CPUAlias *alias; + ObjectClass *alias_oc; + + info = g_try_new0(CpuDefinitionInfo, 1); + if (!info) { + goto out; + } + info->name = strdup_cpu_name(cc); + info->runnable = cc->is_active[ACCEL_TEMP]; + info->has_runnable = info->runnable; + info->is_default = cc->is_host[ACCEL_TEMP]; + info->has_is_default = info->is_default; + info->live_migration_safe = cc->is_migration_safe; + info->has_live_migration_safe = info->live_migration_safe; + info->order = cc->mach.order; + info->has_order = info->order != 0; + + entry = g_try_new0(CpuDefinitionInfoList, 1); + if (!entry) { + goto out; + } + entry->value = info; + entry->next = *list; + *list = entry; + + for (item = s390_cpu_aliases; item != NULL; item = item->next) { + alias = (S390CPUAlias *) item->data; + alias_oc = s390_cpu_class_by_name(alias->model); + if (alias_oc != oc) { + continue; + } + + info = g_try_new0(CpuDefinitionInfo, 1); + if (!info) { + goto out; + } + info->name = g_strdup(alias->name); + info->runnable = cc->is_active[ACCEL_TEMP]; + info->has_runnable = info->runnable; + + entry = g_try_new0(CpuDefinitionInfoList, 1); + if (!entry) { + goto out; + } + entry->value = info; + entry->next = *list; + *list = entry; + } + + return; +out: + if (info) { + g_free(info->name); + g_free(info); + } +} + CpuDefinitionInfoList *arch_query_cpu_definitions(bool has_machine, const char *machine, bool has_accel, AccelId accel, Error **errp) { - CpuDefinitionInfoList *entry; - CpuDefinitionInfo *info; + S390MachineProps mach; + GSList *classes; + uint64_t *mask = NULL; + CpuDefinitionInfoList *list = NULL; + + if (has_machine) { + mask = s390_fac_list_mask_by_machine(machine); + if (!mask) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "machine", + "a valid machine type"); + return NULL; + } + } - info = g_malloc0(sizeof(*info)); - info->name = g_strdup("host"); + memset(&mach, 0, sizeof(mach)); + if (has_accel) { + switch (accel) { + case ACCEL_ID_KVM: + kvm_s390_get_machine_props(NULL, &mach); + break; + default: + return qmp_query_cpu_definition_host(); + } + } - entry = g_malloc0(sizeof(*entry)); - entry->value = info; + s390_setup_cpu_classes(ACCEL_TEMP, &mach, mask); + + classes = object_class_get_list(TYPE_S390_CPU, false); + classes = g_slist_sort(classes, s390_cpu_class_asc_order_compare); + g_slist_foreach(classes, qmp_query_cpu_definition_entry, &list); + g_slist_free(classes); - return entry; + return list; } #endif diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c index 4d75ff0..94fede5 100644 --- a/target-s390x/kvm.c +++ b/target-s390x/kvm.c @@ -276,12 +276,59 @@ static int cpu_model_set(KVMState *s, uint64_t attr, void *addr) return rc; } -static int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop) +static int get_machine_props_fallback(S390MachineProps *prop) +{ + struct kvm_device_attr dev_attr; + int rc, kvmfd = -1, vmfd = -1; + + rc = qemu_open("/dev/kvm", O_RDWR); + if (rc < 0) { + goto out_err; + } + kvmfd = rc; + + rc = ioctl(kvmfd, KVM_CREATE_VM, 0); + if (rc < 0) { + goto out_err; + } + vmfd = rc; + + rc = ioctl(vmfd, KVM_CHECK_EXTENSION, KVM_CAP_VM_ATTRIBUTES); + if (rc < 0) { + rc = -ENOSYS; + goto out_err; + } + + dev_attr.group = KVM_S390_VM_CPU_MODEL; + dev_attr.attr = KVM_S390_VM_CPU_MACHINE; + rc = ioctl(vmfd, KVM_HAS_DEVICE_ATTR, &dev_attr); + if (rc < 0) { + rc = -EFAULT; + goto out_err; + } + + dev_attr.addr = (uint64_t) prop; + rc = ioctl(vmfd, KVM_GET_DEVICE_ATTR, &dev_attr); + +out_err: + if (vmfd >= 0) { + close(vmfd); + } + if (kvmfd >= 0) { + close(kvmfd); + } + + return rc; +} + +int kvm_s390_get_machine_props(KVMState *s, S390MachineProps *prop) { int rc = -EFAULT; if (s) { rc = cpu_model_get(s, KVM_S390_VM_CPU_MACHINE, prop); + } else { + rc = get_machine_props_fallback(prop); } trace_kvm_get_machine_props(rc, prop->cpuid, prop->ibc);
This patch implements the QMP command 'query-cpu-definitions' in the S390 context. The command returns a list of cpu definitions in the current host context. A runnable and migratable cpu model has the related attributes set to true. The order attribute is used to bring the listed cpu definitions in a release order. request: {"execute": "query-cpu-definitions", "arguments": { "accel": "kvm", "machine": "s390-ccw-virtio" } } answer: {"return": [ { "order": 3345, "name": "2964-ga1", "live-migration-safe": true }, { "name": "zBC12", "runnable": true }, { "name": "2828", "runnable": true }, ... { "name": "host", "runnable": true }, ... { "order": 3090, "name": "2827-ga2", "runnable": true, "live-migration-safe": true, "is-default": true }, ... { "name": "none", "runnable": true } ] } The request arguments are optional and if omitted lead to different answers. Eventually only the CPU model none gets returned as runnable and as default ... { "name": "none", "runnable": true, "is-default": true } The returned values for attributes like runnable depend on the machine type QEMU is running on. The function kvm_s390_get_machine_props() is used to determine that. If QEMU was started for accelerator KVM, a KVMstate is established and machine properties are retrieved by cpu_model_get(). In case no KVMstate was established, e.g. during QEMU startup in daemonized mode with the default accelerator TCG, a fallback routine named get_machine_props_fallback() is used to retrieve the KVM machine properties. It first creates a temporary VM, performs the required ioctls and finally destroys the VM again. Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> --- target-s390x/cpu-models.c | 15 ++++++ target-s390x/cpu-models.h | 7 +++ target-s390x/cpu.c | 126 +++++++++++++++++++++++++++++++++++++++++++--- target-s390x/kvm.c | 49 +++++++++++++++++- 4 files changed, 189 insertions(+), 8 deletions(-)