Re: [Xen-devel] [PATCH v2 03/30] xen/x86: fix parameters and return value of *_set_allocation functions

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
> Roger Pau Monne
> Sent: 27 September 2016 16:57
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>; boris.ostrovsky@xxxxxxxxxx; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH v2 03/30] xen/x86: fix parameters and return
> value of *_set_allocation functions
> 
> Return should be an int, and the number of pages should be an unsigned
> long.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> ---
>  xen/arch/x86/mm/hap/hap.c       |  6 +++---
>  xen/arch/x86/mm/shadow/common.c |  7 +++----
>  xen/include/asm-x86/domain.h    | 12 ++++++------
>  3 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 3218fa2..b6d2c61 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -325,7 +325,7 @@ static void hap_free_p2m_page(struct domain *d,
> struct page_info *pg)
>  static unsigned int
>  hap_get_allocation(struct domain *d)
>  {
> -    unsigned int pg = d->arch.paging.hap.total_pages
> +    unsigned long pg = d->arch.paging.hap.total_pages
>          + d->arch.paging.hap.p2m_pages;

You are modifying this calculation (by including hap.p2m_pages as well as 
hap.total_pages) so the comment should probably mention this.

> 
>      return ((pg >> (20 - PAGE_SHIFT))
> @@ -334,8 +334,8 @@ hap_get_allocation(struct domain *d)
> 
>  /* Set the pool of pages to the required number of pages.
>   * Returns 0 for success, non-zero for failure. */
> -static unsigned int
> -hap_set_allocation(struct domain *d, unsigned int pages, int *preempted)
> +static int
> +hap_set_allocation(struct domain *d, unsigned long pages, int
> *preempted)
>  {
>      struct page_info *pg;
> 
> diff --git a/xen/arch/x86/mm/shadow/common.c
> b/xen/arch/x86/mm/shadow/common.c
> index 21607bf..d3cc2cc 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1613,9 +1613,8 @@ shadow_free_p2m_page(struct domain *d, struct
> page_info *pg)
>   * Input will be rounded up to at least shadow_min_acceptable_pages(),
>   * plus space for the p2m table.
>   * Returns 0 for success, non-zero for failure. */
> -static unsigned int sh_set_allocation(struct domain *d,
> -                                      unsigned int pages,
> -                                      int *preempted)
> +static int sh_set_allocation(struct domain *d, unsigned long pages,
> +                             int *preempted)
>  {
>      struct page_info *sp;
>      unsigned int lower_bound;
> @@ -1692,7 +1691,7 @@ static unsigned int sh_set_allocation(struct domain
> *d,
>  /* Return the size of the shadow pool, rounded up to the nearest MB */
>  static unsigned int shadow_get_allocation(struct domain *d)
>  {
> -    unsigned int pg = d->arch.paging.shadow.total_pages
> +    unsigned long pg = d->arch.paging.shadow.total_pages
>          + d->arch.paging.shadow.p2m_pages;

Same here.

>      return ((pg >> (20 - PAGE_SHIFT))
>              + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));

OMG. Is there no rounding macro you can use for this?

  Paul

> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 5807a1f..11ac2a5 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -93,9 +93,9 @@ struct shadow_domain {
> 
>      /* Memory allocation */
>      struct page_list_head freelist;
> -    unsigned int      total_pages;  /* number of pages allocated */
> -    unsigned int      free_pages;   /* number of pages on freelists */
> -    unsigned int      p2m_pages;    /* number of pages allocates to p2m */
> +    unsigned long      total_pages;  /* number of pages allocated */
> +    unsigned long      free_pages;   /* number of pages on freelists */
> +    unsigned long      p2m_pages;    /* number of pages allocates to p2m */
> 
>      /* 1-to-1 map for use when HVM vcpus have paging disabled */
>      pagetable_t unpaged_pagetable;
> @@ -155,9 +155,9 @@ struct shadow_vcpu {
>  /************************************************/
>  struct hap_domain {
>      struct page_list_head freelist;
> -    unsigned int      total_pages;  /* number of pages allocated */
> -    unsigned int      free_pages;   /* number of pages on freelists */
> -    unsigned int      p2m_pages;    /* number of pages allocates to p2m */
> +    unsigned long      total_pages;  /* number of pages allocated */
> +    unsigned long      free_pages;   /* number of pages on freelists */
> +    unsigned long      p2m_pages;    /* number of pages allocates to p2m */
>  };
> 
>  /************************************************/
> --
> 2.7.4 (Apple Git-66)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel