Re: [PATCH v4] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[George Dunlap <george.dunlap@eu.citrix.com>]

On 06/29/2015 04:45 PM, Vitaly Kuznetsov wrote:
> 'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as input.
> 
> On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
> declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.
> 
> On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
> declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
> 'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.
> 
> Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on ARM)
> interfaces to using gft_t instead of unsigned long and update all users of
> these functions.
> 
> There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed to
> gfn_lock/gfn_unlock is not defined. This code compiles only because of a
> coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
> second argument.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> Changes since v3:
> - Comment codying style fix [Razvan Cojocaru]
> - Use INVALID_GFN instead of ~0 and -1 [Andrew Cooper]
> - Convert p2m_get_mem_access/p2m_set_mem_access interfaces to using gfn_t
>   [Andrew Cooper]

But you missed a change...

> @@ -1600,9 +1600,11 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      return (p2ma == p2m_access_n2rwx);
>  }
>  
> -/* Set access type for a region of pfns.
> - * If start_pfn == -1ul, sets the default access type */
> -long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
> +/*
> + * Set access type for a region of gfns.
> + * If gfn == INVALID_GFN, sets the default access type.
> + */
> +long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>                          uint32_t start, uint32_t mask, xenmem_access_t access)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> @@ -1638,18 +1640,19 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
>          return -EINVAL;
>      }
>  
> -    /* If request to set default access */
> -    if ( pfn == ~0ul )
> +    /* If request to set default access. */
> +    if ( gfn_x(gfn) == INVALID_GFN )
>      {
>          p2m->default_access = a;
>          return 0;
>      }
>  
>      p2m_lock(p2m);
> -    for ( pfn += start; nr > start; ++pfn )
> +    for ( gfn = _gfn(gfn_x(gfn) + start); nr > start;
> +          gfn = _gfn(gfn_x(gfn) + 1) )

Now in p2m_set_mem_access(), rather than just using an unsigned long in
the loop iterating over gfns, you do this thing where you convert gfn_t
to unsigned long, add one, and then convert it back to gfn_t again.

I can't see any comments in v3 that suggest you doing that, and it seems
a bit clunky.  Is that really necessary?  Wouldn't it be better to
declare a local variable?

I'm not strongly opinionated on this one, it just seems a bit strange.

Everything else looks good, thanks.

 -George