| Message ID | 20190507151458.29350-7-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm. | expand |
On Tue, 7 May 2019, Julien Grall wrote: > Convert online_page, offline_page and query_page_offline to use > typesafe MFN. I would like to have a statement in the commit message mentioning the changes below to mci_action_add_pageoffline and mc_memerr_dhandler. From an ARM point of view, it is fine. > Note, for clarity, the words have been re-ordered in the error message > updated by this patch. > > No functional changes. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Acked-by: Jan Beulich <jbeulich@suse.com> > > --- > Changes: > - Update error message > - Add Jan's acked-by > --- > xen/arch/x86/cpu/mcheck/mcaction.c | 18 ++++++++++-------- > xen/common/page_alloc.c | 24 ++++++++++++------------ > xen/common/sysctl.c | 14 +++++++------- > xen/include/xen/mm.h | 6 +++--- > 4 files changed, 32 insertions(+), 30 deletions(-) > > diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c > index e42267414e..69332fb84d 100644 > --- a/xen/arch/x86/cpu/mcheck/mcaction.c > +++ b/xen/arch/x86/cpu/mcheck/mcaction.c > @@ -6,7 +6,7 @@ > > static struct mcinfo_recovery * > mci_action_add_pageoffline(int bank, struct mc_info *mi, > - uint64_t mfn, uint32_t status) > + mfn_t mfn, uint32_t status) > { > struct mcinfo_recovery *rec; > > @@ -22,7 +22,7 @@ mci_action_add_pageoffline(int bank, struct mc_info *mi, > > rec->mc_bank = bank; > rec->action_types = MC_ACTION_PAGE_OFFLINE; > - rec->action_info.page_retire.mfn = mfn; > + rec->action_info.page_retire.mfn = mfn_x(mfn); > rec->action_info.page_retire.status = status; > return rec; > } > @@ -42,7 +42,8 @@ mc_memerr_dhandler(struct mca_binfo *binfo, > struct mcinfo_bank *bank = binfo->mib; > struct mcinfo_global *global = binfo->mig; > struct domain *d; > - unsigned long mfn, gfn; > + mfn_t mfn; > + unsigned long gfn; > uint32_t status; > int vmce_vcpuid; > unsigned int mc_vcpuid; > @@ -54,11 +55,12 @@ mc_memerr_dhandler(struct mca_binfo *binfo, > return; > } > > - mfn = bank->mc_addr >> PAGE_SHIFT; > + mfn = maddr_to_mfn(bank->mc_addr); > if ( offline_page(mfn, 1, &status) ) > { > dprintk(XENLOG_WARNING, > - "Failed to offline page %lx for MCE error\n", mfn); > + "Failed to offline page %"PRI_mfn" for MCE error\n", > + mfn_x(mfn)); > return; > } > > @@ -89,10 +91,10 @@ mc_memerr_dhandler(struct mca_binfo *binfo, > ASSERT(d); > gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT); > > - if ( unmmap_broken_page(d, _mfn(mfn), gfn) ) > + if ( unmmap_broken_page(d, mfn, gfn) ) > { > - printk("Unmap broken memory %lx for DOM%d failed\n", > - mfn, d->domain_id); > + printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n", > + mfn_x(mfn), d->domain_id); > goto vmce_failed; > } > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index be44158033..f445f7daec 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1568,23 +1568,23 @@ static int reserve_heap_page(struct page_info *pg) > > } > > -int offline_page(unsigned long mfn, int broken, uint32_t *status) > +int offline_page(mfn_t mfn, int broken, uint32_t *status) > { > unsigned long old_info = 0; > struct domain *owner; > struct page_info *pg; > > - if ( !mfn_valid(_mfn(mfn)) ) > + if ( !mfn_valid(mfn) ) > { > dprintk(XENLOG_WARNING, > - "try to offline page out of range %lx\n", mfn); > + "try to offline out of range page %"PRI_mfn"\n", mfn_x(mfn)); > return -EINVAL; > } > > *status = 0; > - pg = mfn_to_page(_mfn(mfn)); > + pg = mfn_to_page(mfn); > > - if ( is_xen_fixed_mfn(mfn) ) > + if ( is_xen_fixed_mfn(mfn_x(mfn)) ) > { > *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED | > (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT); > @@ -1595,7 +1595,7 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status) > * N.B. xen's txt in x86_64 is marked reserved and handled already. > * Also kexec range is reserved. > */ > - if ( !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) ) > + if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) ) > { > *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM; > return -EINVAL; > @@ -1677,19 +1677,19 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status) > * The caller should make sure end_pfn <= max_page, > * if not, expand_pages() should be called prior to online_page(). > */ > -unsigned int online_page(unsigned long mfn, uint32_t *status) > +unsigned int online_page(mfn_t mfn, uint32_t *status) > { > unsigned long x, nx, y; > struct page_info *pg; > int ret; > > - if ( !mfn_valid(_mfn(mfn)) ) > + if ( !mfn_valid(mfn) ) > { > dprintk(XENLOG_WARNING, "call expand_pages() first\n"); > return -EINVAL; > } > > - pg = mfn_to_page(_mfn(mfn)); > + pg = mfn_to_page(mfn); > > spin_lock(&heap_lock); > > @@ -1730,11 +1730,11 @@ unsigned int online_page(unsigned long mfn, uint32_t *status) > return ret; > } > > -int query_page_offline(unsigned long mfn, uint32_t *status) > +int query_page_offline(mfn_t mfn, uint32_t *status) > { > struct page_info *pg; > > - if ( !mfn_valid(_mfn(mfn)) || !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) ) > + if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) ) > { > dprintk(XENLOG_WARNING, "call expand_pages() first\n"); > return -EINVAL; > @@ -1743,7 +1743,7 @@ int query_page_offline(unsigned long mfn, uint32_t *status) > *status = 0; > spin_lock(&heap_lock); > > - pg = mfn_to_page(_mfn(mfn)); > + pg = mfn_to_page(mfn); > > if ( page_state_is(pg, offlining) ) > *status |= PG_OFFLINE_STATUS_OFFLINE_PENDING; > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c > index c0aa6bde4e..ab161793e5 100644 > --- a/xen/common/sysctl.c > +++ b/xen/common/sysctl.c > @@ -186,7 +186,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > case XEN_SYSCTL_page_offline_op: > { > uint32_t *status, *ptr; > - unsigned long pfn; > + mfn_t mfn; > > ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd); > if ( ret ) > @@ -205,21 +205,21 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > memset(status, PG_OFFLINE_INVALID, sizeof(uint32_t) * > (op->u.page_offline.end - op->u.page_offline.start + 1)); > > - for ( pfn = op->u.page_offline.start; > - pfn <= op->u.page_offline.end; > - pfn ++ ) > + for ( mfn = _mfn(op->u.page_offline.start); > + mfn_x(mfn) <= op->u.page_offline.end; > + mfn = mfn_add(mfn, 1) ) > { > switch ( op->u.page_offline.cmd ) > { > /* Shall revert her if failed, or leave caller do it? */ > case sysctl_page_offline: > - ret = offline_page(pfn, 0, ptr++); > + ret = offline_page(mfn, 0, ptr++); > break; > case sysctl_page_online: > - ret = online_page(pfn, ptr++); > + ret = online_page(mfn, ptr++); > break; > case sysctl_query_page_offline: > - ret = query_page_offline(pfn, ptr++); > + ret = query_page_offline(mfn, ptr++); > break; > default: > ret = -EINVAL; > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index e971147234..3ba7168cc9 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -206,9 +206,9 @@ unsigned long avail_domheap_pages(void); > unsigned long avail_node_heap_pages(unsigned int); > #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f)) > #define free_domheap_page(p) (free_domheap_pages(p,0)) > -unsigned int online_page(unsigned long mfn, uint32_t *status); > -int offline_page(unsigned long mfn, int broken, uint32_t *status); > -int query_page_offline(unsigned long mfn, uint32_t *status); > +unsigned int online_page(mfn_t mfn, uint32_t *status); > +int offline_page(mfn_t mfn, int broken, uint32_t *status); > +int query_page_offline(mfn_t mfn, uint32_t *status); > unsigned long total_free_pages(void); > > void heap_init_late(void); > -- > 2.11.0 >
Hi Stefano, On 5/9/19 7:01 PM, Stefano Stabellini wrote: > On Tue, 7 May 2019, Julien Grall wrote: >> Convert online_page, offline_page and query_page_offline to use >> typesafe MFN. > > I would like to have a statement in the commit message mentioning the > changes below to mci_action_add_pageoffline and mc_memerr_dhandler. I would prefer the generic wording: "At the same time, the typesafe is propagated as far as possible without major modifications." > > From an ARM point of view, it is fine. Thank you! Cheers,
diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c index e42267414e..69332fb84d 100644 --- a/xen/arch/x86/cpu/mcheck/mcaction.c +++ b/xen/arch/x86/cpu/mcheck/mcaction.c @@ -6,7 +6,7 @@ static struct mcinfo_recovery * mci_action_add_pageoffline(int bank, struct mc_info *mi, - uint64_t mfn, uint32_t status) + mfn_t mfn, uint32_t status) { struct mcinfo_recovery *rec; @@ -22,7 +22,7 @@ mci_action_add_pageoffline(int bank, struct mc_info *mi, rec->mc_bank = bank; rec->action_types = MC_ACTION_PAGE_OFFLINE; - rec->action_info.page_retire.mfn = mfn; + rec->action_info.page_retire.mfn = mfn_x(mfn); rec->action_info.page_retire.status = status; return rec; } @@ -42,7 +42,8 @@ mc_memerr_dhandler(struct mca_binfo *binfo, struct mcinfo_bank *bank = binfo->mib; struct mcinfo_global *global = binfo->mig; struct domain *d; - unsigned long mfn, gfn; + mfn_t mfn; + unsigned long gfn; uint32_t status; int vmce_vcpuid; unsigned int mc_vcpuid; @@ -54,11 +55,12 @@ mc_memerr_dhandler(struct mca_binfo *binfo, return; } - mfn = bank->mc_addr >> PAGE_SHIFT; + mfn = maddr_to_mfn(bank->mc_addr); if ( offline_page(mfn, 1, &status) ) { dprintk(XENLOG_WARNING, - "Failed to offline page %lx for MCE error\n", mfn); + "Failed to offline page %"PRI_mfn" for MCE error\n", + mfn_x(mfn)); return; } @@ -89,10 +91,10 @@ mc_memerr_dhandler(struct mca_binfo *binfo, ASSERT(d); gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT); - if ( unmmap_broken_page(d, _mfn(mfn), gfn) ) + if ( unmmap_broken_page(d, mfn, gfn) ) { - printk("Unmap broken memory %lx for DOM%d failed\n", - mfn, d->domain_id); + printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n", + mfn_x(mfn), d->domain_id); goto vmce_failed; } diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index be44158033..f445f7daec 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1568,23 +1568,23 @@ static int reserve_heap_page(struct page_info *pg) } -int offline_page(unsigned long mfn, int broken, uint32_t *status) +int offline_page(mfn_t mfn, int broken, uint32_t *status) { unsigned long old_info = 0; struct domain *owner; struct page_info *pg; - if ( !mfn_valid(_mfn(mfn)) ) + if ( !mfn_valid(mfn) ) { dprintk(XENLOG_WARNING, - "try to offline page out of range %lx\n", mfn); + "try to offline out of range page %"PRI_mfn"\n", mfn_x(mfn)); return -EINVAL; } *status = 0; - pg = mfn_to_page(_mfn(mfn)); + pg = mfn_to_page(mfn); - if ( is_xen_fixed_mfn(mfn) ) + if ( is_xen_fixed_mfn(mfn_x(mfn)) ) { *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED | (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT); @@ -1595,7 +1595,7 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status) * N.B. xen's txt in x86_64 is marked reserved and handled already. * Also kexec range is reserved. */ - if ( !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) ) + if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) ) { *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM; return -EINVAL; @@ -1677,19 +1677,19 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status) * The caller should make sure end_pfn <= max_page, * if not, expand_pages() should be called prior to online_page(). */ -unsigned int online_page(unsigned long mfn, uint32_t *status) +unsigned int online_page(mfn_t mfn, uint32_t *status) { unsigned long x, nx, y; struct page_info *pg; int ret; - if ( !mfn_valid(_mfn(mfn)) ) + if ( !mfn_valid(mfn) ) { dprintk(XENLOG_WARNING, "call expand_pages() first\n"); return -EINVAL; } - pg = mfn_to_page(_mfn(mfn)); + pg = mfn_to_page(mfn); spin_lock(&heap_lock); @@ -1730,11 +1730,11 @@ unsigned int online_page(unsigned long mfn, uint32_t *status) return ret; } -int query_page_offline(unsigned long mfn, uint32_t *status) +int query_page_offline(mfn_t mfn, uint32_t *status) { struct page_info *pg; - if ( !mfn_valid(_mfn(mfn)) || !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) ) + if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) ) { dprintk(XENLOG_WARNING, "call expand_pages() first\n"); return -EINVAL; @@ -1743,7 +1743,7 @@ int query_page_offline(unsigned long mfn, uint32_t *status) *status = 0; spin_lock(&heap_lock); - pg = mfn_to_page(_mfn(mfn)); + pg = mfn_to_page(mfn); if ( page_state_is(pg, offlining) ) *status |= PG_OFFLINE_STATUS_OFFLINE_PENDING; diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c index c0aa6bde4e..ab161793e5 100644 --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -186,7 +186,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) case XEN_SYSCTL_page_offline_op: { uint32_t *status, *ptr; - unsigned long pfn; + mfn_t mfn; ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd); if ( ret ) @@ -205,21 +205,21 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) memset(status, PG_OFFLINE_INVALID, sizeof(uint32_t) * (op->u.page_offline.end - op->u.page_offline.start + 1)); - for ( pfn = op->u.page_offline.start; - pfn <= op->u.page_offline.end; - pfn ++ ) + for ( mfn = _mfn(op->u.page_offline.start); + mfn_x(mfn) <= op->u.page_offline.end; + mfn = mfn_add(mfn, 1) ) { switch ( op->u.page_offline.cmd ) { /* Shall revert her if failed, or leave caller do it? */ case sysctl_page_offline: - ret = offline_page(pfn, 0, ptr++); + ret = offline_page(mfn, 0, ptr++); break; case sysctl_page_online: - ret = online_page(pfn, ptr++); + ret = online_page(mfn, ptr++); break; case sysctl_query_page_offline: - ret = query_page_offline(pfn, ptr++); + ret = query_page_offline(mfn, ptr++); break; default: ret = -EINVAL; diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index e971147234..3ba7168cc9 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -206,9 +206,9 @@ unsigned long avail_domheap_pages(void); unsigned long avail_node_heap_pages(unsigned int); #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f)) #define free_domheap_page(p) (free_domheap_pages(p,0)) -unsigned int online_page(unsigned long mfn, uint32_t *status); -int offline_page(unsigned long mfn, int broken, uint32_t *status); -int query_page_offline(unsigned long mfn, uint32_t *status); +unsigned int online_page(mfn_t mfn, uint32_t *status); +int offline_page(mfn_t mfn, int broken, uint32_t *status); +int query_page_offline(mfn_t mfn, uint32_t *status); unsigned long total_free_pages(void); void heap_init_late(void);