On Thu, 2022-02-17 at 16:03 -0500, Paolo Bonzini wrote:
> Now that __kvm_mmu_new_pgd does not look at the MMU's root_level and
> shadow_root_level anymore, pull the PGD load after the initialization of
> the shadow MMUs.
Again, thanks a million for this! I once spend at least a hour figuring
out why my kernel panics when I did a similiar change, only to figure out
that __kvm_mmu_new_pgd needs to happen before mmu re-initialization.
>
> Besides being more intuitive, this enables future simplifications
> and optimizations because it's not necessary anymore to compute the
> role outside kvm_init_mmu. In particular, kvm_mmu_reset_context was not
> attempting to use a cached PGD to avoid having to figure out the new role.
> It will soon be able to follow what nested_{vmx,svm}_load_cr3 are doing,
> and avoid unloading all the cached roots.
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> arch/x86/kvm/mmu/mmu.c | 37 +++++++++++++++++--------------------
> arch/x86/kvm/svm/nested.c | 6 +++---
> arch/x86/kvm/vmx/nested.c | 6 +++---
> 3 files changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index da324a317000..906a9244ad28 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4903,9 +4903,8 @@ void kvm_init_shadow_npt_mmu(struct kvm_vcpu *vcpu, unsigned long cr0,
>
> new_role = kvm_calc_shadow_npt_root_page_role(vcpu, ®s);
>
> - __kvm_mmu_new_pgd(vcpu, nested_cr3, new_role.base);
> -
> shadow_mmu_init_context(vcpu, context, ®s, new_role);
> + __kvm_mmu_new_pgd(vcpu, nested_cr3, new_role.base);
> }
> EXPORT_SYMBOL_GPL(kvm_init_shadow_npt_mmu);
>
> @@ -4943,27 +4942,25 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
> kvm_calc_shadow_ept_root_page_role(vcpu, accessed_dirty,
> execonly, level);
>
> - __kvm_mmu_new_pgd(vcpu, new_eptp, new_role.base);
> -
> - if (new_role.as_u64 == context->mmu_role.as_u64)
> - return;
> -
> - context->mmu_role.as_u64 = new_role.as_u64;
> + if (new_role.as_u64 != context->mmu_role.as_u64) {
> + context->mmu_role.as_u64 = new_role.as_u64;
>
> - context->shadow_root_level = level;
> + context->shadow_root_level = level;
>
> - context->ept_ad = accessed_dirty;
> - context->page_fault = ept_page_fault;
> - context->gva_to_gpa = ept_gva_to_gpa;
> - context->sync_page = ept_sync_page;
> - context->invlpg = ept_invlpg;
> - context->root_level = level;
> - context->direct_map = false;
> + context->ept_ad = accessed_dirty;
> + context->page_fault = ept_page_fault;
> + context->gva_to_gpa = ept_gva_to_gpa;
> + context->sync_page = ept_sync_page;
> + context->invlpg = ept_invlpg;
> + context->root_level = level;
> + context->direct_map = false;
> + update_permission_bitmask(context, true);
> + context->pkru_mask = 0;
> + reset_rsvds_bits_mask_ept(vcpu, context, execonly, huge_page_level);
> + reset_ept_shadow_zero_bits_mask(context, execonly);
> + }
>
> - update_permission_bitmask(context, true);
> - context->pkru_mask = 0;
> - reset_rsvds_bits_mask_ept(vcpu, context, execonly, huge_page_level);
> - reset_ept_shadow_zero_bits_mask(context, execonly);
> + __kvm_mmu_new_pgd(vcpu, new_eptp, new_role.base);
> }
> EXPORT_SYMBOL_GPL(kvm_init_shadow_ept_mmu);
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index f284e61451c8..96bab464967f 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -492,14 +492,14 @@ static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
> CC(!load_pdptrs(vcpu, cr3)))
> return -EINVAL;
>
> - if (!nested_npt)
> - kvm_mmu_new_pgd(vcpu, cr3);
> -
> vcpu->arch.cr3 = cr3;
>
> /* Re-initialize the MMU, e.g. to pick up CR4 MMU role changes. */
> kvm_init_mmu(vcpu);
>
> + if (!nested_npt)
> + kvm_mmu_new_pgd(vcpu, cr3);
> +
> return 0;
> }
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b7bc634d35e2..1dfe23963a9e 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1126,15 +1126,15 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
> return -EINVAL;
> }
>
> - if (!nested_ept)
> - kvm_mmu_new_pgd(vcpu, cr3);
> -
> vcpu->arch.cr3 = cr3;
> kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
>
> /* Re-initialize the MMU, e.g. to pick up CR4 MMU role changes. */
> kvm_init_mmu(vcpu);
>
> + if (!nested_ept)
> + kvm_mmu_new_pgd(vcpu, cr3);
> +
> return 0;
> }
>
Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
Best regards,
Maxim Levitsky
