On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > /*
> > * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
> > * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
> > @@ -440,6 +440,15 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
> > * whether the supplied CPUID data is equal to what's already set.
> > */
> > if (kvm_vcpu_has_run(vcpu)) {
> > + /*
> > + * Note, runtime CPUID updates may consume other CPUID-driven
> > + * vCPU state, e.g. KVM or Xen CPUID bases. Updating runtime
> > + * state before full CPUID processing is functionally correct
> > + * only because any change in CPUID is disallowed, i.e. using
> > + * stale data is ok because KVM will reject the change.
> > + */
>
> If I understand correctly the sole reason for the below
> __kvm_update_cpuid_runtime is to ensure that kvm_cpuid_check_equal doesn't
> fail because current cpuid also was post-processed with runtime updates.
Yep.
> Can we have a comment stating this? Or even better how about moving the
> call to __kvm_update_cpuid_runtime into the kvm_cpuid_check_equal,
> to emphasize this?
Ya, I'll do both.
> > + __kvm_update_cpuid_runtime(vcpu, e2, nent);
> > +
> > r = kvm_cpuid_check_equal(vcpu, e2, nent);
> > if (r)
> > return r;
>
>
>
> Overall I am not 100% sure what is better:
>
> Before the patch it was roughly like this:
>
> 1. Post process the user given cpuid with bits of KVM runtime state (like xcr0)
> At that point the vcpu->arch.cpuid_entries is stale but consistent, it is just old CPUID.
>
> 2. kvm_hv_vcpu_init call (IMHO this call can be moved to kvm_vcpu_after_set_cpuid)
>
> 3. kvm_check_cpuid on the user provided cpuid
>
> 4. Update the vcpu->arch.cpuid_entries with new and post processed cpuid
>
> 5. kvm_get_hypervisor_cpuid - I think this also can be cosmetically moved to kvm_vcpu_after_set_cpuid
>
> 6. kvm_vcpu_after_set_cpuid itself.
>
>
> After this change it works like that:
>
> 1. kvm_hv_vcpu_init (again this belongs more to kvm_vcpu_after_set_cpuid)
> 2. kvm_check_cpuid on the user cpuid without post processing - in theory this can cause bugs
> 3. Update the vcpu->arch.cpuid_entries with new cpuid but without post-processing
> 4. kvm_get_hypervisor_cpuid
> 5. kvm_update_cpuid_runtime
> 6. The old kvm_vcpu_after_set_cpuid
>
> I'm honestly not sure what is better but IMHO moving the kvm_hv_vcpu_init and
> kvm_get_hypervisor_cpuid into kvm_vcpu_after_set_cpuid would clean up this
> mess a bit regardless of this patch.
It takes many more patches, but doing the swap() allows for the removal of several
APIs that poke into a "raw" kvm_cpuid_entry2 array, and by the end of the series
(with your above feedback addressed) the code gets to (sans comments):
swap(vcpu->arch.cpuid_entries, e2);
swap(vcpu->arch.cpuid_nent, nent);
memcpy(vcpu_caps, vcpu->arch.cpu_caps, sizeof(vcpu_caps));
BUILD_BUG_ON(sizeof(vcpu_caps) != sizeof(vcpu->arch.cpu_caps));
if (kvm_vcpu_has_run(vcpu)) {
r = kvm_cpuid_check_equal(vcpu, e2, nent);
if (r)
goto err;
goto success;
}
Those are really just bonuses though, the main goal is to prevent recurrences of
bugs where KVM consumes stale vCPU state[*], which is what prompted this change.
[*] https://lore.kernel.org/all/20240228101837.93642-1-vkuznets@xxxxxxxxxx
