[Xen-devel] [PATCH] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access
'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 --- - 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] - I have not tried compiling ARM sources. --- xen/arch/arm/p2m.c | 13 +++-- xen/arch/x86/mm/p2m.c| 24 xen/include/xen/p2m-common.h | 12 ++-- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 903fa3f..eb6c47d 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 start_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 start_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,15 @@ 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 ( start_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(start_gfn + start), + pfn_to_paddr(start_gfn + nr), 0, MATTR_MEM, mask, 0, a); if ( rc < 0 ) return rc; @@ -1769,14 +1770,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..9459fff 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 */ -long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr, +/* Set access type for a region of gfns. + * If start_gfn == -1ul, sets the default access type */ +long p2m_set_mem_access(struct domain *d, unsigned long start_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 ( start_gfn == ~0ul ) { p2m->default_access = a; return 0; } p2m_lock(p2m); -for ( pfn += start; nr > start; ++pfn ) +for ( start_gfn += start; nr > start; ++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, start_gfn, &t, &_a, 0, NULL); +rc = p2m->set_entry(p2m, start_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 */ -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 */ +int p2m_get_mem_access(struct domain *d, unsigned long gfn, xenmem_access_t
Re: [Xen-devel] [PATCH] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access
"Jan Beulich" writes: On 26.05.15 at 15:32, wrote: >> --- 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 start_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 start_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,15 @@ 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 ( start_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(start_gfn + start), > > Particularly due to this expression I'm not really happy about the > start_ prefix that you're adding here, but I'll let the maintainers > of the respective pieces of code decide if they're happy with it. Sorry for the ping but it has been almost one month... -- Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access
On Tue, 2015-06-23 at 18:25 +0200, Vitaly Kuznetsov wrote: > "Jan Beulich" writes: > > On 26.05.15 at 15:32, wrote: > >> --- 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 start_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 start_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,15 @@ 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 ( start_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(start_gfn + start), > > > > Particularly due to this expression I'm not really happy about the > > start_ prefix that you're adding here, but I'll let the maintainers > > of the respective pieces of code decide if they're happy with it. > > Sorry for the ping but it has been almost one month... Sorry, I must have missed this one, pinging was absolutely the right thing to do (after a week or two would have been fine, no need to wait a month). I'm not super keen on the start_ prefix either, but I would prefer consistency between arm and x86 here more than I object to the prefix. IOW my preference would be to drop it everywhere, but if x86 folks prefer to keep it then I don't mind but ARM should keep it too. I've also copied the (new) mem access maintainers in case they have an opinion. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access
On 06/25/2015 01:27 PM, Ian Campbell wrote: > On Tue, 2015-06-23 at 18:25 +0200, Vitaly Kuznetsov wrote: >> "Jan Beulich" writes: >> >> On 26.05.15 at 15:32, wrote: --- 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 start_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 start_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,15 @@ 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 ( start_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(start_gfn + start), >>> >>> Particularly due to this expression I'm not really happy about the >>> start_ prefix that you're adding here, but I'll let the maintainers >>> of the respective pieces of code decide if they're happy with it. >> >> Sorry for the ping but it has been almost one month... > > Sorry, I must have missed this one, pinging was absolutely the right > thing to do (after a week or two would have been fine, no need to wait a > month). > > I'm not super keen on the start_ prefix either, but I would prefer > consistency between arm and x86 here more than I object to the prefix. > IOW my preference would be to drop it everywhere, but if x86 folks > prefer to keep it then I don't mind but ARM should keep it too. > > I've also copied the (new) mem access maintainers in case they have an > opinion. FWIW, I agree with you and Jan, the start_ prefix makes it a bit confusing. And again FWIW, I have no problem with this being changed for both x86 and ARM. Regards, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access
>>> On 26.05.15 at 15:32, wrote: > --- 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 start_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 start_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,15 @@ 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 ( start_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(start_gfn + start), Particularly due to this expression I'm not really happy about the start_ prefix that you're adding here, but I'll let the maintainers of the respective pieces of code decide if they're happy with it. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel