On 06/13/2018 04:39 AM, Paolo Bonzini wrote:
>> +
>> +union kvm_mmu_page_role
>> +kvm_mmu_calc_root_page_role(struct kvm_vcpu *vcpu);
>> +
>> +union kvm_mmu_page_role
>> +kvm_calc_shadow_mmu_root_page_role(struct kvm_vcpu *vcpu);
>> +
>> +union kvm_mmu_page_role
>> +kvm_mmu_calc_shadow_ept_root_page_role(struct kvm_vcpu *vcpu,
>> + bool accessed_dirty);
> Having these public is a little ugly.
>
> Really only kvm_mmu_calc_root_page_role is needed but perhaps it's even
> better if you have something like
>
> void kvm_mmu_new_cr3(struct kvm_vcpu *vcpu, gpa_t cr3)
> {
> __kvm_mmu_new_cr3(vcpu, cr3, kvm_mmu_calc_root_page_role(vcpu));
> }
>
> Then vmx.c can initialize the vcpu->arch.mmu.* function pointers
> *before* calling kvm_init_shadow_ept_mmu, and kvm_init_shadow_ept_mmu
> can call __kvm_mmu_new_cr3 itself because it can get the eptp. This
> also avoids computing the role twice, once in vmx.c and once in
> kvm_init_shadow_ept_mmu.
Sounds good. I actually don't think that we even need to move the initialization of the function pointers before the kvm_init_shadow_ept_mmu() call. Just doing the __kvm_mmu_new_cr3() call (along with the role calculation) at the start of kvm_init_shadow_ept_mmu() should be enough.
Thanks,
Junaid
