On Mon, Feb 27, 2023, Robert Hoo wrote:
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 835426254e76..3efec7f8d8c6 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3699,7 +3699,14 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
> int quadrant, i, r;
> hpa_t root;
>
> - root_pgd = mmu->get_guest_pgd(vcpu);
> + /*
> + * Omit guest_cpuid_has(X86_FEATURE_LAM) check but can unconditionally
> + * strip CR3 LAM bits because they resides in high reserved bits,
> + * with LAM or not, those high bits should be striped anyway when
> + * interpreted to pgd.
> + */
This misses the most important part: why it's safe to ignore LAM bits when reusing
a root.
> + root_pgd = mmu->get_guest_pgd(vcpu) &
> + ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
Unconditionally clearing LAM bits is unsafe. At some point the EPTP may define
bits in the same area that must NOT be omitted from the root cache, e.g. the PWL
bits in the EPTP _need_ to be incorporated into is_root_usable().
For simplicity, I'm very, very tempted to say we should just leave the LAM bits
in root.pgd, i.e. force a new root for a CR3+LAM combination. First and foremost,
that only matters for shadow paging. Second, unless a guest kernel allows per-thread
LAM settings, KVM the extra checks will be benign. And AIUI, the proposed kernel
implementation is to apply LAM on a per-MM basis.
And I would much prefer to solve the GFN calculation generically. E.g. it really
should be something like this
root_pgd = mmu->get_guest_pgd(vcpu);
root_gfn = mmu->gpte_to_gfn(root_pgd);
but having to set gpte_to_gfn() in the MMU is quite unfortunate, and gpte_to_gfn()
is technically insufficient for PAE since it relies on previous checks to prevent
consuming a 64-bit CR3.
I was going to suggest extracting the maximal base addr mask and use that, e.g.
#define __PT_BASE_ADDR_MASK (((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1))
Maybe this?
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c8ebe542c565..8b2d2a6081b3 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3732,7 +3732,12 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
hpa_t root;
root_pgd = mmu->get_guest_pgd(vcpu);
- root_gfn = root_pgd >> PAGE_SHIFT;
+
+ /*
+ * The guest PGD has already been checked for validity, unconditionally
+ * strip non-address bits when computing the GFN.
+ */
+ root_gfn = (root_pgd & __PT_BASE_ADDR_MASK) >> PAGE_SHIFT;
if (mmu_check_root(vcpu, root_gfn))
return 1;
diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
index cc58631e2336..c0479cbc2ca3 100644
--- a/arch/x86/kvm/mmu/mmu_internal.h
+++ b/arch/x86/kvm/mmu/mmu_internal.h
@@ -21,6 +21,7 @@ extern bool dbg;
#endif
/* Page table builder macros common to shadow (host) PTEs and guest PTEs. */
+#define __PT_BASE_ADDR_MASK (((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1))
#define __PT_LEVEL_SHIFT(level, bits_per_level) \
(PAGE_SHIFT + ((level) - 1) * (bits_per_level))
#define __PT_INDEX(address, level, bits_per_level) \
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 57f0b75c80f9..0583bfce3b52 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -62,7 +62,7 @@
#endif
/* Common logic, but per-type values. These also need to be undefined. */
-#define PT_BASE_ADDR_MASK ((pt_element_t)(((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1)))
+#define PT_BASE_ADDR_MASK ((pt_element_t)__PT_BASE_ADDR_MASK)
#define PT_LVL_ADDR_MASK(lvl) __PT_LVL_ADDR_MASK(PT_BASE_ADDR_MASK, lvl, PT_LEVEL_BITS)
#define PT_LVL_OFFSET_MASK(lvl) __PT_LVL_OFFSET_MASK(PT_BASE_ADDR_MASK, lvl, PT_LEVEL_BITS)
#define PT_INDEX(addr, lvl) __PT_INDEX(addr, lvl, PT_LEVEL_BITS)
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 1279db2eab44..777f7d443e3b 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -36,7 +36,7 @@ static_assert(SPTE_TDP_AD_ENABLED == 0);
#ifdef CONFIG_DYNAMIC_PHYSICAL_MASK
#define SPTE_BASE_ADDR_MASK (physical_mask & ~(u64)(PAGE_SIZE-1))
#else
-#define SPTE_BASE_ADDR_MASK (((1ULL << 52) - 1) & ~(u64)(PAGE_SIZE-1))
+#define SPTE_BASE_ADDR_MASK __PT_BASE_ADDR_MASK
#endif
#define SPTE_PERM_MASK (PT_PRESENT_MASK | PT_WRITABLE_MASK | shadow_user_mask \
> root_gfn = root_pgd >> PAGE_SHIFT;
>
> if (mmu_check_root(vcpu, root_gfn))
> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index 0f6455072055..57f39c7492ed 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -324,7 +324,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
> trace_kvm_mmu_pagetable_walk(addr, access);
> retry_walk:
> walker->level = mmu->cpu_role.base.level;
> - pte = mmu->get_guest_pgd(vcpu);
> + pte = mmu->get_guest_pgd(vcpu) & ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
This should be unnecessary, gpte_to_gfn() is supposed to strip non-address bits.
> have_ad = PT_HAVE_ACCESSED_DIRTY(mmu);
>
> #ifdef CONFIG_X86_64
> bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
>
> @@ -1254,14 +1265,26 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> * stuff CR3, e.g. for RSM emulation, and there is no guarantee that
> * the current vCPU mode is accurate.
> */
> - if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
> + if (!kvm_vcpu_is_valid_cr3(vcpu, cr3))
> return 1;
>
> if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
> return 1;
>
> - if (cr3 != kvm_read_cr3(vcpu))
> - kvm_mmu_new_pgd(vcpu, cr3);
> + old_cr3 = kvm_read_cr3(vcpu);
> + if (cr3 != old_cr3) {
> + if ((cr3 ^ old_cr3) & ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57)) {
> + kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 |
> + X86_CR3_LAM_U57));
As above, no change is needed here if LAM is tracked in the PGD.
