[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access
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 <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 ) > { > 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 <rcojocaru@xxxxxxxxxxxxxxx> Cheers, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |