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

[Paolo Bonzini]

On 22/01/2018 11:36, Kang, Luwei wrote:
>>> On Thu, Jan 18, 2018 at 03:44:49PM +0100, Paolo Bonzini wrote:
>>>> On 18/01/2018 15:37, Eduardo Habkost wrote:
>>>>> On Thu, Jan 18, 2018 at 02:39:57PM +0100, Paolo Bonzini wrote:
>>>>>> On 18/01/2018 14:24, Eduardo Habkost wrote:
>>>>>>> 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?
>>>>>>
>>>>>> Some bits in there can be treated as CPU features (e.g. EBX bit 0
>>>>>> "CR3 filtering support").  Probably we should handle these in KVM right 
>>>>>> now.
>>>>>> KVM needs to compute a mask of valid 1 bits for IA32_RTIT_CTL based
>>>>>> on CPUID, and apply it when the MSR is written.
>>>>>
>>>>> Does this mean QEMU can't set CPUID values that won't match the host
>>>>> with the existing implementation, or this won't matter for
>>>>> well-behaved guests that don't try to set reserved bits on the MSRs?
>>>>
>>>> All the features could be handled exactly like regular feature bits.
>>>> If QEMU sets them incorrectly and "enforce" is not used, bad things
>>>> happen but it's the user's fault.
>>>
>>> Oh, I mean setting the bit to 0 when it's 1 on the host (if it's
>>> 0 on the host, QEMU would never set it anyway).  Is it safe to do it
>>> with the current KVM intel-pt implementation?
>>
>> It's not, but it's (very) easy to fix.
> 
> Hi Paolo,
> Do you mean there need to add some check before setting IA32_RTIT_CTL
> MSR in KVM because some bits of this MSR is depend on the result of
> CPUID[14]. Any attempts to change these reserved bit should cause a #GP.

Yes, but the guest's CPUID[14] need not match the host.

Likewise, the number of address range MSRs in the guest, from
CPUID[EAX=14h,ECX=1].EAX[2:0], might be lower than in the host.

>>>>>>                                               It also needs to
>>>>>> whitelist bits like we do for other feature words.  These include:
>>>>>>
>>>>>> - CPUID[EAX=14h,ECX=0].EBX
>>>>>>
>>>>>> - CPUID[EAX=14h,ECX=0].ECX except bit 31
>>>>>>
>>>>>> - CPUID[EAX=14h,ECX=1].EAX bits 16:31 (if
>>>>>> CPUID[EAX=14h,ECX=0].EBX[3]=1)
>>>>>>
>>>>>> - CPUID[EAX=14h,ECX=1].EBX (if CPUID[EAX=14h,ECX=0].EBX[1]=1)
>>>>>
>>>>> What do you mean by whitelist?
>>>>
>>>> KVM needs to tell QEMU the bits it knows about.
> 
> I think kvm_arch_get_supported_cpuid() function can get the result of 
> CPUID[14] from KVM. Is this the whitelist what you mentioned?

Whitelist means that KVM must not return all the bits from CPUID[14];
only those it knows about.

Paolo

> Thanks,
> Luwei Kang
> 
>>>
>>> So KVM isn't currently doing it on GET_SUPPORTED_CPUID?  Oops.
>>>
>>>
>>>>
>>>>>> Others, currently only CPUID[EAX=14h,ECX=0].ECX[31] must match,
>>>>>> there is no way to emulate the "wrong" value.
>>>>>
>>>>> In this case we could make it configurable but require the host and
>>>>> guest value to always match.
>>>>>
>>>>> This might be an obstacle to enabling intel-pt by default (because
>>>>> it could make VMs not migratable to newer hosts), but may allow the
>>>>> feature to be configured in a predictable way.
>>>>
>>>> Yeah, but consider that virtualized PT anyway would only be enabled
>>>> on Ice Lake processors.  It's a few years away anyway!
>>>>
>>>>>> Others, currently only CPUID[EAX=14h,ECX=1].EAX[2:0] are numeric
>>>>>> values, and it's possible to emulate a lower value than the one in the 
>>>>>> processor.
>>>>>
>>>>> This could be handled by QEMU.  There's no requirement that all
>>>>> GET_SUPPORTED_CPUID values should be validated by simple bit
>>>>> masking.
>>>>
>>>> Good!
>>>>
>>>> Paolo
>>>
>