Re: [Xen-devel] [PATCH v2 3/3] xen/arm: fix mask calculation in pdx_init_mask

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.19 at 00:47, <sstabellini@xxxxxxxxxx> wrote:
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask)
>      return mask;
>  }
>  
> +/*
> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they
> + * are left uncompressed.
> + */
>  u64 __init pdx_init_mask(u64 base_addr)
>  {
> -    return fill_mask(base_addr - 1);
> +    return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);

Personally I think that despite the surrounding u64 this should be
uint64_t. You could avoid this altogether by using 1ULL.

> @@ -80,6 +84,8 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
>       * This guarantees that page-pointer arithmetic remains valid within
>       * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our
>       * buddy allocator relies on this assumption.
> +     *
> +     * If the logic changes here, we might have to update init_pdx too.
>       */
>      for ( j = MAX_ORDER-1; ; )
>      {

At first I was puzzled by a reference to something that I didn't
think would exist, and I was then assuming you mean
pdx_init_mask(). But then I thought I'd use grep nevertheless,
and found the Arm instance of it (which this patch actually
changes, but I'm rarely looking at the "diff -p" symbols when
context is otherwise obvious). If you make such a reference in
common code (I think I'd prefer if it was simply omitted), then
please name the specific architecture as well, or make otherwise
clear that this isn't a symbol that's common or required to be
supplied by each arch.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel