[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID b
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-stable] [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable |
Date: |
Sat, 5 Jan 2019 00:35:22 -0200 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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:address@hidden On
> > > >> Behalf Of Paolo Bonzini
> > > >> Sent: Friday, December 21, 2018 8:44 PM
> > > >> To: address@hidden
> > > >> Cc: address@hidden; address@hidden
> > > >> 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 <address@hidden>
> > > > 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