On Wed, Nov 15, 2023, Robert Hoo wrote: > On 11/14/2023 9:48 PM, Sean Christopherson wrote: > > On Mon, Nov 13, 2023, Robert Hoo wrote: > ... > > > u32 *caps = vcpu->arch.cpu_caps; > > > and update guest_cpu_cap_set(), guest_cpu_cap_clear(), > > > guest_cpu_cap_change() and guest_cpu_cap_restrict() to pass in > > > vcpu->arch.cpu_caps instead of vcpu, since all of them merely refer to vcpu > > > cap, rather than whole vcpu info. > > > > No, because then every caller would need extra code to pass > > vcpu->cpu_caps, > > Emm, I don't understand this. I tried to modified and compiled, all need to > do is simply substitute "vcpu" with "vcpu->arch.cpu_caps" in calling. (at > the end is my diff based on this patch set) Yes, and I'm saying that guest_cpu_cap_restrict(vcpu, X86_FEATURE_PAUSEFILTER); guest_cpu_cap_restrict(vcpu, X86_FEATURE_PFTHRESHOLD); guest_cpu_cap_restrict(vcpu, X86_FEATURE_VGIF); guest_cpu_cap_restrict(vcpu, X86_FEATURE_VNMI); is harder to read and write than this guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_PAUSEFILTER); guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_PFTHRESHOLD); guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_VGIF); guest_cpu_cap_restrict(vcpu->arch.cpu_caps, X86_FEATURE_VNMI); a one-time search-replace is easy, but the extra boilerplate has a non-zero cost for every future developer/reader. > > and passing 'u32 *' provides less type safety than 'struct kvm_vcpu *'. > > That tradeoff isn't worth making this one path slightly easier to read. > > My point is also from vulnerability, long term, since as a principle, we'd > better pass in param/info to a function of its necessity. Attempting to apply the principle of least privilege to low level C helpers is nonsensical. E.g. the helper can trivially get at the owning vcpu via container_of() (well, if not for typeof assertions not playing nice with arrays, but open coding container_of() is also trivial and illustrates the point). struct kvm_vcpu_arch *arch = (void *)caps - offsetof(struct kvm_vcpu_arch, cpu_caps); struct kvm_vcpu *vcpu = container_of(arch, struct kvm_vcpu, arch); if (!kvm_cpu_cap_has(x86_feature)) guest_cpu_cap_clear(vcpu, x86_feature); And the intent behind that principle is to improve security/robustness; what I'm saying is that passing in a 'u32 *" makes the overall implementation _less_ robust, as it opens up the possibilities of passing in an unsafe/incorrect pointer. E.g. a well-intentioned, not _that_ obviously broken example is: guest_cpu_cap_restrict(&vcpu->arch.cpu_caps[CPUID_1_ECX], X86_FEATURE_XSAVE); > e.g. cpuid_entry2_find(). The main reason cpuid_entry2_find() exists is because KVM checks the incoming array provided by KVM_SET_CPUID2, which is also the reason why __kvm_update_cpuid_runtime() takes an @entries array instead of just @vcpu.
