Re: [kvm-unit-tests PATCH 10/14] x86: Look up the PTEs rather than assuming them

[Babu Moger <babu.moger@amd.com>]

Hi,

This patch caused the regression. Here is the failure. Failure seems to
happen only on 5-level paging. Still investigating.. Let me know if you
have any idea,

#./tests/access
BUILD_HEAD=f3e081d7
timeout -k 1s --foreground 180 /usr/local/bin/qemu-system-x86_64
--no-reboot -nodefaults -device pc-testdev

-device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio
-device pci-testdev -machine accel=kvm

-kernel /tmp/tmp.w1JL6jhyfN -smp 1 -cpu max # -initrd /tmp/tmp.9coF1FJSwD
enabling apic
starting test

starting 5-level paging test.

run
............................FAIL access

=============================

Git bisect log.

 git bisect log
git bisect start
# bad: [68aa4e32f2b717b991e4dce7dfdde2247366abbc] x86: do not run
vmx_pf_exception_test twice
git bisect bad 68aa4e32f2b717b991e4dce7dfdde2247366abbc
# good: [c90c646d7381c99ac7d9d7812bd8535214458978] access: treat NX as
reserved if EFER.NXE=0
git bisect good c90c646d7381c99ac7d9d7812bd8535214458978
# good: [c90c646d7381c99ac7d9d7812bd8535214458978] access: treat NX as
reserved if EFER.NXE=0
git bisect good c90c646d7381c99ac7d9d7812bd8535214458978
# good: [c73cc92d8060dd79b71cbd2ded1454a6e924b771] s390x: uv-host: Fence a
destroy cpu test on z15
git bisect good c73cc92d8060dd79b71cbd2ded1454a6e924b771
# good: [c73cc92d8060dd79b71cbd2ded1454a6e924b771] s390x: uv-host: Fence a
destroy cpu test on z15
git bisect good c73cc92d8060dd79b71cbd2ded1454a6e924b771
# good: [2e88ad238a19253b94e9f410e4c86ed632c134a0] unify field names and
definitions for GDT descriptors
git bisect good 2e88ad238a19253b94e9f410e4c86ed632c134a0
# good: [91abf0b9aa0bac4ca17df0be63871ca6e1562eac] Merge branch
'gdt-idt-cleanup' into master
git bisect good 91abf0b9aa0bac4ca17df0be63871ca6e1562eac
# bad: [0f10d9aea13631a414a3023699dd2dfd47dfd02f] x86: Prepare access test
for running in L2
git bisect bad 0f10d9aea13631a414a3023699dd2dfd47dfd02f
# good: [7a14c1d9468626d4cdd0d883097c7caaa36a91bf] x86: Fix operand size
for lldt
git bisect good 7a14c1d9468626d4cdd0d883097c7caaa36a91bf
# bad: [f3e081d74812ee05be7e744eb8be3f04a2f65c87] x86: Look up the PTEs
rather than assuming them
git bisect bad f3e081d74812ee05be7e744eb8be3f04a2f65c87
# good: [f7599ce50db691c4281479002f03d611927bed1c] x86: Add a regression
test for L1 LDTR persistence bug
git bisect good f7599ce50db691c4281479002f03d611927bed1c
# first bad commit: [f3e081d74812ee05be7e744eb8be3f04a2f65c87] x86: Look
up the PTEs rather than assuming them

Thanks

Babu


