Messages in this thread | | | Date | Mon, 27 Nov 2023 16:43:45 -0800 | Subject | Re: [PATCH 6/9] KVM: x86: Update guest cpu_caps at runtime for dynamic CPUID-based features | From | Sean Christopherson <> |
| |
On Fri, Nov 24, 2023, Xu Yilun wrote: > On Sun, Nov 19, 2023 at 07:35:30PM +0200, Maxim Levitsky wrote: > > On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote: > > > static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries, > > > int nent) > > > { > > > struct kvm_cpuid_entry2 *best; > > > + struct kvm_vcpu *caps = vcpu; > > > + > > > + /* > > > + * Don't update vCPU capabilities if KVM is updating CPUID entries that > > > + * are coming in from userspace! > > > + */ > > > + if (entries != vcpu->arch.cpuid_entries) > > > + caps = NULL; > > > > I think that this should be decided by the caller. Just a boolean will suffice.
I strongly disagree. The _only_ time the caps should be updated is if entries == vcpu->arch.cpuid_entries, and if entries == cpuid_entires than the caps should _always_ be updated.
> kvm_set_cpuid() calls this function only to validate/adjust the temporary > "entries" variable. While kvm_update_cpuid_runtime() calls it to do system > level changes. > > So I kind of agree to make the caller fully awared, how about adding a > newly named wrapper for kvm_set_cpuid(), like: > > > static void kvm_adjust_cpuid_entry(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries, > int nent) > > { > WARN_ON(entries == vcpu->arch.cpuid_entries); > __kvm_update_cpuid_runtime(vcpu, entries, nent);
But taking it a step further, we end up with
WARN_ON_ONCE(update_caps != (entries == vcpu->arch.cpuid_entries));
which is silly since any bugs that would result in the WARN firing can be avoided by doing:
update_caps = entries == vcpu->arch.cpuid_entries;
which eventually distils down to the code I posted.
> > Or even better: since the userspace CPUID update is really not important in > > terms of performance, why to special case it? > > > > Even if these guest caps are later overwritten, I don't see why we need to > > avoid updating them, and in fact introduce a small risk of them not being > > consistent > > IIUC, for kvm_set_cpuid() case, KVM may then fail the userspace cpuid setting, > so we can't change guest caps at this phase.
> Or even better: since the userspace CPUID update is really not important in > terms of performance, why to special case it?
Yep, and sadly __kvm_update_cpuid_runtime() *must* be invoked before kvm_set_cpuid() is guaranteed to succeed because the whole point is to massage guest CPUID before checking for divergences.
> > With this we can avoid having the 'cap' variable which is *very* confusing as well.
I agree the "caps" variable is confusing, but it's the least awful option I see. The alternatives I can think of are:
1. Update a dummy caps array 2. Take a snapshot of the caps and restore them 3. Have separate paths for updated guest CPUID versus guest caps
#1 would require passing a "u32 *" to guest_cpu_cap_change() (or an equivalent), which I really, really don't want to do. It' also a waste of cycles, and I'm skeptical that it would be any less confusing than the proposed code.
#2 increases the complexity of kvm_set_cpuid() by introducing recovery paths, i.e. adds more things that can break, and again is wasteful (though copying ~100 bytes or so in a slow path isn't a big deal).
#3 would create unnecessary maintenance burden as we'd have to ensure any changes hit both paths.
| |