Re: [PATCH v2 2/4] x86: Add test case for LAM_SUP

[Chao Gao <chao.gao@intel.com>]

On Sun, Mar 19, 2023 at 04:22:23PM +0800, Binbin Wu wrote:
>+#define LAM57_BITS 6
>+#define LAM48_BITS 15
>+#define LAM57_MASK	GENMASK_ULL(62, 57)
>+#define LAM48_MASK	GENMASK_ULL(62, 48)
>+
>+struct invpcid_desc {
>+    u64 pcid : 12;
>+    u64 rsv  : 52;
>+    u64 addr : 64;

u64 addr;

>+
>+};
>+static int get_sup_lam_bits(void)
>+{
>+	if (this_cpu_has(X86_FEATURE_LA57) && read_cr4() & X86_CR4_LA57)
>+		return LAM57_BITS;
>+	else
>+		return LAM48_BITS;
>+}
>+
>+/* According to LAM mode, set metadata in high bits */
>+static u64 set_metadata(u64 src, unsigned long lam)
>+{
>+	u64 metadata;
>+
>+	switch (lam) {
>+	case LAM57_BITS: /* Set metadata in bits 62:57 */
>+		metadata = (NONCANONICAL & ((1UL << LAM57_BITS) - 1)) << 57;
>+		metadata |= (src & ~(LAM57_MASK));

this can be simplified to

	return (src & ~LAM47_MASK) | (NONCANONICAL & LAM47_MASK);

and you can pass a mask to set_metadata() and set mask to 0 if LAM isn't
enabled. Then set_metadata() can be further simplified to

	return (src & ~mask) | (NONCANONICAL & mask);

>+		break;
>+	case LAM48_BITS: /* Set metadata in bits 62:48 */
>+		metadata = (NONCANONICAL & ((1UL << LAM48_BITS) - 1)) << 48;
>+		metadata |= (src & ~(LAM48_MASK));
>+		break;
>+	default:
>+		metadata = src;
>+		break;
>+	}
>+
>+	return metadata;
>+}
>+
>+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);
>+}
>+
>+static void test_cr4_lam_set_clear(bool lam_enumerated)
>+{
>+	bool fault;
>+
>+	fault = test_for_exception(GP_VECTOR, &cr4_set_lam_sup, NULL);
>+	if (lam_enumerated)
>+		report(!fault && (read_cr4() & X86_CR4_LAM_SUP),
>+		       "Set CR4.LAM_SUP");
>+	else
>+		report(fault, "Set CR4.LAM_SUP causes #GP");
>+
>+	fault = test_for_exception(GP_VECTOR, &cr4_clear_lam_sup, NULL);
>+	report(!fault, "Clear CR4.LAM_SUP");
>+}
>+
>+static void do_strcpy(void *mem)
>+{
>+	strcpy((char *)mem, "LAM SUP Test string.");
>+}
>+
>+static inline uint64_t test_tagged_ptr(uint64_t arg1, uint64_t arg2,
>+	uint64_t arg3, uint64_t arg4)
>+{
>+	bool lam_enumerated = !!arg1;
>+	int lam_bits = (int)arg2;
>+	u64 *ptr = (u64 *)arg3;
>+	bool la_57 = !!arg4;
>+	bool fault;
>+
>+	fault = test_for_exception(GP_VECTOR, do_strcpy, ptr);
>+	report(!fault, "strcpy to untagged addr");
>+
>+	ptr = (u64 *)set_metadata((u64)ptr, lam_bits);
>+	fault = test_for_exception(GP_VECTOR, do_strcpy, ptr);
>+	if (lam_enumerated)
>+		report(!fault, "strcpy to tagged addr");
>+	else
>+		report(fault, "strcpy to tagged addr causes #GP");

...

>+
>+	if (lam_enumerated && (lam_bits==LAM57_BITS) && !la_57) {
>+		ptr = (u64 *)set_metadata((u64)ptr, LAM48_BITS);
>+		fault = test_for_exception(GP_VECTOR, do_strcpy, ptr);
>+		report(fault, "strcpy to non-LAM-canonical addr causes #GP");
>+	}
>+
>+	return 0;
>+}
>+
>+/* Refer to emulator.c */
>+static void do_mov_mmio(void *mem)
>+{
>+	unsigned long t1, t2;
>+
>+	// test mov reg, r/m and mov r/m, reg
>+	t1 = 0x123456789abcdefull & -1ul;
>+	asm volatile("mov %[t1], (%[mem])\n\t"
>+		     "mov (%[mem]), %[t2]"
>+		     : [t2]"=r"(t2)
>+		     : [t1]"r"(t1), [mem]"r"(mem)
>+		     : "memory");
>+}
>+
>+static inline uint64_t test_tagged_mmio_ptr(uint64_t arg1, uint64_t arg2,
>+	uint64_t arg3, uint64_t arg4)
>+{
>+	bool lam_enumerated = !!arg1;
>+	int lam_bits = (int)arg2;
>+	u64 *ptr = (u64 *)arg3;
>+	bool la_57 = !!arg4;
>+	bool fault;
>+
>+	fault = test_for_exception(GP_VECTOR, do_mov_mmio, ptr);
>+	report(!fault, "Access MMIO with untagged addr");
>+
>+	ptr = (u64 *)set_metadata((u64)ptr, lam_bits);
>+	fault = test_for_exception(GP_VECTOR, do_mov_mmio, ptr);
>+	if (lam_enumerated)
>+		report(!fault,  "Access MMIO with tagged addr");
>+	else
>+		report(fault,  "Access MMIO with tagged addr causes #GP");

Maybe make this (and other similar changes) more dense, e.g.,

	report(fault != lam_enumerated, "Access MMIO with tagged addr")

>+	if (lam_enumerated && (lam_bits==LAM57_BITS) && !la_57) {
>+		ptr = (u64 *)set_metadata((u64)ptr, LAM48_BITS);
>+		fault = test_for_exception(GP_VECTOR, do_mov_mmio, ptr);
>+		report(fault,  "Access MMIO with non-LAM-canonical addr"
>+		               " causes #GP");

don't break long strings.

>+	}

please add a comment to explain the intention of this test.

>+
>+	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 */
>+static void test_invlpg(void *va, bool fep)
>+{
>+	bool fault;
>+	u64 *ptr;
>+
>+	ptr = (u64 *)set_metadata((u64)va, get_sup_lam_bits());
>+	if (fep)
>+		fault = test_for_exception(GP_VECTOR, do_invlpg_fep, ptr);
>+	else
>+		fault = test_for_exception(GP_VECTOR, do_invlpg, ptr);
>+
>+	report(!fault, "%sINVLPG with tagged addr", fep?"fep: ":"");
>+}
>+
>+static void do_invpcid(void *desc)
>+{
>+	unsigned long type = 0;
>+	struct invpcid_desc *desc_ptr = (struct invpcid_desc *)desc;
>+
>+	asm volatile("invpcid %0, %1" :
>+	                              : "m" (*desc_ptr), "r" (type)
>+	                              : "memory");
>+}
>+
>+static void test_invpcid(bool lam_enumerated, void *data)
>+{
>+	struct invpcid_desc *desc_ptr = (struct invpcid_desc *) data;
>+	int lam_bits = get_sup_lam_bits();
>+	bool fault;
>+
>+	if (!this_cpu_has(X86_FEATURE_PCID) ||
>+	    !this_cpu_has(X86_FEATURE_INVPCID)) {
>+		report_skip("INVPCID not supported");
>+		return;
>+	}
>+
>+	memset(desc_ptr, 0, sizeof(struct invpcid_desc));
>+	desc_ptr->addr = (u64)data + 16;

why "+16"? looks you try to avoid invalidating mapping for the descriptor itself.

how about using a local invpcid_desc?

	struct invpcid_desc desc = { .addr = data };

>+
>+	fault = test_for_exception(GP_VECTOR, do_invpcid, desc_ptr);
>+	report(!fault, "INVPCID: untagged pointer + untagged addr");
>+
>+	desc_ptr->addr = set_metadata(desc_ptr->addr, lam_bits);
>+	fault = test_for_exception(GP_VECTOR, do_invpcid, desc_ptr);
>+	report(fault, "INVPCID: untagged pointer + tagged addr causes #GP");
>+
>+	desc_ptr->addr = (u64)data + 16;
>+	desc_ptr = (struct invpcid_desc *)set_metadata((u64)desc_ptr, lam_bits);
>+	fault = test_for_exception(GP_VECTOR, do_invpcid, desc_ptr);
>+	if (lam_enumerated && (read_cr4() & X86_CR4_LAM_SUP))
>+		report(!fault, "INVPCID: tagged pointer + untagged addr");
>+	else
>+		report(fault, "INVPCID: tagged pointer + untagged addr"
>+		              " causes #GP");
>+
>+	desc_ptr = (struct invpcid_desc *)data;
>+	desc_ptr->addr = (u64)data + 16;
>+	desc_ptr->addr = set_metadata(desc_ptr->addr, lam_bits);
>+	desc_ptr = (struct invpcid_desc *)set_metadata((u64)desc_ptr, lam_bits);
>+	fault = test_for_exception(GP_VECTOR, do_invpcid, desc_ptr);
>+	report(fault, "INVPCID: tagged pointer + tagged addr causes #GP");
>+}
>+
>+static void test_lam_sup(bool lam_enumerated, bool fep_available)
>+{
>+	void *vaddr, *vaddr_mmio;
>+	phys_addr_t paddr;
>+	bool fault;
>+	bool la_57 = read_cr4() & X86_CR4_LA57;
>+	int lam_bits = get_sup_lam_bits();
>+
>+	vaddr = alloc_vpage();
>+	vaddr_mmio = alloc_vpage();
>+	paddr = virt_to_phys(alloc_page());
>+	install_page(current_page_table(), paddr, vaddr);
>+	install_page(current_page_table(), IORAM_BASE_PHYS, vaddr_mmio);
>+
>+	test_cr4_lam_set_clear(lam_enumerated);
>+
>+	/* Set for the following LAM_SUP tests */
>+	if (lam_enumerated) {
>+		fault = test_for_exception(GP_VECTOR, &cr4_set_lam_sup, NULL);
>+		report(!fault && (read_cr4() & X86_CR4_LAM_SUP),
>+		       "Set CR4.LAM_SUP");
>+	}
>+
>+	test_tagged_ptr(lam_enumerated, lam_bits, (u64)vaddr, la_57);
>+	test_tagged_mmio_ptr(lam_enumerated, lam_bits, (u64)vaddr_mmio, la_57);
>+	test_invlpg(vaddr, false);
>+	test_invpcid(lam_enumerated, vaddr);
>+
>+	if (fep_available)
>+		test_invlpg(vaddr, true);
>+}
>+
>+int main(int ac, char **av)
>+{
>+	bool lam_enumerated;
>+	bool fep_available = is_fep_available();
>+
>+	setup_vm();
>+
>+	lam_enumerated = this_cpu_has(X86_FEATURE_LAM);

has_lam?

>+	if (!lam_enumerated)
>+		report_info("This CPU doesn't support LAM feature\n");
>+	else
>+		report_info("This CPU supports LAM feature\n");
>+
>+	if (!fep_available)
>+		report_skip("Skipping tests the forced emulation, "
>+			    "use kvm.force_emulation_prefix=1 to enable\n");
>+
>+	test_lam_sup(lam_enumerated, fep_available);
>+
>+	return report_summary();
>+}
>diff --git a/x86/unittests.cfg b/x86/unittests.cfg
>index f324e32..34b09eb 100644
>--- a/x86/unittests.cfg
>+++ b/x86/unittests.cfg
>@@ -478,3 +478,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
>-- 
>2.25.1
>