On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > Drop the manual initialization of maxphyaddr and reserved_gpa_bits during > vCPU creation now that kvm_arch_vcpu_create() unconditionally invokes > kvm_vcpu_after_set_cpuid(), which handles all such CPUID caching. > > None of the helpers between the existing code in kvm_arch_vcpu_create() > and the call to kvm_vcpu_after_set_cpuid() consume maxphyaddr or > reserved_gpa_bits (though auditing vmx_vcpu_create() and svm_vcpu_create() > isn't exactly easy). And even if that weren't the case, KVM _must_ > refresh any affected state during kvm_vcpu_after_set_cpuid(), e.g. to > correctly handle KVM_SET_CPUID2. In other words, this can't introduce a > new bug, only expose an existing bug (of which there don't appear to be > any). IMHO the change is not as bulletproof as claimed: If some code does access the uninitialized state (e.g vcpu->arch.maxphyaddr which will be zero, I assume), in between these calls, then even though later the correct CPUID will be set and should override the incorrect state set earlier, the problem *is* that the mentioned code will have to deal with non architecturally possible value (e.g maxphyaddr == 0) which might cause a bug in it. Of course such code currently doesn't exist, so it works but it can fail in the future. How about we move the call to kvm_vcpu_after_set_cpuid upward? Best regards, Maxim Levitsky > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > --- > arch/x86/kvm/x86.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2f6dda723005..bb34891d2f0a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -12190,9 +12190,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > goto free_emulate_ctxt; > } > > - vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > - vcpu->arch.reserved_gpa_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu); > - > vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; > > kvm_async_pf_hash_reset(vcpu);
