On Mon, Jan 22, 2024, Binbin Wu wrote:
> diff --git a/x86/lam.c b/x86/lam.c
> new file mode 100644
> index 00000000..0ad16be5
> --- /dev/null
> +++ b/x86/lam.c
> @@ -0,0 +1,243 @@
> +/*
> + * Intel LAM unit test
> + *
> + * Copyright (C) 2023 Intel
> + *
> + * Author: Robert Hoo <robert.hu@xxxxxxxxxxxxxxx>
> + * Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or
> + * later.
> + */
> +
> +#include "libcflat.h"
> +#include "processor.h"
> +#include "desc.h"
> +#include "vmalloc.h"
> +#include "alloc_page.h"
> +#include "vm.h"
> +#include "asm/io.h"
> +#include "ioram.h"
> +
> +#define FLAGS_LAM_ACTIVE BIT_ULL(0)
> +#define FLAGS_LA57 BIT_ULL(1)
> +
> +struct invpcid_desc {
> + u64 pcid : 12;
> + u64 rsv : 52;
> + u64 addr;
> +};
> +
> +static inline bool is_la57(void)
> +{
> + return !!(read_cr4() & X86_CR4_LA57);
> +}
> +
> +static inline bool lam_sup_active(void)
Needs an "is_" prefix. And be consistent, e.g. is_lam_sup() to go with is_la57(),
or is_lam_sup_enabled() and is_la57_enabled(). I'd probably vote for the latter,
though KVM does have is_paging() and the like, so I'm fine either way.
And these belong in processor.h
> +{
> + return !!(read_cr4() & X86_CR4_LAM_SUP);
> +}
> +
> +static void cr4_set_lam_sup(void *data)
> +{
> + unsigned long cr4;
> +
> + cr4 = read_cr4();
> + write_cr4_safe(cr4 | X86_CR4_LAM_SUP);
> +}
> +
> +static void cr4_clear_lam_sup(void *data)
> +{
> + unsigned long cr4;
> +
> + cr4 = read_cr4();
> + write_cr4_safe(cr4 & ~X86_CR4_LAM_SUP);
> +}
Please drop these helpers and instead use _safe() variants when possible, e.g.
vector = write_cr4_safe(cr4 | X86_CR4_LAM_SUP);
report(has_lam ? !vector : vector == GP_VECTOR,
"Expected CR4.LAM_SUP=1 to %s" ? has_lam ? "succeed" : "#GP");
vector = write_cr4_safe(cr4 & ~X86_CR4_LAM_SUP);
report(!vector, "Expected CR4.LAM_SUP=0 to succeed");
> +static void test_cr4_lam_set_clear(bool has_lam)
> +{
> + bool fault;
> +
> + fault = test_for_exception(GP_VECTOR, &cr4_set_lam_sup, NULL);
> + report((fault != has_lam) && (lam_sup_active() == has_lam),
> + "Set CR4.LAM_SUP");
> +
> + fault = test_for_exception(GP_VECTOR, &cr4_clear_lam_sup, NULL);
> + report(!fault, "Clear CR4.LAM_SUP");
> +}
> +
> +/* Refer to emulator.c */
> +static void do_mov(void *mem)
> +{
> + unsigned long t1, t2;
> +
> + t1 = 0x123456789abcdefull & -1ul;
> + asm volatile("mov %[t1], (%[mem])\n\t"
> + "mov (%[mem]), %[t2]"
> + : [t2]"=r"(t2)
> + : [t1]"r"(t1), [mem]"r"(mem)
> + : "memory");
> + report(t1 == t2, "Mov result check");
> +}
> +
> +static u64 test_ptr(u64 arg1, u64 arg2, u64 arg3, u64 arg4)
There's no reason to name these arg1..arg4. And unless I'm missing something,
there's no need for these flags at all. All the info is derived from vCPU state,
so just re-grab it. The cost of a CR4 read is negligible relative to the expected
runtime of these tests.
> +{
> + bool lam_active = !!(arg1 & FLAGS_LAM_ACTIVE);
> + u64 lam_mask = arg2;
> + u64 *ptr = (u64 *)arg3;
> + bool is_mmio = !!arg4;
> + bool fault;
> +
> + fault = test_for_exception(GP_VECTOR, do_mov, ptr);
> + report(!fault, "Test untagged addr (%s)", is_mmio ? "MMIO" : "Memory");
> +
> + ptr = (u64 *)set_la_non_canonical((u64)ptr, lam_mask);
> + fault = test_for_exception(GP_VECTOR, do_mov, ptr);
> + report(fault != lam_active,"Test tagged addr (%s)",
> + is_mmio ? "MMIO" : "Memory");
> +
> + return 0;
> +}
> +
> +static void do_invlpg(void *mem)
> +{
> + invlpg(mem);
> +}
> +
> +static void do_invlpg_fep(void *mem)
> +{
> + asm volatile(KVM_FEP "invlpg (%0)" ::"r" (mem) : "memory");
> +}
> +
> +/* invlpg with tagged address is same as NOP, no #GP expected. */
> +static void test_invlpg(u64 lam_mask, void *va, bool fep)
> +{
> + bool fault;
> + u64 *ptr;
> +
> + ptr = (u64 *)set_la_non_canonical((u64)va, lam_mask);
> + if (fep)
> + fault = test_for_exception(GP_VECTOR, do_invlpg_fep, ptr);
> + else
> + fault = test_for_exception(GP_VECTOR, do_invlpg, ptr);
INVLPG never faults, so don't bother with wrappers. If INVPLG faults, the test
fails, i.e. mission accomplished.
> +
> + report(!fault, "%sINVLPG with tagged addr", fep ? "fep: " : "");
> +}
> +
> +static void do_invpcid(void *desc)
> +{
> + struct invpcid_desc *desc_ptr = (struct invpcid_desc *)desc;
> +
> + asm volatile("invpcid %0, %1" :
> + : "m" (*desc_ptr), "r" (0UL)
> + : "memory");
> +}
Similar thing here, invpcid() belongs in processor.h, alongside invpcid_safe().
> +/* LAM doesn't apply to the linear address in the descriptor of invpcid */
> +static void test_invpcid(u64 flags, u64 lam_mask, void *data)
> +{
> + /*
> + * Reuse the memory address for the descriptor since stack memory
> + * address in KUT doesn't follow the kernel address space partitions.
> + */
> + struct invpcid_desc *desc_ptr = (struct invpcid_desc *)data;
> + bool lam_active = !!(flags & FLAGS_LAM_ACTIVE);
> + bool fault;
> +
> + if (!this_cpu_has(X86_FEATURE_PCID) ||
I don't _think_ we need to check for PCID support. It's a KVM/QEMU bug if INVPCID
is advertised but it doesn't work for the "all PCIDs" flavor.
> + !this_cpu_has(X86_FEATURE_INVPCID)) {
> + report_skip("INVPCID not supported");
> + return;
> + }
> +
...
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 3fe59449..224df45b 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -491,3 +491,13 @@ file = cet.flat
> arch = x86_64
> smp = 2
> extra_params = -enable-kvm -m 2048 -cpu host
> +
> +[intel-lam]
> +file = lam.flat
> +arch = x86_64
> +extra_params = -enable-kvm -cpu host
> +
> +[intel-no-lam]
> +file = lam.flat
> +arch = x86_64
> +extra_params = -enable-kvm -cpu host,-lam
Hrm, not something that needs to be solved now, but we really need a better
interface for iterating over features in tests :-/
