Re: [kvm-unit-tests v3 3/4] x86: Add test cases for LAM_{U48,U57}

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

On Wed, Apr 12, 2023 at 03:51:33PM +0800, Binbin Wu wrote:
>This unit test covers:
>1. CR3 LAM bits toggles.
>2. Memory/MMIO access with user mode address containing LAM metadata.
>
>Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

two nits below:

>---
>+static void test_lam_user(bool has_lam)
>+{
>+	phys_addr_t paddr;
>+	unsigned long cr3 = read_cr3();
>+
>+	/*
>+	 * The physical address width is within 36 bits, so that using identical
>+	 * mapping, the linear address will be considered as user mode address
>+	 * from the view of LAM.
>+	 */

Why 36 bits (i.e., 64G)?

would you mind adding a comment in patch 2 to explain why the virtual
addresses are kernel mode addresses?

>+	paddr = virt_to_phys(alloc_page());
>+	install_page((void *)cr3, paddr, (void *)paddr);
>+	install_page((void *)cr3, IORAM_BASE_PHYS, (void *)IORAM_BASE_PHYS);

are the two lines necessary?

>+
>+	test_lam_user_mode(has_lam, LAM48_MASK, paddr, IORAM_BASE_PHYS);
>+	test_lam_user_mode(has_lam, LAM57_MASK, paddr, IORAM_BASE_PHYS);
>+}
>+
> int main(int ac, char **av)
> {
> 	bool has_lam;
>@@ -239,6 +309,7 @@ int main(int ac, char **av)
> 			    "use kvm.force_emulation_prefix=1 to enable\n");
> 
> 	test_lam_sup(has_lam, fep_available);
>+	test_lam_user(has_lam);
> 
> 	return report_summary();
> }
>-- 
>2.25.1
>