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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.18 at 15:48, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -221,12 +221,12 @@ 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.kind == npfec_kind_with_gla )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> -            else if ( npfec.kind == npfec_kind_in_gpt )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>          }
> +
> +        if ( npfec.kind == npfec_kind_with_gla )
> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> +        else if ( npfec.kind == npfec_kind_in_gpt )
> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;

Without explanation in the commit message and without comment
this change is a no-go imo: I consider it at least questionable to
have npfec_kind_with_gla without .gla_valid set to true. Perhaps
it's just a naming issue, but that would then still require half a
sentence to explain.

> @@ -366,8 +366,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>      /* If request to set default access. */
>      if ( gfn_eq(gfn, INVALID_GFN) )
>      {
> -        p2m->default_access = a;
> -        return 0;
> +        if ( (rc = p2m->check_access(a)) == 0 )
> +        {
> +            p2m->default_access = a;
> +            return 0;
> +        }
>      }

This latching into rc makes subsequent code yield inconsistent
behavior.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -667,6 +667,12 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>      return spurious ? (rc >= 0) : (rc > 0);
>  }
>  
> +int ept_check_access(p2m_access_t p2ma)
> +{
> +    /* All access is permitted */
> +    return 0;
> +}

With this I'd rather see the hook omitted here, to avoid the indirect
call. Perhaps you'll want a wrapper around the indirect call, abstracting
away the NULL check for callers.

> +static void p2m_set_access(struct p2m_domain *p2m, unsigned long gfn,
> +                                      p2m_access_t a)
> +{
> +    int rc;
> +
> +    if ( !p2m->mem_access_settings )
> +        return;

No error indication?

> +    if ( p2m_access_rwx == a )
> +    {
> +        radix_tree_delete(p2m->mem_access_settings, gfn);
> +        return;
> +    }

Is it really p2m_access_rwx that you want to special case here, rather
than ->default_access?

> @@ -31,6 +34,18 @@ int arch_monitor_init_domain(struct domain *d)
>      if ( !d->arch.monitor.msr_bitmap )
>          return -ENOMEM;
>  
> +    if ( cpu_has_svm && !p2m->mem_access_settings )
> +    {
> +        p2m->mem_access_settings = xmalloc(struct radix_tree_root);
> +
> +        if( !p2m->mem_access_settings )

Style.

> +        {
> +            xfree(d->arch.monitor.msr_bitmap);
> +            return -ENOMEM;
> +        }
> +        radix_tree_init(p2m->mem_access_settings);
> +    }

What's the SVM connection here? Please don't forget that p2m-pt.c
also serves the shadow case. Perhaps struct p2m_domain should
contain a boolean indicator whether this auxiliary data structure is
needed?

Jan



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