On Wed, Jan 13, 2021, Paolo Bonzini wrote:
> On 12/01/21 17:53, Sean Christopherson wrote:
> > And, masking bits 7:6 is architecturally wrong. Both the SDM and APM state that
> > bits 7:0 contain the number of PA bits.
>
> They cannot be higher than 52,
Drat, I was going to argue that it could be >52 with a new paging mode, but both
the SDM and APM explicitly call out 52 as the max. Spending cycles on the stuff
that really matters here... :-)
> therefore bits 7:6 are (architecturally)
> always zero. In other words, I interpret "bit 7:0 contain the number of PA
> bits" as "you need not do an '& 63' yourself", which is basically the
> opposite of "bit 7:6 might be nonzero". If masking made any difference, it
> would be outside the spec already.
>
> In fact another possibility to avoid UB is to do "& 63" of both s and e in
> rsvd_bits. This would also be masking bits 7:6 of the CPUID leaf, just done
> differently.
Hmm, 'e' is hardcoded in all call sites except kvm_mmu_reset_all_pte_masks(),
and so long as 'e <= 63' holds true, 's &= 63' is unnecessary. What if we add
compile-time asserts on hardcoded values, and mask 'e' for the rare case where
the upper bound isn't hardcoded? That way bogus things like rsvd_bits(63, 65)
will fail the build.
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 581925e476d6..261be1d2032b 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -44,8 +44,15 @@
#define PT32_ROOT_LEVEL 2
#define PT32E_ROOT_LEVEL 3
-static inline u64 rsvd_bits(int s, int e)
+static __always_inline u64 rsvd_bits(int s, int e)
{
+ BUILD_BUG_ON(__builtin_constant_p(e) && __builtin_constant_p(s) && e < s);
+
+ if (__builtin_constant_p(e))
+ BUILD_BUG_ON(e > 63);
+ else
+ e &= 63;
+
if (e < s)
return 0;
