Re: [PATCH RESEND v1 1/2] i386: Add Intel Processor Trace feature support

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Jan 16, 2018 at 06:10:17AM +0000, Kang, Luwei wrote:
> > > On Mon, Jan 15, 2018 at 12:04:55 -0200, Eduardo Habkost wrote:
> > > > CCing libvirt developers.
> > > ...
> > > > This case is slightly more problematic, however: the new feature is
> > > > actually migratable (under very controlled circumstances) because of
> > > > patch 2/2, but it is not migration-safe[1].  This means libvirt
> > > > shouldn't include it in "host-model" expansion (which uses the
> > > > query-cpu-model-expansion QMP command) until we make the feature
> > > > migration-safe.
> > > >
> > > > For QEMU, this means the feature shouldn't be returned by
> > > > "query-cpu-model-expansion type=static model=max" (but it can be
> > > > returned by "query-cpu-model-expansion type=full model=max").
> > > >
> > > > Jiri, it looks like libvirt uses type=full on
> > > > query-cpu-model-expansion on x86.  It needs to use type=static[2],
> > > > or it will have no way to find out if a feature is migration-safe or
> > > > not.
> > > ...
> > > > [2] It looks like libvirt uses type=full because it wants to get
> > > >     all QOM property aliases returned.  In this case, one
> > > >     solution for libvirt is to use:
> > > >
> > > >     static_expansion = query_cpu_model_expansion(type=static, model)
> > > >     all_props = query_cpu_model_expansion(type=full,
> > > > static_expansion)
> > >
> > > This is exactly what libvirt is doing (with model = "host") ever since
> > > query-cpu-model-expansion support was implemented for x86.
> > 
> > Oh, now I see that the x86 code uses
> > QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL and not just QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL.  Nice!
> > 
> 
> So, I need to add Intel PT feature in "X86CPUDefinition
> builtin_x86_defs[]" so that we can get this CPUID in specific
> CPU model not only "-cpu host". Is that right?

The problem is that you won't be able to add intel-pt to any CPU
model unless the feature is made migration-safe (by not calling
kvm_arch_get_supported_cpuid() from cpu_x86_cpuid()).

What's missing here is to either: (a) make cpu_x86_cpuid() return
host-independent data (it can be constant, or it can be
configurable on the command-line); or (b) add a mechanism to skip
intel-pt from "query-cpu-model-expansion type=static".  Probably
(a) is easier to implement, and it also makes the feature more
useful (by making it migration-safe).

> 
> Intel PT is first supported in Intel Core M and 5th generation
> Intel Core processors that are based on the Intel
> micro-architecture code name Broadwell but Intel PT use EPT is
> first supported in Ice Lake. Intel PT virtualization depend on
> PT use EPT.  I will add Intel PT to "Broadwell" CPU model and
> later to make sure a "Broadwell" guest can use Intel PT if the
> host is Ice Lake.

The "if the host is Ice Lake" part is problematic.  On
migration-safe CPU models (all of them except "max" and "host"),
the data seen on CPUID can't depend on the host at all.  It
should depend only on the machine-type + command-line options.

-- 
Eduardo