Re: [kvm-unit-tests PATCH] x86: test combined access

[Sean Christopherson <seanjc@google.com>]

On Thu, Jun 03, 2021, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> Check combined access when guest shares pagetables.

Can we avoid "combined"?  Purely because of Intel using "combined" to refer to
the full IA32+EPT translation.  It'd also be helpful to provide a bit more
detail.  E.g. 

  Add a test to verify that KVM correctly handles the case where two or more
  non-leaf page table entries point at the same table gfn, but with different
  parent access permissions.

> For example, here is a shared pagetable:
>    pgd[]   pud[]        pmd[]            virtual address pointers
>                      /->pmd1(u--)->pte1(uw-)->page1 <- ptr1 (u--)
>         /->pud1(uw-)--->pmd2(uw-)->pte2(uw-)->page2 <- ptr2 (uw-)
>    pgd-|           (shared pmd[] as above)
>         \->pud2(u--)--->pmd1(u--)->pte1(uw-)->page1 <- ptr3 (u--)
>                      \->pmd2(uw-)->pte2(uw-)->page2 <- ptr4 (u--)
>   pud1 and pud2 point to the same pmd table
> 
> The test is usefull when TDP is not enabled.
              ^^^^^^^
	      useful

> Co-Developed-by: Hou Wenlong <houwenlong.hwl@antgroup.com>

Co-developed-by, and this needs Hou Wenlong's SoB as well.

> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>  x86/access.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 93 insertions(+), 6 deletions(-)
> 
> diff --git a/x86/access.c b/x86/access.c
> index 7dc9eb6..6dbe6e5 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -60,6 +60,8 @@ enum {
>      AC_PDE_BIT36_BIT,
>      AC_PDE_BIT13_BIT,
>  
> +    AC_PUDE_NO_WRITABLE_BIT, // special test case to disable write bit on PUD entry

Part of me thinks we should use AC_PDPTE_WRITABLE_BIT to be consistent with the
PDE and PTE bits, but I think I agree that special casing this one-off tests is
cleaner overall.

For better or worse, this test uses x86 legacy paging terminology for the entry
bits, not Linux's generic PTE/PMD/PUD/PGD.  I.e. for consistency, I think it
makes sense to use AC_PDPTE_NO_WRITABLE_BIT.

This new bit also needs an entry in ac_names[], otherwise it'll print garbage on
failure.  I'm thinking:

@@ -134,6 +134,7 @@ const char *ac_names[] = {
     [AC_PDE_BIT51_BIT] = "pde.51",
     [AC_PDE_BIT36_BIT] = "pde.36",
     [AC_PDE_BIT13_BIT] = "pde.13",
+    [AC_PDPTE_NO_WRITABLE_BIT] = "pdpte.ro",
     [AC_PKU_AD_BIT] = "pkru.ad",
     [AC_PKU_WD_BIT] = "pkru.wd",
     [AC_PKU_PKEY_BIT] = "pkey=1",


> +
>      AC_PKU_AD_BIT,
>      AC_PKU_WD_BIT,
>      AC_PKU_PKEY_BIT,
> @@ -97,6 +99,8 @@ enum {
>  #define AC_PDE_BIT36_MASK     (1 << AC_PDE_BIT36_BIT)
>  #define AC_PDE_BIT13_MASK     (1 << AC_PDE_BIT13_BIT)
>  
> +#define AC_PUDE_NO_WRITABLE_MASK  (1 << AC_PUDE_NO_WRITABLE_BIT)
> +
>  #define AC_PKU_AD_MASK        (1 << AC_PKU_AD_BIT)
>  #define AC_PKU_WD_MASK        (1 << AC_PKU_WD_BIT)
>  #define AC_PKU_PKEY_MASK      (1 << AC_PKU_PKEY_BIT)
> @@ -326,6 +330,7 @@ static pt_element_t ac_test_alloc_pt(ac_pool_t *pool)
>  {
>      pt_element_t ret = pool->pt_pool + pool->pt_pool_current;
>      pool->pt_pool_current += PAGE_SIZE;
> +    memset(va(ret), 0, PAGE_SIZE);
>      return ret;
>  }
>  
> @@ -408,7 +413,8 @@ static void ac_emulate_access(ac_test_t *at, unsigned flags)
>  	goto fault;
>      }
>  
> -    writable = F(AC_PDE_WRITABLE);
> +    writable = !F(AC_PUDE_NO_WRITABLE);
> +    writable &= F(AC_PDE_WRITABLE);

These can be combined, e.g.

       writable = !F(AC_PDPTE_NO_WRITABLE) && F(AC_PDE_WRITABLE);

>      user = F(AC_PDE_USER);
>      executable = !F(AC_PDE_NX);
>  
> @@ -471,7 +477,7 @@ static void ac_set_expected_status(ac_test_t *at)
>      ac_emulate_access(at, at->flags);
>  }
>  
> -static void __ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool,
> +static void __ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool, bool reuse,
>  				      u64 pd_page, u64 pt_page)
>  
>  {
> @@ -496,13 +502,26 @@ static void __ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool,
>  	    goto next;
>  	}
>  	skip = false;
> +	if (reuse && vroot[index]) {
> +	    switch (i) {
> +	    case 2:
> +		at->pdep = &vroot[index];
> +		break;
> +	    case 1:
> +		at->ptep = &vroot[index];
> +		break;
> +	    }
> +	    goto next;
> +	}
>  
>  	switch (i) {
>  	case 5:
>  	case 4:
>  	case 3:
> -	    pte = pd_page ? pd_page : ac_test_alloc_pt(pool);
> +	    pte = (i==3 && pd_page) ? pd_page : ac_test_alloc_pt(pool);
>  	    pte |= PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK;
> +	    if (F(AC_PUDE_NO_WRITABLE))
> +		pte &= ~PT_WRITABLE_MASK

This will seemingly clear the WRITABLE bit for PML4 and PML5, but due to reuse
behavior, that may not be reflected in the actual page tables depending on
whether or not the first test clears the writable bit.

For robustness and clarity, I think it'd be better to do:

        case 5:
        case 4:
            pte = ac_test_alloc_pt(pool);
            pte |= PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK;
            break;
        case 3:
            pte = pd_page ? pd_page : ac_test_alloc_pt(pool);
            pte |= PT_PRESENT_MASK | PT_USER_MASK;
            if (!F(AC_PDPTE_NO_WRITABLE))
                pte |= PT_WRITABLE_MASK;
            break;

>  	case 2:
>  	    if (!F(AC_PDE_PSE)) {
> @@ -568,13 +587,13 @@ static void __ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool,
>  
>  static void ac_test_setup_pte(ac_test_t *at, ac_pool_t *pool)
>  {
> -	__ac_setup_specific_pages(at, pool, 0, 0);
> +	__ac_setup_specific_pages(at, pool, false, 0, 0);
>  }
>  
>  static void ac_setup_specific_pages(ac_test_t *at, ac_pool_t *pool,
>  				    u64 pd_page, u64 pt_page)
>  {
> -	return __ac_setup_specific_pages(at, pool, pd_page, pt_page);
> +	return __ac_setup_specific_pages(at, pool, false, pd_page, pt_page);
>  }
>  
>  static void dump_mapping(ac_test_t *at)
> @@ -930,6 +949,73 @@ err:
>  	return 0;
>  }
>  
> +static int check_combined_protection(ac_pool_t *pool)

To avoid the "combined" verbiage, how about check_effective_sp_permissions()?