On Sat, May 21, 2022, Lai Jiangshan wrote:
> +static bool using_local_root_page(struct kvm_mmu *mmu)
Hmm, I agree with David that "local" isn't the most intuitive terminology. But
I also do want to avoid private vs. shared to avoid confusion with confidential VMs.
Luckily, I don't think we need to come up with new terminology, just be literal
and call 'em "per-vCPU root pages". E.g.
static bool kvm_mmu_has_per_vcpu_root_page()
That way readers don't have to understand what "local" means, and that also captures
per-vCPU roots are an exception, i.e. that most roots are NOT per-vCPU.
> +{
> + return mmu->root_role.level == PT32E_ROOT_LEVEL ||
> + (!mmu->root_role.direct && mmu->cpu_role.base.level <= PT32E_ROOT_LEVEL);
> +}
> +
> static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct)
> {
> struct kvm_mmu_page *sp;
> @@ -4252,10 +4285,11 @@ static bool fast_pgd_switch(struct kvm *kvm, struct kvm_mmu *mmu,
> {
> /*
> * For now, limit the caching to 64-bit hosts+VMs in order to avoid
> - * having to deal with PDPTEs. We may add support for 32-bit hosts/VMs
> - * later if necessary.
> + * having to deal with PDPTEs. Local roots can not be put into
> + * mmu->prev_roots[] because mmu->pae_root can not be shared for
> + * different roots at the same time.
> */
> - if (VALID_PAGE(mmu->root.hpa) && !to_shadow_page(mmu->root.hpa))
> + if (unlikely(using_local_root_page(mmu)))
I don't know that I like using the local/per-vCPU helper. The problem isn't _just_
that KVM is using a per-vCPU root, KVM is also deliberately punting on dealing with
PDTPRs. E.g. the per-vCPU aspect doesn't explain why KVM doesn't allow reusing the
current root. I don't like that the using_local_root_page() obfuscates that check.
My preference for this would be to revert back to a streamlined variation of the
code prior to commit 5499ea73e7db ("KVM: x86/mmu: look for a cached PGD when going
from 32-bit to 64-bit").
KVM switched to the !to_shadow_page() check to _avoid_ consuming (what is now)
mmu->root_role because, at the time of the patch, mmu held the _old_ data, which
was wrong/stale for nested virtualization transitions.
In other words, I would prefer that explicitly do (in a separate patch):
/*
* For now, limit the fast switch to 64-bit VMs in order to avoid having
* to deal with PDPTEs. 32-bit VMs can be supported later if necessary.
*/
if (new_role.level < PT64_ROOT_LEVEL4)
kvm_mmu_free_roots(kvm, mmu, KVM_MMU_ROOT_CURRENT);
The "hosts+VMs" can be shortened to just "VMs", because running a 64-bit VM with
a 32-bit host just doesn't work for a variety of reasons, i.e. doesn't need to be
called out here.
