[PATCH] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[PATCH] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Andrew Cooper]

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.

Doing this reveals that const-correctness of page_list_{next,prev}() is
suspect, taking a const pointer and returning a non-const one.  It is left
functioning as it did before, with an explicit typecast to remove constness.

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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/include/xen/mm.h | 95 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 74 insertions(+), 21 deletions(-)

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index a795dd6..38e12b7 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_next_entry((struct page_info *)page, list);
+}
+static inline struct page_info *
+page_list_prev(const struct page_info *page,
+               const struct page_list_head *head)
+{
+    return list_prev_entry((struct page_info *)page, 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)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 1817 bytes --]

On 3/5/16 10:52 AM, 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.
> 
> Doing this reveals that const-correctness of page_list_{next,prev}() is
> suspect, taking a const pointer and returning a non-const one.  It is left
> functioning as it did before, with an explicit typecast to remove constness.
> 
> 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>

I had planned on doing something similar but your commit message is much
better than what I would have come up with.

Not sure if people would like Identified-by: Travis CI but it actually
required a patch to be found.
http://lists.xen.org/archives/html/xen-devel/2016-03/msg00735.html which
is waiting to be merged.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Jan Beulich]

>>> On 05.03.16 at 17:52, <andrew.cooper3@citrix.com> wrote:
> Doing this reveals that const-correctness of page_list_{next,prev}() is
> suspect, taking a const pointer and returning a non-const one.  It is left
> functioning as it did before, with an explicit typecast to remove constness.

I don't see anything suspect here: Retrieving the next or prev list
element doesn't alter the current one, so the input pointer can
legitimately be const, while the output one obviously shouldn't be.
Or else why don't you similarly consider page_list_{first,last}()
bogus in that same respect?

> +static inline bool_t
> +page_list_empty(const struct page_list_head *head)
> +{
> +    return list_empty(head);
> +}

While I appreciate the conversion to bool_t, to be fully correct you
either need to use !! here, or switch list_empty() to bool_t too.

> +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_next_entry((struct page_info *)page, list);
> +}
> +static inline struct page_info *
> +page_list_prev(const struct page_info *page,
> +               const struct page_list_head *head)
> +{
> +    return list_prev_entry((struct page_info *)page, list);
> +}

I'd suggest to avoid the explicit casts, by using
list_entry(page->list.next, struct page_info, list) and
list_entry(page->list.prev, struct page_info, list) respectively.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Andrew Cooper]

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.

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>

v2: Remove explicit casts, and missing !! in page_list_empty()
---
 xen/include/xen/mm.h | 95 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 74 insertions(+), 21 deletions(-)

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)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Jan Beulich]

>>> On 07.03.16 at 16:01, <andrew.cooper3@citrix.com> wrote:
> +static inline void
> +page_list_del2(struct page_info *page, struct page_list_head *head1,
> +               struct page_list_head *head2)
> +{
> +    list_del(&page->list);
> +}

The conversion of this in particular causes a build failure on ARM
(which doesn't d->arch.relmem_list, used as the second argument
to the above in page_alloc.c), which required me to remove that
commit again before pushing.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Andrew Cooper]

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(-)

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)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments

[George Dunlap]

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel