Re: [PATCH v6 1/9] x86/cpu: KVM: Add common defines for architectural memory types (PAT, MTRRs, etc.)

From: Huang, Kai
Date: Wed Apr 03 2024 - 17:17:45 EST




On 4/04/2024 7:57 am, Sean Christopherson wrote:
On Wed, Mar 27, 2024, Kai Huang wrote:
On Fri, 2024-03-08 at 17:27 -0800, Sean Christopherson wrote:
Add defines for the architectural memory types that can be shoved into
various MSRs and registers, e.g. MTRRs, PAT, VMX capabilities MSRs, EPTPs,
etc. While most MSRs/registers support only a subset of all memory types,
the values themselves are architectural and identical across all users.

Leave the goofy MTRR_TYPE_* definitions as-is since they are in a uapi
header, but add compile-time assertions to connect the dots (and sanity
check that the msr-index.h values didn't get fat-fingered).

Keep the VMX_EPTP_MT_* defines so that it's slightly more obvious that the
EPTP holds a single memory type in 3 of its 64 bits; those bits just
happen to be 2:0, i.e. don't need to be shifted.

Opportunistically use X86_MEMTYPE_WB instead of an open coded '6' in
setup_vmcs_config().

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>


[...]

#include "mtrr.h"
+static_assert(X86_MEMTYPE_UC == MTRR_TYPE_UNCACHABLE);
+static_assert(X86_MEMTYPE_WC == MTRR_TYPE_WRCOMB);
+static_assert(X86_MEMTYPE_WT == MTRR_TYPE_WRTHROUGH);
+static_assert(X86_MEMTYPE_WP == MTRR_TYPE_WRPROT);
+static_assert(X86_MEMTYPE_WB == MTRR_TYPE_WRBACK);
+


Hi Sean,

IIUC, the purpose of this patch is for the kernel to use X86_MEMTYPE_xx, which
are architectural values, where applicable?

Maybe? Probably?

Yeah we need to keep MTRR_TYPE_xx in the uapi header, but in the kernel, should
we change all places that use MTRR_TYPE_xx to X86_MEMTYPE_xx? The
static_assert()s above have guaranteed the two are the same, so there's nothing
wrong for the kernel to use X86_MEMTYPE_xx instead.

Both PAT_xx and VMX_BASIC_MEM_TYPE_xx to X86_MEMTYPE_xx, it seems a little bit
odd if we don't switch for MTRR_TYPE_xx.

However by simple search MEM_TYPE_xx are intensively used in many files, so...

Yeah, I definitely don't want to do it in this series due to the amount of churn
that would be required.

$ git grep MTRR_TYPE_ | wc -l
100

I'm not even entirely convinced that it would be a net positive. Much of the KVM
usage that's being cleaned up is flat out wrong, e.g. using "MTRR" enums in places
that having nothing to do with MTRRs. But the majority of the remaining usage is
in MTRR code, i.e. isn't wrong, and is arguably better off using the MTRR specific
#defines.

Yeah understood.

But the patch title says we also "add common defines for ... MTRRs", so to me looks we should get rid of MTRR_TPYE_xx and use the common ones instead. And it also looks a little bit inconsistent if we remove the PAT_xx but keep the MTRR_TYPE_xx.

Perhaps we can keep PAT_xx but add macros?

#define PAT_UC X86_MEMTYPE_UC
...

But looks not nice either because the only purpose is to keep the PAT_xx..

So up to you :-)