kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Hou Wenlong <houwenlong.hwl@antgroup.com>,
	Lai Jiangshan <laijs@linux.alibaba.com>
Subject: Re: [kvm-unit-tests PATCH] x86: test combined access
Date: Thu, 3 Jun 2021 18:39:25 +0000	[thread overview]
Message-ID: <YLkh3bQ106M9nV3k@google.com> (raw)
In-Reply-To: <20210603050537.19605-1-jiangshanlai@gmail.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()?


  reply	other threads:[~2021-06-03 18:39 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-03  5:05 Lai Jiangshan
2021-06-03 18:39 ` Sean Christopherson [this message]
2021-06-03 22:58   ` [kvm-unit-tests PATCH V2] x86: Add a test to check effective permissions Lai Jiangshan
2021-06-04 21:40     ` Sean Christopherson
2021-06-06  3:20       ` Lai Jiangshan
2021-06-05 17:49         ` [kvm-unit-tests PATCH V3] " Lai Jiangshan
2021-06-08  0:24           ` Sean Christopherson
2021-06-08 18:49           ` Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YLkh3bQ106M9nV3k@google.com \
    --to=seanjc@google.com \
    --cc=houwenlong.hwl@antgroup.com \
    --cc=jiangshanlai@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=laijs@linux.alibaba.com \
    --cc=pbonzini@redhat.com \
    --subject='Re: [kvm-unit-tests PATCH] x86: test combined access' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).