Message ID | 0c61e460-122c-6486-00b7-22a9a24ee1fc@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/mm: address observations made while working on XSA-388 | expand |
On 04.01.2022 10:48, Jan Beulich wrote: > Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty > pointless local variable "p". Make sure the MFN logged in a debugging > error message is actually the offending one. Return negative errno > values rather than -1 (presently no caller really cares, but imo this > should change). Adjust style. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Return -errno. Drop exclamation mark from log message. Ping? > --- a/xen/arch/x86/mm/p2m-pod.c > +++ b/xen/arch/x86/mm/p2m-pod.c > @@ -58,34 +58,27 @@ p2m_pod_cache_add(struct p2m_domain *p2m > unsigned int order) > { > unsigned long i; > - struct page_info *p; > struct domain *d = p2m->domain; > + mfn_t mfn = page_to_mfn(page); > > #ifndef NDEBUG > - mfn_t mfn; > - > - mfn = page_to_mfn(page); > - > /* Check to make sure this is a contiguous region */ > if ( mfn_x(mfn) & ((1UL << order) - 1) ) > { > printk("%s: mfn %lx not aligned order %u! (mask %lx)\n", > __func__, mfn_x(mfn), order, ((1UL << order) - 1)); > - return -1; > + return -EINVAL; > } > > - for ( i = 0; i < 1UL << order ; i++) > + for ( i = 0; i < (1UL << order); i++) > { > - struct domain * od; > + const struct domain *od = page_get_owner(page + i); > > - p = mfn_to_page(mfn_add(mfn, i)); > - od = page_get_owner(p); > if ( od != d ) > { > - printk("%s: mfn %lx expected owner d%d, got owner d%d!\n", > - __func__, mfn_x(mfn), d->domain_id, > - od ? od->domain_id : -1); > - return -1; > + printk("%s: mfn %lx owner: expected %pd, got %pd\n", > + __func__, mfn_x(mfn) + i, d, od); > + return -EACCES; > } > } > #endif > @@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m > * promise to provide zero pages. So we scrub pages before using. > */ > for ( i = 0; i < (1UL << order); i++ ) > - clear_domain_page(mfn_add(page_to_mfn(page), i)); > + clear_domain_page(mfn_add(mfn, i)); > > /* First, take all pages off the domain list */ > lock_page_alloc(p2m); > - for ( i = 0; i < 1UL << order ; i++ ) > - { > - p = page + i; > - page_list_del(p, &d->page_list); > - } > - > + for ( i = 0; i < (1UL << order); i++ ) > + page_list_del(page + i, &d->page_list); > unlock_page_alloc(p2m); > > /* Then add to the appropriate populate-on-demand list. */ > >
On 27.01.2022 16:04, Jan Beulich wrote: > On 04.01.2022 10:48, Jan Beulich wrote: >> Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty >> pointless local variable "p". Make sure the MFN logged in a debugging >> error message is actually the offending one. Return negative errno >> values rather than -1 (presently no caller really cares, but imo this >> should change). Adjust style. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Return -errno. Drop exclamation mark from log message. > > Ping? Another 4 weeks have passed ... Thanks for feedback either way, Jan >> --- a/xen/arch/x86/mm/p2m-pod.c >> +++ b/xen/arch/x86/mm/p2m-pod.c >> @@ -58,34 +58,27 @@ p2m_pod_cache_add(struct p2m_domain *p2m >> unsigned int order) >> { >> unsigned long i; >> - struct page_info *p; >> struct domain *d = p2m->domain; >> + mfn_t mfn = page_to_mfn(page); >> >> #ifndef NDEBUG >> - mfn_t mfn; >> - >> - mfn = page_to_mfn(page); >> - >> /* Check to make sure this is a contiguous region */ >> if ( mfn_x(mfn) & ((1UL << order) - 1) ) >> { >> printk("%s: mfn %lx not aligned order %u! (mask %lx)\n", >> __func__, mfn_x(mfn), order, ((1UL << order) - 1)); >> - return -1; >> + return -EINVAL; >> } >> >> - for ( i = 0; i < 1UL << order ; i++) >> + for ( i = 0; i < (1UL << order); i++) >> { >> - struct domain * od; >> + const struct domain *od = page_get_owner(page + i); >> >> - p = mfn_to_page(mfn_add(mfn, i)); >> - od = page_get_owner(p); >> if ( od != d ) >> { >> - printk("%s: mfn %lx expected owner d%d, got owner d%d!\n", >> - __func__, mfn_x(mfn), d->domain_id, >> - od ? od->domain_id : -1); >> - return -1; >> + printk("%s: mfn %lx owner: expected %pd, got %pd\n", >> + __func__, mfn_x(mfn) + i, d, od); >> + return -EACCES; >> } >> } >> #endif >> @@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m >> * promise to provide zero pages. So we scrub pages before using. >> */ >> for ( i = 0; i < (1UL << order); i++ ) >> - clear_domain_page(mfn_add(page_to_mfn(page), i)); >> + clear_domain_page(mfn_add(mfn, i)); >> >> /* First, take all pages off the domain list */ >> lock_page_alloc(p2m); >> - for ( i = 0; i < 1UL << order ; i++ ) >> - { >> - p = page + i; >> - page_list_del(p, &d->page_list); >> - } >> - >> + for ( i = 0; i < (1UL << order); i++ ) >> + page_list_del(page + i, &d->page_list); >> unlock_page_alloc(p2m); >> >> /* Then add to the appropriate populate-on-demand list. */ >> >> >
On Tue, Jan 4, 2022 at 9:48 AM Jan Beulich <jbeulich@suse.com> wrote: > Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty > pointless local variable "p". Make sure the MFN logged in a debugging > error message is actually the offending one. Return negative errno > values rather than -1 (presently no caller really cares, but imo this > should change). Adjust style. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Sorry, not sure how I managed to miss this: Reviewed-by: George Dunlap <george.dunlap@cloud.com>
--- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -58,34 +58,27 @@ p2m_pod_cache_add(struct p2m_domain *p2m unsigned int order) { unsigned long i; - struct page_info *p; struct domain *d = p2m->domain; + mfn_t mfn = page_to_mfn(page); #ifndef NDEBUG - mfn_t mfn; - - mfn = page_to_mfn(page); - /* Check to make sure this is a contiguous region */ if ( mfn_x(mfn) & ((1UL << order) - 1) ) { printk("%s: mfn %lx not aligned order %u! (mask %lx)\n", __func__, mfn_x(mfn), order, ((1UL << order) - 1)); - return -1; + return -EINVAL; } - for ( i = 0; i < 1UL << order ; i++) + for ( i = 0; i < (1UL << order); i++) { - struct domain * od; + const struct domain *od = page_get_owner(page + i); - p = mfn_to_page(mfn_add(mfn, i)); - od = page_get_owner(p); if ( od != d ) { - printk("%s: mfn %lx expected owner d%d, got owner d%d!\n", - __func__, mfn_x(mfn), d->domain_id, - od ? od->domain_id : -1); - return -1; + printk("%s: mfn %lx owner: expected %pd, got %pd\n", + __func__, mfn_x(mfn) + i, d, od); + return -EACCES; } } #endif @@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m * promise to provide zero pages. So we scrub pages before using. */ for ( i = 0; i < (1UL << order); i++ ) - clear_domain_page(mfn_add(page_to_mfn(page), i)); + clear_domain_page(mfn_add(mfn, i)); /* First, take all pages off the domain list */ lock_page_alloc(p2m); - for ( i = 0; i < 1UL << order ; i++ ) - { - p = page + i; - page_list_del(p, &d->page_list); - } - + for ( i = 0; i < (1UL << order); i++ ) + page_list_del(page + i, &d->page_list); unlock_page_alloc(p2m); /* Then add to the appropriate populate-on-demand list. */
Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty pointless local variable "p". Make sure the MFN logged in a debugging error message is actually the offending one. Return negative errno values rather than -1 (presently no caller really cares, but imo this should change). Adjust style. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Return -errno. Drop exclamation mark from log message.