From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH v3] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access Date: Fri, 26 Jun 2015 18:46:45 +0300 Message-ID: <558D73E5.2090203@bitdefender.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 , Andrew Cooper , Tim Deegan , Stefano Stabellini , Jan Beulich , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org On 06/26/2015 06:17 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'. > > 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 ) > { > p2m->default_access = a; > return 0; > } > > rc = apply_p2m_changes(d, MEMACCESS, > - pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr), > + pfn_to_paddr(gfn + start), pfn_to_paddr(gfn + nr), > 0, MATTR_MEM, mask, 0, a); > if ( rc < 0 ) > return rc; > @@ -1769,14 +1769,14 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr, > return 0; > } > > -int p2m_get_mem_access(struct domain *d, unsigned long gpfn, > +int p2m_get_mem_access(struct domain *d, unsigned long gfn, > xenmem_access_t *access) > { > int ret; > struct p2m_domain *p2m = p2m_get_hostp2m(d); > > spin_lock(&p2m->lock); > - ret = __p2m_get_mem_access(d, gpfn, access); > + ret = __p2m_get_mem_access(d, gfn, access); > spin_unlock(&p2m->lock); > > return ret; > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 1fd1194..0d01f89 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1600,9 +1600,9 @@ 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 */ This multiline comment does not follow the current coding style (please see the first multiline comment of the patch for an example of proper style: sentences end with '.', and lines with text never contain /* or */). This is of course not something you've introduced, but it's something that can be corrected while passing through there. Though, if nobody else minds I wouldn't bump the patch version over it. > -long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr, > +/* Set access type for a region of gfns. > + * If gfn == -1ul, sets the default access type */ Comment coding style. > +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); > @@ -1639,17 +1639,17 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr, > } > > /* If request to set default access */ > - if ( pfn == ~0ul ) > + if ( gfn == ~0ul ) > { > p2m->default_access = a; > return 0; > } > > p2m_lock(p2m); > - for ( pfn += start; nr > start; ++pfn ) > + for ( gfn += start; nr > start; ++gfn ) > { > - mfn = p2m->get_entry(p2m, pfn, &t, &_a, 0, NULL); > - rc = p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a); > + mfn = p2m->get_entry(p2m, gfn, &t, &_a, 0, NULL); > + rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a); > if ( rc ) > break; > > @@ -1664,9 +1664,9 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr, > return rc; > } > > -/* Get access type for a pfn > - * If pfn == -1ul, gets the default access type */ Comment coding style. > -int p2m_get_mem_access(struct domain *d, unsigned long pfn, > +/* Get access type for a gfn > + * If gfn == -1ul, gets the default access type */ Comment coding style. > +int p2m_get_mem_access(struct domain *d, unsigned long gfn, > xenmem_access_t *access) > { > struct p2m_domain *p2m = p2m_get_hostp2m(d); > @@ -1690,14 +1690,14 @@ int p2m_get_mem_access(struct domain *d, unsigned long pfn, > }; > > /* If request to get default access */ > - if ( pfn == ~0ull ) > + if ( gfn == ~0ull ) > { > *access = memaccess[p2m->default_access]; > return 0; > } > > gfn_lock(p2m, gfn, 0); > - mfn = p2m->get_entry(p2m, pfn, &t, &a, 0, NULL); > + mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL); > gfn_unlock(p2m, gfn, 0); > > if ( mfn_x(mfn) == INVALID_MFN ) > diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h > index bd36826..1e7e583 100644 > --- a/xen/include/xen/p2m-common.h > +++ b/xen/include/xen/p2m-common.h > @@ -45,17 +45,17 @@ int unmap_mmio_regions(struct domain *d, > unsigned long mfn); > > /* > - * Set access type for a region of pfns. > - * If start_pfn == -1ul, sets the default access type. > + * Set access type for a region of gfns. > + * If gfn == -1ul, sets the default access type. > */ > -long p2m_set_mem_access(struct domain *d, unsigned long start_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); > > /* > - * Get access type for a pfn. > - * If pfn == -1ul, gets the default access type. > + * Get access type for a gfn. > + * If gfn == -1ul, gets the default access type. > */ > -int p2m_get_mem_access(struct domain *d, unsigned long pfn, > +int p2m_get_mem_access(struct domain *d, unsigned long gfn, > xenmem_access_t *access); > > #endif /* _XEN_P2M_COMMON_H */ Other than the multiline comments coding style, Reviewed-by: Razvan Cojocaru Cheers, Razvan