| Message ID | 20170327091059.8452-2-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/03/2017 10:10, Wei Liu wrote: > Prefix them with "mm_" and add declarations to asm-x86/mm.h. > > They will be needed when we split PV specific code out of x86/mm.c. > > No functional change. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> I have to admit that I don't understand why they are called {get,put}_pg_owner. Perhaps very historical from Linux? They are nothing to do with pages, and get a reference on the domain. I'd recommend s/pg_owner/domain/ so the function calls actually indicate what object is having the reference taken on it. ~Andrew
>>> On 28.03.17 at 23:11, <andrew.cooper3@citrix.com> wrote: > On 27/03/2017 10:10, Wei Liu wrote: >> Prefix them with "mm_" and add declarations to asm-x86/mm.h. >> >> They will be needed when we split PV specific code out of x86/mm.c. Is that actually the case? They're about PV (target) domains, so I'd kind of expect them to move together with the PV-only code, even if the caller may not be PV. >> No functional change. >> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > I have to admit that I don't understand why they are called > {get,put}_pg_owner. Perhaps very historical from Linux? I don't think any of this code has Linux origin. > They are nothing to do with pages, and get a reference on the domain. Depends on the perspective you take: For all of their callers, they have precisely that meaning. > I'd recommend s/pg_owner/domain/ so the function calls actually indicate > what object is having the reference taken on it. Well, to make clear what uses are legitimate, perhaps s/pg_owner/foreign_domain/ (if you really continue to think these should be renamed in the first place)? Using just "domain" pretty clearly results in too generic names. Perhaps additionally they should be prefixed mm_? Jan
On 29/03/17 10:03, Jan Beulich wrote: >>>> On 28.03.17 at 23:11, <andrew.cooper3@citrix.com> wrote: >> On 27/03/2017 10:10, Wei Liu wrote: >>> Prefix them with "mm_" and add declarations to asm-x86/mm.h. >>> >>> They will be needed when we split PV specific code out of x86/mm.c. > Is that actually the case? They're about PV (target) domains, so > I'd kind of expect them to move together with the PV-only code, > even if the caller may not be PV. > >>> No functional change. >>> >>> Signed-off-by: Wei Liu <wei.liu2@citrix.com> >> I have to admit that I don't understand why they are called >> {get,put}_pg_owner. Perhaps very historical from Linux? > I don't think any of this code has Linux origin. > >> They are nothing to do with pages, and get a reference on the domain. > Depends on the perspective you take: For all of their callers, > they have precisely that meaning. > >> I'd recommend s/pg_owner/domain/ so the function calls actually indicate >> what object is having the reference taken on it. > Well, to make clear what uses are legitimate, perhaps > s/pg_owner/foreign_domain/ (if you really continue to think > these should be renamed in the first place)? Using just "domain" > pretty clearly results in too generic names. Perhaps additionally > they should be prefixed mm_? Sorry - I had intended my suggestion to be in combination with the mm_ prefixes, so mm_{get,put}_domain(). ~Andrew
On Wed, Mar 29, 2017 at 10:10:39AM +0100, Andrew Cooper wrote: > On 29/03/17 10:03, Jan Beulich wrote: > >>>> On 28.03.17 at 23:11, <andrew.cooper3@citrix.com> wrote: > >> On 27/03/2017 10:10, Wei Liu wrote: > >>> Prefix them with "mm_" and add declarations to asm-x86/mm.h. > >>> > >>> They will be needed when we split PV specific code out of x86/mm.c. > > Is that actually the case? They're about PV (target) domains, so > > I'd kind of expect them to move together with the PV-only code, > > even if the caller may not be PV. > > > >>> No functional change. > >>> > >>> Signed-off-by: Wei Liu <wei.liu2@citrix.com> > >> I have to admit that I don't understand why they are called > >> {get,put}_pg_owner. Perhaps very historical from Linux? > > I don't think any of this code has Linux origin. > > > >> They are nothing to do with pages, and get a reference on the domain. > > Depends on the perspective you take: For all of their callers, > > they have precisely that meaning. > > > >> I'd recommend s/pg_owner/domain/ so the function calls actually indicate > >> what object is having the reference taken on it. > > Well, to make clear what uses are legitimate, perhaps > > s/pg_owner/foreign_domain/ (if you really continue to think > > these should be renamed in the first place)? Using just "domain" > > pretty clearly results in too generic names. Perhaps additionally > > they should be prefixed mm_? > > Sorry - I had intended my suggestion to be in combination with the mm_ > prefixes, so mm_{get,put}_domain(). Fine by me. I've always found the original names cryptic. Assuming exporting the functions are still necessary, I will use the suggested names. > > ~Andrew
>>> On 30.03.17 at 12:07, <wei.liu2@citrix.com> wrote: > Assuming exporting the functions are still necessary, I will use the > suggested names. Well, I've asked before: Are they really needed outside of PV-specific code? It would be odd if so ... Jan
On Thu, Mar 30, 2017 at 06:25:33AM -0600, Jan Beulich wrote: > >>> On 30.03.17 at 12:07, <wei.liu2@citrix.com> wrote: > > Assuming exporting the functions are still necessary, I will use the > > suggested names. > > Well, I've asked before: Are they really needed outside of PV-specific > code? It would be odd if so ... > I need to check this later. That's why I said "assuming...". > Jan >
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 4dbd24ff87..c0bdc667bf 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -3025,7 +3025,7 @@ int new_guest_cr3(unsigned long mfn) return rc; } -static struct domain *get_pg_owner(domid_t domid) +struct domain *mm_get_pg_owner(domid_t domid) { struct domain *pg_owner = NULL, *curr = current->domain; @@ -3068,7 +3068,7 @@ static struct domain *get_pg_owner(domid_t domid) return pg_owner; } -static void put_pg_owner(struct domain *pg_owner) +void mm_put_pg_owner(struct domain *pg_owner) { rcu_unlock_domain(pg_owner); } @@ -3153,19 +3153,19 @@ long do_mmuext_op( if ( unlikely(!guest_handle_okay(uops, count)) ) return -EFAULT; - if ( (pg_owner = get_pg_owner(foreigndom)) == NULL ) + if ( (pg_owner = mm_get_pg_owner(foreigndom)) == NULL ) return -ESRCH; if ( !is_pv_domain(pg_owner) ) { - put_pg_owner(pg_owner); + mm_put_pg_owner(pg_owner); return -EINVAL; } rc = xsm_mmuext_op(XSM_TARGET, d, pg_owner); if ( rc ) { - put_pg_owner(pg_owner); + mm_put_pg_owner(pg_owner); return rc; } @@ -3640,7 +3640,7 @@ long do_mmuext_op( MMU_UPDATE_PREEMPTED, null, rc); } - put_pg_owner(pg_owner); + mm_put_pg_owner(pg_owner); perfc_add(num_mmuext_ops, i); @@ -3716,7 +3716,7 @@ long do_mmu_update( } } - if ( (pg_owner = get_pg_owner((uint16_t)foreigndom)) == NULL ) + if ( (pg_owner = mm_get_pg_owner((uint16_t)foreigndom)) == NULL ) { rc = -ESRCH; goto out; @@ -3951,7 +3951,7 @@ long do_mmu_update( MMU_UPDATE_PREEMPTED, null, rc); } - put_pg_owner(pg_owner); + mm_put_pg_owner(pg_owner); domain_mmap_cache_destroy(&mapcache); @@ -4571,12 +4571,12 @@ long do_update_va_mapping_otherdomain(unsigned long va, u64 val64, struct domain *pg_owner; int rc; - if ( (pg_owner = get_pg_owner(domid)) == NULL ) + if ( (pg_owner = mm_get_pg_owner(domid)) == NULL ) return -ESRCH; rc = __do_update_va_mapping(va, val64, flags, pg_owner); - put_pg_owner(pg_owner); + mm_put_pg_owner(pg_owner); return rc; } diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index e22603ce98..e2baffa9a4 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -602,4 +602,8 @@ extern const char zero_page[]; /* Build a 32bit PSE page table using 4MB pages. */ void write_32bit_pse_identmap(uint32_t *l2); +/* Grab the domain lock of target domain after passing checks */ +struct domain *mm_get_pg_owner(domid_t domid); +void mm_put_pg_owner(struct domain *pg_owner); + #endif /* __ASM_X86_MM_H__ */
Prefix them with "mm_" and add declarations to asm-x86/mm.h. They will be needed when we split PV specific code out of x86/mm.c. No functional change. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- xen/arch/x86/mm.c | 20 ++++++++++---------- xen/include/asm-x86/mm.h | 4 ++++ 2 files changed, 14 insertions(+), 10 deletions(-)