On 11/10/21 3:19 PM, Aaron Lewis wrote:
> Rather than assuming which PTEs the SMEP test runs on, look them up to
> ensure they are correct.  If this test were to run on a different page
> table (ie: run in an L2 test) the wrong PTEs would be set.  Switch to
> looking up the PTEs to avoid this from happening.
>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  lib/libcflat.h |  1 +
>  lib/x86/vm.c   | 21 +++++++++++++++++++++
>  lib/x86/vm.h   |  3 +++
>  x86/access.c   | 26 ++++++++++++++++++--------
>  x86/cstart64.S |  1 -
>  x86/flat.lds   |  1 +
>  6 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 9bb7e08..c1fd31f 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -35,6 +35,7 @@
>  #define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
>  #define __ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
>  #define ALIGN(x, a)		__ALIGN((x), (a))
> +#define ALIGN_DOWN(x, a)	__ALIGN((x) - ((a) - 1), (a))
>  #define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
>  
>  #define MIN(a, b)		((a) < (b) ? (a) : (b))
> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> index 5cd2ee4..6a70ef6 100644
> --- a/lib/x86/vm.c
> +++ b/lib/x86/vm.c
> @@ -281,3 +281,24 @@ void force_4k_page(void *addr)
>  	if (pte & PT_PAGE_SIZE_MASK)
>  		split_large_page(ptep, 2);
>  }
> +
> +/*
> + * Call the callback on each page from virt to virt + len.
> + */
> +void walk_pte(void *virt, size_t len, pte_callback_t callback)
> +{
> +    pgd_t *cr3 = current_page_table();
> +    uintptr_t start = (uintptr_t)virt;
> +    uintptr_t end = (uintptr_t)virt + len;
> +    struct pte_search search;
> +    size_t page_size;
> +    uintptr_t curr;
> +
> +    for (curr = start; curr < end; curr = ALIGN_DOWN(curr + page_size, page_size)) {
> +        search = find_pte_level(cr3, (void *)curr, 1);
> +        assert(found_leaf_pte(search));
> +        page_size = 1ul << PGDIR_BITS(search.level);
> +
> +        callback(search, (void *)curr);
> +    }
> +}
> diff --git a/lib/x86/vm.h b/lib/x86/vm.h
> index d9753c3..4c6dff9 100644
> --- a/lib/x86/vm.h
> +++ b/lib/x86/vm.h
> @@ -52,4 +52,7 @@ struct vm_vcpu_info {
>          u64 cr0;
>  };
>  
> +typedef void (*pte_callback_t)(struct pte_search search, void *va);
> +void walk_pte(void *virt, size_t len, pte_callback_t callback);
> +
>  #endif
> diff --git a/x86/access.c b/x86/access.c
> index a781a0c..8e3a718 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -201,10 +201,24 @@ static void set_cr0_wp(int wp)
>      }
>  }
>  
> +static void clear_user_mask(struct pte_search search, void *va)
> +{
> +	*search.pte &= ~PT_USER_MASK;
> +}
> +
> +static void set_user_mask(struct pte_search search, void *va)
> +{
> +	*search.pte |= PT_USER_MASK;
> +
> +	/* Flush to avoid spurious #PF */
> +	invlpg(va);
> +}
> +
>  static unsigned set_cr4_smep(int smep)
>  {
> +    extern char stext, etext;
> +    size_t len = (size_t)&etext - (size_t)&stext;
>      unsigned long cr4 = shadow_cr4;
> -    extern u64 ptl2[];
>      unsigned r;
>  
>      cr4 &= ~CR4_SMEP_MASK;
> @@ -214,14 +228,10 @@ static unsigned set_cr4_smep(int smep)
>          return 0;
>  
>      if (smep)
> -        ptl2[2] &= ~PT_USER_MASK;
> +        walk_pte(&stext, len, clear_user_mask);
>      r = write_cr4_checking(cr4);
> -    if (r || !smep) {
> -        ptl2[2] |= PT_USER_MASK;
> -
> -	/* Flush to avoid spurious #PF */
> -	invlpg((void *)(2 << 21));
> -    }
> +    if (r || !smep)
> +        walk_pte(&stext, len, set_user_mask);
>      if (!r)
>          shadow_cr4 = cr4;
>      return r;
> diff --git a/x86/cstart64.S b/x86/cstart64.S
> index ddb83a0..ff79ae7 100644
> --- a/x86/cstart64.S
> +++ b/x86/cstart64.S
> @@ -17,7 +17,6 @@ stacktop:
>  .data
>  
>  .align 4096
> -.globl ptl2
>  ptl2:
>  i = 0
>  	.rept 512 * 4
> diff --git a/x86/flat.lds b/x86/flat.lds
> index a278b56..337bc44 100644
> --- a/x86/flat.lds
> +++ b/x86/flat.lds
> @@ -3,6 +3,7 @@ SECTIONS
>      . = 4M + SIZEOF_HEADERS;
>      stext = .;
>      .text : { *(.init) *(.text) *(.text.*) }
> +    etext = .;
>      . = ALIGN(4K);
>      .data : {
>            *(.data)

-- 
Thanks
Babu Moger