From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access Date: Mon, 29 Jun 2015 13:45:54 +0100 Message-ID: <55913E02.409@citrix.com> References: <1435331830-7704-1-git-send-email-vkuznets@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1435331830-7704-1-git-send-email-vkuznets@redhat.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Vitaly Kuznetsov , xen-devel@lists.xen.org Cc: Keir Fraser , Ian Campbell , Razvan Cojocaru , Tim Deegan , Stefano Stabellini , Jan Beulich , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org 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 > --- > 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 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