Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature

[Eduardo Habkost]

On Thu, Oct 24, 2019 at 01:36:50PM +0000, Kang, Luwei wrote:
> > > > > > > > > f9f4cd1..097c953 100644
> > > > > > > > > --- a/target/i386/kvm.c
> > > > > > > > > +++ b/target/i386/kvm.c
> > > > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, 
> > > > > > > > > int level)
> > > > > > > > >                  kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), 
> > > > > > > > > mask);
> > > > > > > > >              }
> > > > > > > > >          }
> > > > > > > > > +        if (env->features[FEAT_7_0_EBX] & 
> > > > > > > > > CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > > > +            int addr_num = 
> > > > > > > > > kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > > > +                                                    0x14,
> > > > > > > > > + 1,
> > > > > > > > > + R_EAX) & 0x7;
> > > > > > > > > +
> > > > > > > > > +            kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > > > +                            env->msr_rtit_ctrl);
> > > > > > > > > +            kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > > > +                            env->msr_rtit_status);
> > > > > > > > > +            kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > > > +                            env->msr_rtit_output_base);
> > > > > > > >
> > > > > > > > This causes the following crash on some hosts:
> > > > > > > >
> > > > > > > >   qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > > >   qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: 
> > > > > > > > Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > > > >
> > > > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > > > additional conditions that might prevent writing to this MSR
> > > > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output).  This
> > > > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > > > >
> > > > > > > > Writing and reading this MSR (and the ones below) need to be 
> > > > > > > > conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > > > >
> > > > > > >
> > > > > > > Hi Eduardo,
> > > > > > >     I found this issue can't be reproduced in upstream source
> > > > > > > code but can be reproduced on RHEL8.1. I haven't got the qemu
> > > > > > > source
> > > > > > code of RHEL8.1. But after adding some trace in KVM, I found the
> > > > > > KVM has reported the complete Intel PT CPUID information to qemu
> > > > > > but the Intel PT CPUID (0x14) is lost when qemu setting the
> > > > > > CPUID
> > > > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > > > >
> > > > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > > > Author: Luwei Kang <address@hidden>
> > > > > > > Date:   Tue Jan 29 18:52:59 2019 -0500
> > > > > > >     i386: extended the cpuid_level when Intel PT is enabled
> > > > > > >
> > > > > > >     Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > > >     have no change when create a kvm guest with
> > > > > > >     e.g. "-cpu qemu64,+intel-pt".
> > > > > >
> > > > > > Thanks for the pointer.  This may avoid triggering the bug in
> > > > > > the default configuration, but we still need to make the MSR
> > > > > > writing conditional on KVM_GET_MSR_INDEX_LIST.  Older
> > > > > > machine-types have x-intel-pt-auto-level=off, and the user may
> > > > > > set `level`
> > > > manually.
> > > > >
> > > > > Hi Eduardo,
> > > > > Sorry for a delay reply because my mail filter. I tried with the
> > > > > Q35 machine type and default, all looks work well (With some old
> > > > > cpu type
> > > > > + "intel_pt" also work well).  KVM will check the Intel PT work
> > > > > + mode
> > > > > and HW to decide if Intel PT can be exposed to guest, only
> > > > > extended the CPUID level is useless. If the guest doesn't support
> > > > > Intel PT, any MSR read or write will cause #GP. Please remind me
> > > > > if I lost something.
> > > >
> > > > I understand you have tried q35 and pc, but have you tried with older 
> > > > machine-type versions?
> > > >
> > > > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so 
> > > > it only avoids triggering the crash in the default case.
> > > > Doesn't QEMU crash if running:
> > > > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> > > >
> > > > KVM rejecting MSR writes when something is missing is correct.
> > > > QEMU trying to write the MSR when something is missing (and crashing 
> > > > because of that) is a bug.
> > >
> > > Hi Eduardo,
> > >     Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 
> > > 0x14 is lost because of the leaf number still 0xd (should 0x14).
> > >     May I remove the "off" like this?
> > 
> > We can't.  This is necessary to keep guest ABI compatibility.
> > Instead, we need to make QEMU not crash if xlevel is too low, because 
> > xlevel can be configured by the user.
> 
> Thanks Eduardo.  But I think it is a little complex for user.
> User found crash but how does he know it need to configure the
> xlevel or others?
> If we want to the old machine type support PT can we add some
> code to extend the level to 0x14? Or old machine type can't
> support PT,  mask off this feature from leaf 0x07.ebx[25]
> directly and output some messages?

I agree it's complex for the user, but let's address this
separately:

the first issue here is the crash: QEMU must not crash if using
(e.g.) "-cpu ...,+intel-pt,xlevel=0x13".  This can't be solved by
making any machine-type changes.

The second issue is usability.  This is hard to fix on old
machine-types because we must keep guest ABI compatibility.

In QEMU 3.1 the results of:
  -machine pc-i440fx-3.1 -cpu qemu64,+intel-pt
was:
  CPUID[0].EAX (level) = 7
  CPUID[7].EBX[25] (intel-pt) = 1

and we can't change the behavior of pc-i440fx-3.1.

Your suggestion of printing a warning is good, though.  We can do
that if intel-pt is enabled and level < 0x14.

> 
> Luwei Kang
> 
> > 
> > >
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> > >      { "Icelake-Client" "-" TYPE_X86_CPU,      "mpx", "on" },
> > >      { "Icelake-Server" "-" TYPE_X86_CPU,      "mpx", "on" },
> > >      { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> > > -    { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> > >  };
> > >  const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
> > >
> > > Thanks,
> > > Luwei Kang
> > >
> > > >
> > > > --
> > > > Eduardo
> > >
> > 
> > --
> > Eduardo
> 

-- 
Eduardo