[Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Paolo Bonzini]

Marshaling of processor tracing MSRs is not yet implemented in
QEMU, mark the feature as unmigratable.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9c54c41e7a..a6a597d2bf 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1012,6 +1012,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             .needs_ecx = true, .ecx = 0,
             .reg = R_EBX,
         },
+        .unmigratable_features = CPUID_7_0_EBX_INTEL_PT,
         .tcg_features = TCG_7_0_EBX_FEATURES,
     },
     [FEAT_7_0_ECX] = {
-- 
2.20.1

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Kang, Luwei]

> From: Qemu-devel [mailto:qemu-devel-bounces+luwei.kang=intel.com@nongnu.org] On Behalf Of Paolo Bonzini
> Sent: Friday, December 21, 2018 8:44 PM
> To: qemu-devel@nongnu.org
> Cc: ehabkost@redhat.com; qemu-stable@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.peng@linux.intel.com>
    Date:   Mon Mar 5 00:48:36 2018 +0800
    i386: Add support to get/set/migrate Intel Processor Trace feature

Thanks,
Luwei Kang

> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 9c54c41e7a..a6a597d2bf 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1012,6 +1012,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              .needs_ecx = true, .ecx = 0,
>              .reg = R_EBX,
>          },
> +        .unmigratable_features = CPUID_7_0_EBX_INTEL_PT,
>          .tcg_features = TCG_7_0_EBX_FEATURES,
>      },
>      [FEAT_7_0_ECX] = {
> --
> 2.20.1
> 

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181221124410.11920-1-pbonzini@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

libpmem support   no
libudev           no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled:  'cc'
  GEN     x86_64-softmmu/config-devices.mak.tmp
---
  CC      aarch64-softmmu/target/arm/translate.o
  CC      aarch64-softmmu/target/arm/op_helper.o
  CC      aarch64-softmmu/target/arm/helper.o
/tmp/qemu-test/src/target/i386/cpu.c:1015:9: error: unknown field 'unmigratable_features' specified in initializer
         .unmigratable_features = CPUID_7_0_EBX_INTEL_PT,
         ^
make[1]: *** [target/i386/cpu.o] Error 1


The full log is available at
http://patchew.org/logs/20181221124410.11920-1-pbonzini@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181221124410.11920-1-pbonzini@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/hw/misc/cbus.o
  CC      x86_64-softmmu/trace/generated-helpers.o
  CC      aarch64-softmmu/hw/misc/exynos4210_pmu.o
/tmp/qemu-test/src/target/i386/cpu.c:1015:10: error: 'FeatureWordInfo {aka struct FeatureWordInfo}' has no member named 'unmigratable_features'; did you mean 'unmigratable_flags'?
         .unmigratable_features = CPUID_7_0_EBX_INTEL_PT,
          ^~~~~~~~~~~~~~~~~~~~~
          unmigratable_flags


The full log is available at
http://patchew.org/logs/20181221124410.11920-1-pbonzini@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Paolo Bonzini]

On 25/12/18 09:23, Kang, Luwei wrote:
>> From: Qemu-devel [mailto:qemu-devel-bounces+luwei.kang=intel.com@nongnu.org] On Behalf Of Paolo Bonzini
>> Sent: Friday, December 21, 2018 8:44 PM
>> To: qemu-devel@nongnu.org
>> Cc: ehabkost@redhat.com; qemu-stable@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.peng@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?

Paolo

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Eduardo Habkost]

On Fri, Dec 28, 2018 at 02:53:22PM +0100, Paolo Bonzini wrote:
> On 25/12/18 09:23, Kang, Luwei wrote:
> >> From: Qemu-devel [mailto:qemu-devel-bounces+luwei.kang=intel.com@nongnu.org] On Behalf Of Paolo Bonzini
> >> Sent: Friday, December 21, 2018 8:44 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: ehabkost@redhat.com; qemu-stable@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.peng@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)?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Kang, Luwei]

> On 25/12/18 09:23, Kang, Luwei wrote:
> >> From: Qemu-devel
> >> [mailto:qemu-devel-bounces+luwei.kang=intel.com@nongnu.org] On Behalf
> >> Of Paolo Bonzini
> >> Sent: Friday, December 21, 2018 8:44 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: ehabkost@redhat.com; qemu-stable@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.peng@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?

Yes, it is indeed so long. I will check if it still works.

Thanks,
Luwei Kang

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Kang, Luwei]

> > On 25/12/18 09:23, Kang, Luwei wrote:
> > >> From: Qemu-devel
> > >> [mailto:qemu-devel-bounces+luwei.kang=intel.com@nongnu.org] On
> > >> Behalf Of Paolo Bonzini
> > >> Sent: Friday, December 21, 2018 8:44 PM
> > >> To: qemu-devel@nongnu.org
> > >> Cc: ehabkost@redhat.com; qemu-stable@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.peng@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).

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

Re: [Qemu-devel] [PATCH] i386: mark the 'INTEL_PT' CPUID bit as unmigratable

[Eduardo Habkost]

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.com@nongnu.org] On
> > > >> Behalf Of Paolo Bonzini
> > > >> Sent: Friday, December 21, 2018 8:44 PM
> > > >> To: qemu-devel@nongnu.org
> > > >> Cc: ehabkost@redhat.com; qemu-stable@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.peng@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