From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v4] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access Date: Thu, 2 Jul 2015 11:48:25 +0100 Message-ID: <559516F9.8050402@eu.citrix.com> References: <1435592708-26456-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: <1435592708-26456-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 , Andrew Cooper , Tim Deegan , Stefano Stabellini , Jan Beulich , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org 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 > --- > 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