Re: [Qemu-devel] [PATCH 6/7] target-i386: add implementation of query-cpudefs

[Anthony Liguori <aliguori@us.ibm.com>]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Aug 10, 2012 at 09:43:21AM -0500, Anthony Liguori wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Fri, Jul 27, 2012 at 08:37:18AM -0500, Anthony Liguori wrote:
>> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> >> ---
>> >>  target-i386/cpu.c |   22 ++++++++++++++++++++++
>> >>  1 files changed, 22 insertions(+), 0 deletions(-)
>> >> 
>> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> >> index 6b9659f..b398439 100644
>> >> --- a/target-i386/cpu.c
>> >> +++ b/target-i386/cpu.c
>> >> @@ -28,6 +28,7 @@
>> >>  #include "qemu-config.h"
>> >>  
>> >>  #include "qapi/qapi-visit-core.h"
>> >> +#include "qmp-commands.h"
>> >>  
>> >>  #include "hyperv.h"
>> >>  
>> >> @@ -1123,6 +1124,27 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
>> >>      }
>> >>  }
>> >>  
>> >> +CpuDefInfoList *qmp_query_cpudefs(Error **errp)
>> >> +{
>> >> +    CpuDefInfoList *cpu_list = NULL;
>> >> +    x86_def_t *def;
>> >> +
>> >> +    for (def = x86_defs; def; def = def->next) {
>> >> +        CpuDefInfoList *entry;
>> >> +        CpuDefInfo *info;
>> >> +
>> >> +        info = g_malloc0(sizeof(*info));
>> >> +        info->name = g_strdup(def->name);
>> >> +
>> >> +        entry = g_malloc0(sizeof(*entry));
>> >> +        entry->value = info;
>> >> +        entry->next = cpu_list;
>> >> +        cpu_list = entry;
>> >> +    }
>> >> +
>> >> +    return cpu_list;
>> >> +}
>> >
>> > How would the interface look like once we:
>> > - let libvirt know which features are available on each CPU model
>> >   (libvirt needs that information[1]); and
>> 
>> I'm not sure I understand why libvirt needs this information.  Can you elaborate?
>
> I see two reasons:
>
> - The libvirt API has functions to tell the user which features are
>   going to be enabled for each CPU model, so it needs to know which
>   features are enabled or not, for each machine-type + cpu-model
>   combination, so this information can be reported proeprly.

Ok, step number one is that CPU 'features' need to be defined more
formally.  By formally, I mean via qapi-schema.json.

Then we can extend this command to return the set of features supported
by each CPU type.

The first step will need to sort out how this maps across architectures.

>   - Also, if libvirt can enable/disable specific CPU features in the
>     command-line, it just makes sens to know which ones are already
>     enabled in each built-in CPU model.
>
> - Probing for migration: libvirt needs to know if a given CPU model on a
>   host can be migrated to another host. To know that, two pieces of
>   information are needed:
>   A) Which CPU features are visible to the guest for a specific
>      configuration;
>   B) Which of those features are really supported by the host
>      hardware+kernel+QEMU, on the destination host, so it can
>      know if migration is really possible.

Note that what QEMU thinks it exposes is not necessarily what gets
exposed.  KVM may mask additional features.  How is this handled today?

>> > - add machine-type-specific cpudef compatibility changes?
>> 
>> I think we've discussed this in IRC.  I don't think we need to worry
>> about this.
>
> I remember discussing a lot about the mechanism we will use to add the
> compatibility changes, but I don t know how the query API will look
> like, after we implement this mechanism.

0) User-defined CPU definitions go away
   - We already made a big step in this direction

1) CPU becomes a DeviceState

2) Features are expressed as properties

3) Same global mechanism used for everything else is used for CPUs

Regards,

Anthony Liguori

>> > Would the command report different results depending on -machine?
>> 
>> No.
>
> The problem is:
>
> 1) We need to introduce fixes on a CPU model that changes the set of
>    guest-visible features (add or remove a feature)[1];
> 2) The fix has to keep compatibility, so older machine-types will
>    keep exposing the old set of gues-visible features;
>    - That means different machine-types will have different CPU
>      features being exposed.
> 3) libvirt needs to control/know which guest-visible CPU features are
>    available to the guest (see above);
> 4) Because of (2), the querying system used by libvirt need to depend on
>    the CPU model and machine-type.
>
>
> [1] Example:
>     The SandyBridge model today has the "tsc-deadline" bit set, but
>     QEMU-1.1 did not expose the tsc-deadline feature properly because of
>     incorrect expectations about the GET_SUPPORTED_CPUID ioctl. This was
>     fixed on qemu-1.2.
>     
>     That means "qemu-1.1 -machine pc-1.1 -cpu SandyBridge" does _not_
>     expose tsc-deadline to the guest, and we need to make "qemu-1.2
>     -machine pc-1.1 -cpu SandyBridge" _not_ expose it, too (otherwise
>     migration from qemu-1.1 to qemu-1.2 will be broken).
>
>> 
>> >
>> > Would the command return the latest cpudef without any machine-type
>> > hacks, and libvirt would have to query for the cpudef compatibility data
>> > for each machine-type and combine both pieces of information itself?
>> 
>> I'm not sure what you mean by compatibility data.
>
> I mean any guest-visible compatibility bit that we will need to
> introduce on older machine-types, when making changes on CPU models (see
> the SandyBridge + tsc-deadline example above).
>
> I see two options:
> - Libvirt queries for a [f(machine_type, cpu_model) -> cpu_features]
>   function, that will take into account the machine-type-specific
>   compatibility bits.
> - Libvirt queries for a [f(cpu_model) -> cpu_features] function and a
>   [f(machine_type) -> compatibility_changes] function, and combine both.
>   - I don't like this approach, I am just including it as a possible
>     alternative.
>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>> >
>> > [1] Note that it doesn't have to be low-level leaf-by-leaf
>> >     register-by-register CPUID bits (I prefer a more high-level
>> >     interface, myself), but it has to at least say "feature FOO is
>> >     enabled/disabled" for a set of features libvirt cares about.
>> >
>> > -- 
>> > Eduardo
>> 
>
> -- 
> Eduardo