Re: [Xen-devel] [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.06.18 at 15:00, <apop@xxxxxxxxxxxxxxx> wrote:
> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>          }
>          break;
>  
> +    case HVMOP_altp2m_get_mem_access:
> +        if ( a.u.mem_access.pad )
> +            rc = -EINVAL;
> +        else
> +        {
> +            xenmem_access_t access;
> +
> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> +                                    a.u.mem_access.view);
> +            if ( !rc )
> +            {
> +                a.u.mem_access.hvmmem_access = access;
> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;

__copy_field_to_guest()? Or wait, no, the function argument is still a
handle of void.

And then - here we are again: Is it reasonable to permit a domain inquiring
for itself?

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -32,17 +32,10 @@
>  
>  #include "mm-locks.h"
>  
> -/*
> - * Get access type for a gfn.
> - * If gfn == INVALID_GFN, gets the default access type.
> - */
> -static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
> -                               xenmem_access_t *access)
> +static int p2m_access_to_xenmem_access(struct p2m_domain *p2m,

This is not even p2m code - why the p2m_ prefix?

> @@ -458,11 +462,41 @@ long p2m_set_mem_access_multi(struct domain *d,
>      return rc;
>  }
>  
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> +                       unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    mfn_t mfn;
> +
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +    else
> +        p2m = host_p2m;
> +
> +    p2m_read_lock(host_p2m);
> +    if (ap2m)

Missing blanks (also below).

> +        p2m_read_lock(ap2m);
> +
> +    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> +
> +    if (ap2m)
> +        p2m_read_unlock(ap2m);
> +    p2m_read_unlock(host_p2m);
> +
> +    if ( mfn_eq(mfn, INVALID_MFN) )
> +        return -ESRCH;
>  
> -    return _p2m_get_mem_access(p2m, gfn, access);
> +    return p2m_access_to_xenmem_access(p2m, a, access);

I'm confused: Why does p2m_get_mem_access() not use its helper
function p2m_get_mem_access() (which you retain) anymore? I
guess the description is a little too terse. It might also have helped
if some of the mechanical preparation steps were broken out.

Jan



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