| Message ID | 1457374376-30786-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 07.03.16 at 19:12, <andrew.cooper3@citrix.com> wrote: > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -65,6 +65,7 @@ struct arch_domain > RELMEM_mapping, > RELMEM_done, > } relmem; > + struct page_list_head relmem_list; Well, if I was ARM maintainer I would say no to this otherwise pointless addition (even more so that this list doesn't get initialized anywhere). The expectation I had for how the build issue would be fixed was to simply not convert (at least) page_list_del2() to an inline function. Jan
On 08/03/16 09:57, Jan Beulich wrote: >>>> On 07.03.16 at 19:12, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -65,6 +65,7 @@ struct arch_domain >> RELMEM_mapping, >> RELMEM_done, >> } relmem; >> + struct page_list_head relmem_list; > Well, if I was ARM maintainer I would say no to this otherwise pointless > addition (even more so that this list doesn't get initialized anywhere). > The expectation I had for how the build issue would be fixed was to > simply not convert (at least) page_list_del2() to an inline function. No. Discarding parameters is what got us into the first mess. I will not propagate the problem. It is a bug that ARM relied on the discarded parameters to compile. Ultimately, the bug is that common/page_alloc.c references d->arch. More generally, the problem is that common/page_alloc.c has x86 specifics in it. The two options are to make relmem_list common, or to remove x86 specifics from common by introduce arch_free_domheap_page() helpers which maintain relmem_list on x86. On second thoughts, this latter option seems to be better, as it would also allow the removal of page_list_del2, although it is not clear if this is safe to do. ~Andrew
>>> On 08.03.16 at 12:05, <andrew.cooper3@citrix.com> wrote: > The two options are to make relmem_list common, or to remove x86 > specifics from common by introduce arch_free_domheap_page() helpers > which maintain relmem_list on x86. On second thoughts, this latter > option seems to be better, as it would also allow the removal of > page_list_del2, although it is not clear if this is safe to do. Introducing arch_free_domheap_page() would definitely be fine with me. Removing page_list_del2() altogether, otoh, would not be safe. Jan
On 07/03/16 18:12, Andrew Cooper wrote: > If an architecture does not provide a custom page_list_entry, default > page_list_* helpers are provided, wrapping list_head as an underlying type for > page_list_head. > > The two declarations of the page_list_* helpers differ between defines and > static inline functions, where the defines discard some of their parameters. > > This causes a compilation failure if CONFIG_BIGMEM and debug=n in p2m-pod.c: > > p2m-pod.c: In function ‘p2m_pod_cache_add’: > p2m-pod.c:72:20: error: unused variable ‘d’ [-Werror=unused-variable] > struct domain *d = p2m->domain; > ^ > cc1: all warnings being treated as errors > > because the use of d outside of the !NDEBUG section doesn't get evaluated as a > parameter by page_list_del(). > > Fix this by turning all #defines into static inline functions, so all > parameters are evaluated even if they are not used. > > This reveals a build issue on ARM. page_alloc.c references > d->arch.relmem_list in the previously-discarded parameter. Fix this by > introducing relmem_list for ARM (currently unused). > > While editing this area, correct the return type of page_list_empty from int > to bool_t. > > Reported-by: Doug Goldstein <cardoe@cardoe.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Doug Goldstein <cardoe@cardoe.com> FWIW this patch is fine with me as it is: Acked-by: George Dunlap <george.dunlap@citrix.com> But properly fixing the common code not to reference an arch-specific data structure would be even better, if you've got the time and motivation to do it.
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index c35ed40..c274547 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -65,6 +65,7 @@ struct arch_domain RELMEM_mapping, RELMEM_done, } relmem; + struct page_list_head relmem_list; /* Virtual CPUID */ uint32_t vpidr; diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index a795dd6..8600cf6 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -220,7 +220,7 @@ struct page_list_head # define INIT_PAGE_LIST_HEAD(head) ((head)->tail = (head)->next = NULL) # define INIT_PAGE_LIST_ENTRY(ent) ((ent)->prev = (ent)->next = PAGE_LIST_NULL) -static inline int +static inline bool_t page_list_empty(const struct page_list_head *head) { return !head->next; @@ -392,31 +392,84 @@ page_list_splice(struct page_list_head *list, struct page_list_head *head) # define PAGE_LIST_HEAD LIST_HEAD # define INIT_PAGE_LIST_HEAD INIT_LIST_HEAD # define INIT_PAGE_LIST_ENTRY INIT_LIST_HEAD -# define page_list_empty list_empty -# define page_list_first(hd) \ - list_first_entry(hd, struct page_info, list) -# define page_list_last(hd) \ - list_last_entry(hd, struct page_info, list) -# define page_list_next(pg, hd) list_next_entry(pg, list) -# define page_list_prev(pg, hd) list_prev_entry(pg, list) -# define page_list_add(pg, hd) list_add(&(pg)->list, hd) -# define page_list_add_tail(pg, hd) list_add_tail(&(pg)->list, hd) -# define page_list_del(pg, hd) list_del(&(pg)->list) -# define page_list_del2(pg, hd1, hd2) list_del(&(pg)->list) -# define page_list_remove_head(hd) (!page_list_empty(hd) ? \ - ({ \ - struct page_info *__pg = page_list_first(hd); \ - list_del(&__pg->list); \ - __pg; \ - }) : NULL) -# define page_list_move(dst, src) (!list_empty(src) ? \ - list_replace_init(src, dst) : (void)0) + +static inline bool_t +page_list_empty(const struct page_list_head *head) +{ + return !!list_empty(head); +} +static inline struct page_info * +page_list_first(const struct page_list_head *head) +{ + return list_first_entry(head, struct page_info, list); +} +static inline struct page_info * +page_list_last(const struct page_list_head *head) +{ + return list_last_entry(head, struct page_info, list); +} +static inline struct page_info * +page_list_next(const struct page_info *page, + const struct page_list_head *head) +{ + return list_entry(page->list.next, struct page_info, list); +} +static inline struct page_info * +page_list_prev(const struct page_info *page, + const struct page_list_head *head) +{ + return list_entry(page->list.prev, struct page_info, list); +} +static inline void +page_list_add(struct page_info *page, struct page_list_head *head) +{ + list_add(&page->list, head); +} +static inline void +page_list_add_tail(struct page_info *page, struct page_list_head *head) +{ + list_add_tail(&page->list, head); +} +static inline void +page_list_del(struct page_info *page, struct page_list_head *head) +{ + list_del(&page->list); +} +static inline void +page_list_del2(struct page_info *page, struct page_list_head *head1, + struct page_list_head *head2) +{ + list_del(&page->list); +} +static inline struct page_info * +page_list_remove_head(struct page_list_head *head) +{ + struct page_info *pg; + + if ( page_list_empty(head) ) + return NULL; + + pg = page_list_first(head); + list_del(&pg->list); + return pg; +} +static inline void +page_list_move(struct page_list_head *dst, struct page_list_head *src) +{ + if ( !list_empty(src) ) + list_replace_init(src, dst); +} +static inline void +page_list_splice(struct page_list_head *list, struct page_list_head *head) +{ + list_splice(list, head); +} + # define page_list_for_each(pos, head) list_for_each_entry(pos, head, list) # define page_list_for_each_safe(pos, tmp, head) \ list_for_each_entry_safe(pos, tmp, head, list) # define page_list_for_each_safe_reverse(pos, tmp, head) \ list_for_each_entry_safe_reverse(pos, tmp, head, list) -# define page_list_splice(list, hd) list_splice(list, hd) #endif static inline unsigned int get_order_from_bytes(paddr_t size)
If an architecture does not provide a custom page_list_entry, default page_list_* helpers are provided, wrapping list_head as an underlying type for page_list_head. The two declarations of the page_list_* helpers differ between defines and static inline functions, where the defines discard some of their parameters. This causes a compilation failure if CONFIG_BIGMEM and debug=n in p2m-pod.c: p2m-pod.c: In function ‘p2m_pod_cache_add’: p2m-pod.c:72:20: error: unused variable ‘d’ [-Werror=unused-variable] struct domain *d = p2m->domain; ^ cc1: all warnings being treated as errors because the use of d outside of the !NDEBUG section doesn't get evaluated as a parameter by page_list_del(). Fix this by turning all #defines into static inline functions, so all parameters are evaluated even if they are not used. This reveals a build issue on ARM. page_alloc.c references d->arch.relmem_list in the previously-discarded parameter. Fix this by introducing relmem_list for ARM (currently unused). While editing this area, correct the return type of page_list_empty from int to bool_t. Reported-by: Doug Goldstein <cardoe@cardoe.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Doug Goldstein <cardoe@cardoe.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Stefano Stabellini <stefano.stabellini@citrix.com> CC: Julien Grall <julien.grall@arm.com> v2: * Remove explicit casts, and missing !! in page_list_empty() v3: * Fix build on ARM. As it is referenced from common, relmem_list shouldn't be an .arch variable. However, moving it would split it away from the relmem enumeration, which is specific to the arch. (Basically - I lack sufficient TUITs to disentagle this properly, and this is the most simple fix.) --- xen/include/asm-arm/domain.h | 1 + xen/include/xen/mm.h | 95 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 75 insertions(+), 21 deletions(-)