Re: [Xen-devel] [PATCH v2] x86/mm: Add mem access rights to NPT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.06.18 at 17:17, <aisaila@xxxxxxxxxxxxxxx> wrote:
> From: Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>
> 
> This patch adds access rights for the NPT pages. The access rights are
> saved in a radix tree with the root saved in p2m_domain.

Sounds resource intensive. How many nodes would such a radix tree have
on average?

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,6 +221,9 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>          {
>              req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>              req->u.mem_access.gla = gla;
> +        }
> +        if ( npfec.gla_valid || cpu_has_svm )
> +        {
>  
>              if ( npfec.kind == npfec_kind_with_gla )

You leave a bogusly placed blank line. Please put it ahead of the if()
you add.

> @@ -112,8 +117,37 @@ static unsigned long p2m_type_to_flags(const struct 
> p2m_domain *p2m,
>              flags |= _PAGE_PWT;
>              ASSERT(!level);
>          }
> -        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
> +        flags |= P2M_BASE_FLAGS | _PAGE_PCD;
> +        break;
> +    }
> +    switch (access)

Coding style.

> +static void p2m_set_access(struct p2m_domain *p2m, unsigned long gfn,
> +                                      p2m_access_t a)
> +{
> +    int rc;
> +
> +    if ( p2m_access_rwx == a )
> +        radix_tree_delete(&p2m->mem_access_settings, gfn);
> +
> +    rc = radix_tree_insert(&p2m->mem_access_settings, gfn,
> +                           radix_tree_int_to_ptr(a));

Is there an "else" missing above here? Otherwise why would you
delete the node first?

Also the access rights are far fewer bits than there can be stored in a
node. Did you consider clustering several GFNs' access rights into a
single node?

> @@ -750,7 +820,7 @@ p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_,
>       * XXX we will return p2m_invalid for unmapped gfns */
>      *t = p2m_mmio_dm;
>      /* Not implemented except with EPT */
> -    *a = p2m_access_rwx; 
> +    *a = p2m_access_n;

And the comment remains nevertheless?

> @@ -1127,6 +1200,7 @@ void p2m_pt_init(struct p2m_domain *p2m)
>      p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
>      p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
>      p2m->write_p2m_entry = paging_write_p2m_entry;
> +    radix_tree_init(&p2m->mem_access_settings);

While I don't think there's a risk of races, wouldn't it be better anyway
to set up resources before installing hooks / callbacks?

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -675,6 +675,9 @@ void p2m_teardown(struct p2m_domain *p2m)
>  
>      d = p2m->domain;
>  
> +    if ( cpu_has_svm )
> +        radix_tree_destroy(&p2m->mem_access_settings, NULL);

What about shadow mode, or SVM without NPT? The init above is not
conditional upon SVM, nor is any of the manipulation of the radix tree.

> --- a/xen/include/asm-x86/mem_access.h
> +++ b/xen/include/asm-x86/mem_access.h
> @@ -46,7 +46,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
>  /* Sanity check for mem_access hardware support */
>  static inline bool p2m_mem_access_sanity_check(struct domain *d)
>  {
> -    return is_hvm_domain(d) && cpu_has_vmx && hap_enabled(d);
> +    return is_hvm_domain(d) && hap_enabled(d);

Considering this, should initialization and manipulation perhaps become
conditional upon hap_enabled(d) (perhaps implicitly by finding the radix
tree uninitialized)? Or even some more specific predicate, so that the
tree wouldn't be maintained at all unless someone cared about the
access rights?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel