Re: [Xen-devel] [PATCH v3] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/06/15 16:17, 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'.
>
> 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@xxxxxxxxxx>
> ---
> Changes since v2:
> - Instead of adding start_ prefix on ARM remove it on x86 [Jan Beulich,
>   Ian Campbell, Razvan Cojocaru]
>
> Changes since v1:
> - This patch is a successor of '[PATCH] x86/mm: use existing 'pfn' in
>   p2m_get_mem_access', instead of fixing gfn_lock/gfn_unlock arguments we do
>   s/pfn/gfn/g for both p2m_get_mem_access/p2m_set_mem_access [Andrew Cooper,
>   Jan Beulich]
>
> P.S.
> - The patch was compile-tested on x86 only.
> ---
>  xen/arch/arm/p2m.c           | 12 ++++++------
>  xen/arch/x86/mm/p2m.c        | 24 ++++++++++++------------
>  xen/include/xen/p2m-common.h | 12 ++++++------
>  3 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 903fa3f..54c238c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1709,9 +1709,9 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, 
> const struct npfec npfec)
>  
>  /*
>   * Set access type for a region of pfns.
> - * If start_pfn == -1ul, sets the default access type.
> + * If gfn == -1ul, sets the default access type.
>   */
> -long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
> +long p2m_set_mem_access(struct domain *d, unsigned long gfn, uint32_t nr,
>                          uint32_t start, uint32_t mask, xenmem_access_t 
> access)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> @@ -1752,14 +1752,14 @@ long p2m_set_mem_access(struct domain *d, unsigned 
> long pfn, uint32_t nr,
>      p2m->mem_access_enabled = true;
>  
>      /* If request to set default access. */
> -    if ( pfn == ~0ul )
> +    if ( gfn == ~0ul )

Please here and everywhere else in the patch, use INVALID_GFN instead of
~0 or -1.

With those changes and the comment style from Razvan,

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

However, if you feel up to it, it would be fantastic if you could
substitute unsigned long for gfn_t, per the justification in e758ed1 and
177bd5f.  c/s 24036a5 is an example of a different API I fixed up in a
similar way.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel