[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 7/7] cpu: Add `machine` parameter to query-cpu-definitions
|
From: |
Markus Armbruster |
|
Subject: |
Re: [PATCH 7/7] cpu: Add `machine` parameter to query-cpu-definitions |
|
Date: |
Fri, 25 Oct 2019 08:36:59 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
Eduardo Habkost <address@hidden> writes:
> The new parameter can be used by management software to query for
> CPU model alias information for multiple machines without
> restarting QEMU.
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> qapi/machine-target.json | 14 +++++++-
> target/arm/helper.c | 4 ++-
> target/i386/cpu.c | 16 +++++++--
> target/mips/helper.c | 4 ++-
> target/ppc/translate_init.inc.c | 4 ++-
> target/s390x/cpu_models.c | 4 ++-
> tests/acceptance/x86_cpu_model_versions.py | 42 ++++++++++++++++++++++
> 7 files changed, 81 insertions(+), 7 deletions(-)
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 55310a6aa2..7bff3811fe 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -281,6 +281,10 @@
This is CpuDefinitionInfo.
> #
> # @alias-of: Name of CPU model this model is an alias for. The target of the
> # CPU model alias may change depending on the machine type.
> +# If the @machine argument was provided to query-cpu-definitions,
> +# alias information that machine type will be returned.
"for that machine type"
> +# If @machine is not provided, alias information for
> +# the current machine will be returned.
> # Management software is supposed to translate CPU model aliases
> # in the VM configuration, because aliases may stop being
> # migration-safe in the future (since 4.1)
> @@ -317,9 +321,17 @@
##
# @query-cpu-definitions:
> #
> # Return a list of supported virtual CPU definitions
> #
> +# @machine: Name of machine type. The command returns some data
> +# that machine-specific. This overrides the machine type
"that is machine-specific"
> +# used to look up that information. This can be used,
> +# for example, to query machine-specific CPU model aliases
> +# without restarting QEMU (since 4.2)
> +#
> # Returns: a list of CpuDefInfo
> #
> # Since: 1.2.0
> ##
> -{ 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> +{ 'command': 'query-cpu-definitions',
> + 'data': { '*machine': 'str' },
> + 'returns': ['CpuDefinitionInfo'],
> 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386)
> || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
I'm afraid the doc comment is less than clear. Before I can suggest
improvements, I have questions.
Looks like @alias-of is the only part of the return value that changes
when I re-run query-cpu-definitions with another @machine argument.
Correct?
How is this going to be used? Will management software run
query-cpu-definitions for each machine type returned by query-machines?
Or just for a few of them?
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0d9a2d2ab7..96f9fe7fff 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6965,7 +6965,9 @@ static void arm_cpu_add_definition(gpointer data,
> gpointer user_data)
> *cpu_list = entry;
> }
>
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> + const char *machine,
> + Error **errp)
> {
> CpuDefinitionInfoList *cpu_list = NULL;
> GSList *list;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 67d1eca4ed..ae633793ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4078,11 +4078,23 @@ static void x86_cpu_definition_entry(gpointer data,
> gpointer user_data)
> args->cpu_list = entry;
> }
>
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> + const char *machine,
> + Error **errp)
> {
> X86CPUDefinitionArgs args = { .cpu_list = NULL };
> GSList *list;
> - MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> + MachineClass *mc;
> +
> + if (!has_machine) {
> + mc = MACHINE_GET_CLASS(qdev_get_machine());
> + } else {
> + mc = machine_find_class(machine);
> + if (!mc) {
> + error_setg(errp, "Machine type '%s' not found", machine);
> + return NULL;
> + }
> + }
>
> args.default_version = default_cpu_version_for_machine(mc);
> list = get_sorted_cpu_model_list();
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index a2b6459b05..a73c767462 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -1481,7 +1481,9 @@ static void mips_cpu_add_definition(gpointer data,
> gpointer user_data)
> *cpu_list = entry;
> }
>
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> + const char *machine,
> + Error **errp)
> {
> CpuDefinitionInfoList *cpu_list = NULL;
> GSList *list;
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ba726dec4d..4493309c4c 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10350,7 +10350,9 @@ static void ppc_cpu_defs_entry(gpointer data,
> gpointer user_data)
> *first = entry;
> }
>
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> + const char *machine,
> + Error **errp)
> {
> CpuDefinitionInfoList *cpu_list = NULL;
> GSList *list;
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 7e92fb2e15..e40f1f6bec 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -456,7 +456,9 @@ static void create_cpu_model_list(ObjectClass *klass,
> void *opaque)
> *cpu_list = entry;
> }
>
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> + const char *machine,
> + Error **errp)
> {
> struct CpuDefinitionInfoListData list_data = {
> .list = NULL,
Should the commit message explain why all implementations but one ignore
@machine?
> diff --git a/tests/acceptance/x86_cpu_model_versions.py
> b/tests/acceptance/x86_cpu_model_versions.py
> index 5fc9ca4bc6..79c5250243 100644
> --- a/tests/acceptance/x86_cpu_model_versions.py
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -234,6 +234,48 @@ class X86CPUModelAliases(avocado_qemu.Test):
>
> self.validate_aliases(cpus)
>
> + def test_machine_arg_none(self):
> + """Check if unversioned CPU model is an alias pointing to right
> version"""
> + vm1 = self.get_vm()
> + vm1.add_args('-S')
> + vm1.set_machine('pc-i440fx-4.0')
> + vm1.launch()
> + cpus1 = dict((m['name'], m.get('alias-of')) for m in
> vm1.command('query-cpu-definitions', machine='none'))
> + vm1.shutdown()
> +
> + vm2 = self.get_vm()
> + vm2.add_args('-S')
> + vm2.set_machine('none')
> + vm2.launch()
> + cpus2 = dict((m['name'], m.get('alias-of')) for m in
> vm2.command('query-cpu-definitions'))
> + vm1.shutdown()
> +
> + self.assertEquals(cpus1, cpus2)
> +
> + def test_machine_arg_4_1(self):
> + """Check if unversioned CPU model is an alias pointing to right
> version"""
> + vm1 = self.get_vm()
> + vm1.add_args('-S')
> + vm1.set_machine('pc-i440fx-4.0')
> + vm1.launch()
> + cpus1 = dict((m['name'], m.get('alias-of')) for m in
> vm1.command('query-cpu-definitions', machine='pc-i440fx-4.1'))
> + vm1.shutdown()
> +
> + vm2 = self.get_vm()
> + vm2.add_args('-S')
> + vm2.set_machine('pc-i440fx-4.1')
> + vm2.launch()
> + cpus2 = dict((m['name'], m.get('alias-of')) for m in
> vm2.command('query-cpu-definitions'))
> + vm1.shutdown()
> +
> + self.assertEquals(cpus1, cpus2)
> +
> + def test_invalid_machine(self):
> + self.vm.add_args('-S')
> + self.vm.launch()
> + r = self.vm.qmp('query-cpu-definitions',
> machine='invalid-machine-123')
> + self.assertIn('error', r)
> +
>
> class CascadelakeArchCapabilities(avocado_qemu.Test):
> """
Tests look good to me.
I admit to being confused on when to use the tests/acceptance/ framework
and when to use qtest.