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

[Eduardo Habkost]

On Thu, Jan 18, 2018 at 05:33:53AM +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()).
> > >
> > > Hi Eduardo,
> > >     Have some question need you help clear. What is
> > >     "migration-safe" feature mean? I find all the feature which
> > >     included in CPU model (builtin_x86_defs[]) will make
> > >     "xcc->migration_safe = true;" in
> > >     x86_cpu_cpudef_class_init(). If create a Skylake guest on
> > >     Skylake HW and live migrate this guest to another machine
> > >     with old HW(e.g Haswell). Can we think the new feature or
> > >     cpu model(Skylake guest) which only supported in Skylake HW
> > >     is safe?
> > 
> > I mean matching the requirements so we can say the feature is 
> > migration-safe, that means exposing the same CPUID data to the
> > guest on any host, if the machine-type + command-line is the same.  The 
> > data on CPUID[7] is OK (it changes according to the
> > command-line options only), but the data exposed on CPUID[14h] on this 
> > patch is not migration-safe (it changes depending on the
> > host it is running).
> > 
> 
> Many thanks for your clarification.  I think I have understood.
> But CPUID[14h] is depend on CPUID[7].EBX[bit25] and it would be
> zero if CPUID[7].EBX[bit25] is not set. Or what you concerned
> is make live migration on two different HW which all support
> Intel PT virtualization but have different  CPUID[14h] result?
> This may need to think about.

Yes.  If intel-pt is off, the results (CPUID[14h] all zeroes) are
migration-safe.  Setting intel-pt=on makes the command-line not
migration-safe.

This is OK, in principle (e.g. the "pmu" option is not
migration-safe and behaves the same way).  The only problem is
that this patch would make query-cpu-model-expansion return
intel-pt=on on type=static expansion.

Probably the easiest way to fix that is to specifically exclude
intel-pt on x86_cpu_static_props().

However, if there's a simple way to make it possible to migrate
between hosts with different CPUID[14h] data, it would be even
better.  With the current KVM intel-pt implementation, what
happens if the CPUID[14h] data seen by the guest doesn't match
exactly the CPUID[14h] leaves from the host?

> 
> Thanks,
> Luwei Kang
> 
> > 
> > >
> > > >
> > > > 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".
> > >
> > > ==> it can be constant:
> > >     I think it shouldn't be constant because Intel PT virtualization can 
> > > only supported in Ice Lake hardware now. Intel PT cpuid info
> > would be mask off on old platform.
> > > ==> or it can be configurable on the command-line:
> > >     Because of I didn't add this feature in any CPU model. We only can 
> > > enable this feature by "-cpu Skylake-Server,+intel-pt" at
> > present.
> > 
> > Note that I'm talking about CPUID[14h], not CPUID[7].  The CPUID[7] bits 
> > are safe because they are set according to the CPU model
> > + command-line options only.  The bits on CPUID[14h] change depending on 
> > the host and are not migration-safe.  CPUID[7].EBX[bit
> > 25] is just the (already configurable) bit that enables the 
> > migration-unsafe code for CPUID[14h].
> > 
> > >
> > > What about add a new cpu model name "Icelake" and add PT in this. Is that 
> > > would be migration safe?
> > 
> > It won't, because of the CPUID[14h] code that makes it unsafe to migrate 
> > between hosts with different Intel-PT capabilities (i.e.
> > different data on CPUID[14h]).
> > 
> > 
> > >
> > > Thanks,
> > > Luwei Kang
> > >
> > > > 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
> > 
> > --
> > Eduardo
> 

-- 
Eduardo