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

[Adrian Pop <apop@bitdefender.com>]

On Fri, Jun 29, 2018 at 09:38:58AM -0600, Jan Beulich wrote:
> >>> On 28.06.18 at 15:00, <apop@bitdefender.com> 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.

I'll then leave the __copy_to_guest() in place as it is for now.

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

Yes, this is a questionable aspect of altp2m that warrants further
discussion.  I'll resend a version with the other problems addressed to
have them out of the way.

> > --- 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?

There's indeed no reason for this to have the p2m_ prefix.  Will remove
it.

> > @@ -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).

All right.

> > +        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.

Ok, I'll fix this and attempt to provide more information with the
commit message.

Thank you!

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