| Message ID | 20200130001023.24339-6-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Move x86 init ops to separate struct | expand |
On 30/01/20 01:10, Sean Christopherson wrote: > Set kvm_x86_ops with the vendor's ops only after ->hardware_setup() > completes to "prevent" using kvm_x86_ops before they are ready, i.e. to > generate a null pointer fault instead of silently consuming unconfigured > state. What about even copying kvm_x86_ops by value, so that they can be accessed with "kvm_x86_ops.callback()" and save one memory access? Paolo
On Thu, Jan 30, 2020 at 06:44:09AM +0100, Paolo Bonzini wrote: > On 30/01/20 01:10, Sean Christopherson wrote: > > Set kvm_x86_ops with the vendor's ops only after ->hardware_setup() > > completes to "prevent" using kvm_x86_ops before they are ready, i.e. to > > generate a null pointer fault instead of silently consuming unconfigured > > state. > > What about even copying kvm_x86_ops by value, so that they can be > accessed with "kvm_x86_ops.callback()" and save one memory access? Oooh, I like that idea. And {svm,vmx}_x86_ops could be marked __initdata to save a few bytes and force all the runtime stuff through kvm_x86_ops.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index eb36762aa2ce..a9f733c4ca28 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7326,8 +7326,6 @@ int kvm_arch_init(void *opaque) if (r) goto out_free_percpu; - kvm_x86_ops = ops->runtime_ops; - kvm_mmu_set_mask_ptes(PT_USER_MASK, PT_ACCESSED_MASK, PT_DIRTY_MASK, PT64_NX_MASK, 0, PT_PRESENT_MASK, 0, sme_me_mask); @@ -9588,6 +9586,8 @@ int kvm_arch_hardware_setup(void *opaque) if (r != 0) return r; + kvm_x86_ops = ops->runtime_ops; + cr4_reserved_bits = kvm_host_cr4_reserved_bits(&boot_cpu_data); if (kvm_has_tsc_control) {
Set kvm_x86_ops with the vendor's ops only after ->hardware_setup() completes to "prevent" using kvm_x86_ops before they are ready, i.e. to generate a null pointer fault instead of silently consuming unconfigured state. An alternative implementation would be to have ->hardware_setup() return the vendor's ops, but that would require non-trivial refactoring, and would arguably result in less readable code, e.g. ->hardware_setup() would need to use ERR_PTR() in multiple locations, and each vendor's declaration of the runtime ops would be less obvious. No functional change intended. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/x86.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)