Re: [Xen-devel] [PATCH] xen/arm: skip first page when RAM starts at 0x0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.19 at 00:25, <sstabellini@xxxxxxxxxx> wrote:
> All right. Looking at the comment in pfn_pdx_hole_setup, it seems that
> it is intending to skip the first MAX_ORDER bits, but actually it is
> skipping the first MAX_ORDER-1 bits, if my calculations are correct.
> 
> MAX_ORDER is 18 on ARM which correspond to 1GB. With the current
> implementation of pfn_pdx_hole_setup, if I pass a mask corresponding to
> 512MB, I can see "PFN compression on bits 17...19". So the range
> 512MB-1GB gets compressed.
> 
> Shouldn't it be:
> 
> diff --git a/xen/common/pdx.c b/xen/common/pdx.c
> index 50c21b6..b334eb9 100644
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -81,7 +81,7 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
>       * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our
>       * buddy allocator relies on this assumption.
>       */
> -    for ( j = MAX_ORDER-1; ; )
> +    for ( j = MAX_ORDER; ; )
>      {
>          i = find_next_zero_bit(&mask, BITS_PER_LONG, j);
>          j = find_next_bit(&mask, BITS_PER_LONG, i); 

Yes, but. Originally we started from zero here. As a wild guess,
I think Keir may have thought the cpumask_next() way when
putting together bdb5439c3f, where an adjustment by 1 is
needed in the call to find_next_bit(). Hence it probably was
intuitive for him to have the index start at one less. I do think,
however, that with the switch away from zero, things would
better have become

    for ( j = MAX_ORDER - 1; ; )
    {
        i = find_next_zero_bit(&mask, BITS_PER_LONG, j + 1);
        j = find_next_bit(&mask, BITS_PER_LONG, i + 1);

As you can see, using j + 1 when starting from zero wouldn't
really have been correct (albeit we surely didn't expect to
compress on bit zero, so this is merely a moot consideration).

Now there's a possible caveat here: While for symmetry also
using i + 1 in the second call would seem desirable, I'm afraid
it can't be used directly that way, as find_{,next_}zero_bit(),
on x86 at least, assume their last argument to be less than
their middle one. This, in turn, may already be violated in
the general case (now that the function lives in common code):
An architecture with all BITS_PER_LONG+PAGE_SIZE bits usable
as physical address (x86-64 can use only up to 52, but x86-32
can in principle - possibly with some extra conditions like running
on top of a 64-bit hypervisor - use all 44 bits) the first call may
already return BITS_PER_LONG, and hence the second call
might already produce UB. As a result, to fix this other (latent
only afaict) issue at the same time, the code imo ought to
become

    for ( j = MAX_ORDER - 1; ; )
    {
        i = find_next_zero_bit(&mask, BITS_PER_LONG, j + 1);
        if ( i >= BITS_PER_LONG )
            break;
        j = find_next_bit(&mask, BITS_PER_LONG, i + 1);
        if ( j >= BITS_PER_LONG )
            break;

Jan



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