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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 3 May 2019, Jan Beulich wrote:
> >>> 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;
> 

Makes sense. I tried it in my setup and it fixes the misbehavior I was
seeing. I am adding this patch to my short series of PDX fixes. I am
adding your signed-off-by to the patch, let me know if it is a problem.

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