On Wed, Jan 02, 2019 at 01:30:28AM +0000, Kang, Luwei wrote: > > > On 25/12/18 09:23, Kang, Luwei wrote: > > > >> From: Qemu-devel > > > >> [mailto:qemu-devel-bounces+luwei.kang=intel....@nongnu.org] On > > > >> Behalf Of Paolo Bonzini > > > >> Sent: Friday, December 21, 2018 8:44 PM > > > >> To: qemu-devel@nongnu.org > > > >> Cc: ehabk...@redhat.com; qemu-sta...@nongnu.org > > > >> Subject: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit > > > >> as unmigratable > > > >> > > > >> Marshaling of processor tracing MSRs is not yet implemented in QEMU, > > > >> mark the feature as unmigratable. > > > > > > > > Hi Paolo, > > > > I think Intel PT has supported live migration. I don't understand > > > > what you mean. > > > > > > > > commit b77146e9a129bcdb60edc23639211679ae846a92 > > > > Author: Chao Peng <chao.p.p...@linux.intel.com> > > > > Date: Mon Mar 5 00:48:36 2018 +0800 > > > > i386: Add support to get/set/migrate Intel Processor Trace > > > > feature > > > > > > Indeed. I had forgotten this patch because it was committed so long > > > before the kernel parts (which really should not happen, but Eduardo > > > and I miscommunicated on it). Can you check that it still works? > > > > My mistake, sorry. I should have checked the status of the kernel code > > before merging the original series, or waited for your review. > > > > I'm re-reading the code now and I'm worried about two things: > > > > The code will break if GET_SUPPORTED_CPUID returns INTEL_PT, but the MSR > > emulation code is not present yet. Maybe it won't be an > > issue in practice because it happens only between the two Linux commits > > (commit 86f5201df0d3 "KVM: x86: Add Intel Processor Trace > > cpuid emulation" and commit bf8c55d8dc09 "KVM: x86: Implement Intel PT MSRs > > read/write emulation") and shipping a kernel with the > > CPUID code without the MSR commit seems pointless. > > > > The kvm_arch_get_supported_cpuid() call inside kvm_get_msrs() looks > > suspicious. What should happen if we try to migrate to a host that > > returns a smaller number on kvm_arch_get_supported_cpuid(0x14, 1, R_EAX)? > > Hi Eduardo, > I think we have some discussion on this feature about live migration safe > about two years ago. In order to make live migration safe, we set all the > values of PT CPUID as constant. > I think what your concern is the number of address range > (CPUID:14H.01.EAX[bit02:00]). Currently, it is a constant value (#define > INTEL_PT_ADDR_RANGES_NUM 0x2) for guest. > 1. if the hardware support < 2, Intel PT will not exposed to guest; > 2. if the hardware support >= 2, we just expose 2 to guest. > So the number of address range in guest is always 2 if Intel PT is > supported in guest (there also have other pre-condition check).
Right, I forgot about that part of the code. So the CPU state save/load code simply saves/loads everything supported by the host, and the responsibility of keeping guest ABI is lies on target/i386/cpu.c. The code looks safe to me. The only unexpected side-effect is unnecessarily loading/saving of MSR_IA32_RTIT_ADDR[123]*, which should be harmless. Thanks for the explanation! > > The value of guest Intel PT CPUID information. > + if (count == 0) { > + *eax = INTEL_PT_MAX_SUBLEAF; > + *ebx = INTEL_PT_MINIMAL_EBX; > + *ecx = INTEL_PT_MINIMAL_ECX; > + } else if (count == 1) { > + *eax = INTEL_PT_MTC_BITMAP | INTEL_PT_ADDR_RANGES_NUM; > + *ebx = INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP; > + } > > The condition check if we can expose Intel PT to guest. > + if (!eax_0 || > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) || > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) || > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) || > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) < > + INTEL_PT_ADDR_RANGES_NUM) || > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) != > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) { > + /* > + * Processor Trace capabilities aren't configurable, so if the > + * host can't emulate the capabilities we report on > + * cpu_x86_cpuid(), intel-pt can't be enabled on the current > host. > + */ > + env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT; > + cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT; > + rv = 1; > + } > > > Thanks, > Luwei Kang -- Eduardo