Re: [PATCH] x86/mem: Make mem_hotadd_check() more legible

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Wed, Jul 19, 2023 at 11:08:08AM +0100, Andrew Cooper wrote:
> Introduce a ROUND() macro to mirror ROUNDUP().  Use both to remove all the
> opencoded rounding in mem_hotadd_check().  Fix other minor style issues.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> The compiled binary is identical.
> ---
>  xen/arch/x86/x86_64/mm.c | 31 +++++++++++++------------------
>  xen/include/xen/macros.h |  1 +
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 60db439af3ec..38f978cab269 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1159,10 +1159,10 @@ static int mem_hotadd_check(unsigned long spfn, 
> unsigned long epfn)
>  {
>      unsigned long s, e, length, sidx, eidx;
>  
> -    if ( (spfn >= epfn) )
> +    if ( spfn >= epfn )
>          return 0;
>  
> -    if (pfn_to_pdx(epfn) > FRAMETABLE_NR)
> +    if ( pfn_to_pdx(epfn) > FRAMETABLE_NR )
>          return 0;
>  
>      if ( (spfn | epfn) & ((1UL << PAGETABLE_ORDER) - 1) )
> @@ -1172,10 +1172,9 @@ static int mem_hotadd_check(unsigned long spfn, 
> unsigned long epfn)
>          return 0;
>  
>      /* Make sure the new range is not present now */
> -    sidx = ((pfn_to_pdx(spfn) + PDX_GROUP_COUNT - 1)  & ~(PDX_GROUP_COUNT - 
> 1))
> -            / PDX_GROUP_COUNT;
> -    eidx = (pfn_to_pdx(epfn - 1) & ~(PDX_GROUP_COUNT - 1)) / PDX_GROUP_COUNT;
> -    if (sidx >= eidx)
> +    sidx = ROUNDUP(pfn_to_pdx(spfn),     PDX_GROUP_COUNT) / PDX_GROUP_COUNT;
> +    eidx = ROUND  (pfn_to_pdx(epfn - 1), PDX_GROUP_COUNT) / PDX_GROUP_COUNT;
See [1] for both the sidx and eidx lines.
> +    if ( sidx >= eidx )
>          return 0;
>  
>      s = find_next_zero_bit(pdx_group_valid, eidx, sidx);
> @@ -1186,28 +1185,24 @@ static int mem_hotadd_check(unsigned long spfn, 
> unsigned long epfn)
>          return 0;
>  
>      /* Caculate at most required m2p/compat m2p/frametable pages */
> -    s = (spfn & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1));
> -    e = (epfn + (1UL << (L2_PAGETABLE_SHIFT - 3)) - 1) &
> -            ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1);
> +    s = ROUND  (spfn, 1UL << (L2_PAGETABLE_SHIFT - 3));
> +    e = ROUNDUP(epfn, 1UL << (L2_PAGETABLE_SHIFT - 3));
>  
>      length = (e - s) * sizeof(unsigned long);
>  
> -    s = (spfn & ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1));
> -    e = (epfn + (1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) &
> -            ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1);
> -
> -    e = min_t(unsigned long, e,
> +    s =     ROUND  (spfn, 1ULL << (L2_PAGETABLE_SHIFT - 2));
See [1] for s.
> +    e = min(ROUNDUP(epfn, 1ULL << (L2_PAGETABLE_SHIFT - 2)),
>              (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2);
>  
>      if ( e > s )
> -        length += (e -s) * sizeof(unsigned int);
> +        length += (e - s) * sizeof(unsigned int);
>  
> -    s = pfn_to_pdx(spfn) & ~(PDX_GROUP_COUNT - 1);
> -    e = ( pfn_to_pdx(epfn) + (PDX_GROUP_COUNT - 1) ) & ~(PDX_GROUP_COUNT - 
> 1);
> +    s = ROUND  (pfn_to_pdx(spfn), PDX_GROUP_COUNT);
See [1] for s.
> +    e = ROUNDUP(pfn_to_pdx(epfn), PDX_GROUP_COUNT);
>  
>      length += (e - s) * sizeof(struct page_info);
>  
> -    if ((length >> PAGE_SHIFT) > (epfn - spfn))
> +    if ( (length >> PAGE_SHIFT) > (epfn - spfn) )
>          return 0;
>  
>      return 1;
> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> index 7b92d345044d..ceeffcaa95ff 100644
> --- a/xen/include/xen/macros.h
> +++ b/xen/include/xen/macros.h
> @@ -1,6 +1,7 @@
>  #ifndef __MACROS_H__
>  #define __MACROS_H__
>  
> +#define ROUND(x, a)   ((x) & ~((a) - 1))
Why not ROUNDDOWN() or ROUND_DOWN()? ROUND() doesn't imply a specific
direction and can be confusing if ROUNDUP is not seen next to it.
>  #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
>  
>  #define IS_ALIGNED(val, align) (!((val) & ((align) - 1)))
> 
> base-commit: b1c16800e52743d9afd9af62c810f03af16dd942
> -- 
> 2.30.2
> 
> 

[1] The hand-crafted alignment there is going to collide with the efforts
to integrate automatic style checkers. It's also not conveying critical
information, so I'd argue for its removal in the spirit of making future
diffs less intrusive.

Alejandro