Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> On Wed, Jan 22, 2025, David Woodhouse wrote:
>> On Wed, 2025-01-22 at 18:44 +0100, Vitaly Kuznetsov wrote:
>> > > What is the purpose of the comparison anyway?
>
> To avoid scenarios where KVM has configured state for a set of features X, and
> doesn't correctly handle vCPU features suddenly become Y. Or more commonly,
> where correctly handling such transitions (if there's even a "correct" option)
> is a complete waste of time and complexity because no sane setup will ever add
> and/or remove features from a running VM.
>
>> > > IIUC we want to ensure that a VMM does not change its mind after KVM_RUN
>> > > so should we not be stashing what was set by the VMM and comparing
>> > > against that *before* mangling any values?
>> >
>> > I guess it can be done this way but we will need to keep these 'original'
>> > unmangled values for the lifetime of the vCPU with very little gain (IMO):
>> > KVM_SET_CPUID{,2} either fails (if the data is different) or does (almost)
>> > nothing when the data is the same.
>
> More importantly, userspace is allowed to set the CPUID returned by KVM_GET_CPUID2.
> E.g. selftests do KVM_GET_CPUID2 specifically to read the bits that are managed
> by KVM.
>
> Disallowing that would likely break userspace, and would create a weird ABI where
> the output of KVM_GET_CPUID2 is rejected by KVM_SET_CPUID2.
>
>> If they're supposed to be entirely unchanged, would it suffice just to
>> keep a hash of them?
In case we want to support both cases:
- VMM calls KVM_SET_CPUID2 at some point in vCPU's lifetime with the
same data it used initially;
- VMM does KVM_GET_CPUID2 and feeds this directly into KVM_SET_CPUID2
we can't use a hash as the later contains entries mangled by
KVM. Currently, we kind of support both but we expect the result of the
mangling done by KVM to always be the same.
I guess we can change the logic the following: when KVM_SET_CPUID2 is
called on a vCPU again we check that all entries which KVM did not touch
match. For that, we will need to keep a list of mangled entries so we
can introduce a kvm_mangle_cpuid_entry() helper to avoid the need to
keep a static list. Personally, I'm not sure this is not an overkill
though.
--
Vitaly
