On 22/10/24 19:30, Sean Christopherson wrote:
> On Tue, Oct 22, 2024, Adrian Hunter wrote:
>> On 14/10/24 21:25, Sean Christopherson wrote:
>>>> Fixes: 2ef444f1600b ("KVM: x86: Add Intel PT context switch for each vcpu")
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>
>>> This is way, way too big for stable@. Given that host/guest mode is disabled by
>>> default and that no one has complained about this, I think it's safe to say that
>>> unless we can provide a minimal patch, fixing this in LTS kernels isn't a priority.
>>>
>>> Alternatively, I'm tempted to simply drop support for host/guest mode. It clearly
>>> hasn't been well tested, and given the lack of bug reports, likely doesn't have
>>> many, if any, users. And I'm guessing the overhead needed to context switch all
>>> the RTIT MSRs makes tracing in the guest relatively useless.
>>
>> As a control flow trace, it is not affected by context switch overhead.
>
> Out of curiosity, how much is Intel PT used purely for control flow tracing, i.e.
> without caring _at all_ about perceived execution time?
It can be used to try to understand how code went wrong.
But timestamps would still indicate what was happening on different VCPUs
at about the same time.
>
>> Intel PT timestamps are also not affected by that.
>
> Timestamps are affected because the guest will see inexplicable jumps in time.
Tracing a task on the host sees jumps in time for scheduler
context switches anyway.
> Those gaps are unavoidable to some degree, but context switching on every entry
> and exit is
>
>> This patch reduces the MSR switching.
>
> To be clear, I'm not objecting to any of the ideas in this patch, I'm "objecting"
> to trying to put band-aids on KVM's existing implementation, which is clearly
> buggy and, like far too many PMU-ish features in KVM, was probably developed
> without any thought as to how it would affect use cases beyond the host admin
> and the VM owner being a single person. And I'm also objecting, vehemently, to
> sending anything of this magnitude and complexity to LTS kernels.
That's your call for sure.
>
>>> /me fiddles around
>>>
>>> LOL, yeah, this needs to be burned with fire. It's wildly broken. So for stable@,
>>
>> It doesn't seem wildly broken. Just the VMM passing invalid CPUID
>> and KVM not validating it.
>
> Heh, I agree with "just", but unfortunately "just ... not validating" a large
> swath of userspace inputs is pretty widly broken. More importantly, it's not
> easy to fix. E.g. KVM could require the inputs to exactly match hardware, but
> that creates an ABI that I'm not entirely sure is desirable in the long term.
Although the CPUID ABI does not really change. KVM does not support
emulating Intel PT, so accepting CPUID that the hardware cannot support
seems like a bit of a lie. Aren't there other features that KVM does not
support if the hardware support is not there?
To some degree, a testing and debugging feature does not have to be
available in 100% of cases because it can still be useful when it is
available.
>
>>> I'll post a patch to hide the module param if CONFIG_BROKEN=n (and will omit
>>> stable@ for the previous patch).
>>>
>>> Going forward, if someone actually cares about virtualizing PT enough to want to
>>> fix KVM's mess, then they can put in the effort to fix all the bugs, write all
>>> the tests, and in general clean up the implementation to meet KVM's current
>>> standards. E.g. KVM usage of intel_pt_validate_cap() instead of KVM's guest CPUID
>>> and capabilities infrastructure needs to go.
>>
>> The problem below seems to be caused by not validating against the *host*
>> CPUID. KVM's CPUID information seems to be invalid.
>
> Yes.
>
>>> My vote is to queue the current code for removal, and revisit support after the
>>> mediated PMU has landed. Because I don't see any point in supporting Intel PT
>>> without a mediated PMU, as host/guest mode really only makes sense if the entire
>>> PMU is being handed over to the guest.
>>
>> Why?
>
> To simplify the implementation, and because I don't see how virtualizing Intel PT
> without also enabling the mediated PMU makes any sense.
>
> Conceptually, KVM's PT implementation is very, very similar to the mediated PMU.
> They both effectively give the guest control of hardware when the vCPU starts
> running, and take back control when the vCPU stops running.
>
> If KVM allows Intel PT without the mediated PMU, then KVM and perf have to support
> two separate implementations for the same model. If virtualizing Intel PT is
> allowed if and only if the mediated PMU is enabled, then .handle_intel_pt_intr()
> goes away. And on the flip side, it becomes super obvious that host usage of
> Intel PT needs to be mutually exclusive with the mediated PMU.
And forgo being able to trace mediated passthough with Intel PT ;-)
>
>> Intel PT PMU is programmed separately from the x86 PMU.
>
> Except for the minor detail that Intel PT generates PMIs, and that PEBS can log
> to PT buffers. Oh, and giving the guest control of the PMU means host usage of
> Intel PT will break the host *and* guest. The host won't get PMIs, while the
> guest will see spurious PMIs.
Yes the PMI is in conflict if it is not also a VM-Exit
Note that Intel PT does have a snapshot mode that doesn't use a PMI.
Trace data continuously overwrites the ring buffer until the user asks
for a snapshot.
>
> So I don't see any reason to try to separate the two.
OK
>
>>> [ 1458.686107] ------------[ cut here ]------------
>>> [ 1458.690766] Invalid MSR 588, please adapt vmx_possible_passthrough_msrs[]
>>
>> VMM is trying to set a non-existent MSR. Looks like it has
>> decided there are more PT address filter MSRs that are architecturally
>> possible.
>>
>> I had no idea QEMU was so broken.
>
> It's not QEMU that's broken, it's KVM that's broken.
>
>> I always just use -cpu host.
>
> Yes, and that's exactly the problem. The only people that have ever touched this
> likely only ever use `-cpu host`, and so KVM's flaws have gone unnoticed.
>
>> What were you setting?
>
> I tweaked your selftest to feed KVM garbage.
