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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 9 May 2019, Jan Beulich wrote:
> >>> 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.

I cannot use 1ULL because it would break the build: the reason is that
u64 is defined as unsigned long on some architectures and unsigned long
long on others. The pointers comparison in the max macro will fail to
compile.

I could use uint64_t, that works, but I think is not a good idea to use
potentially different types between the arguments passed to max. If you
still would like me to change (u64)1 to (uint64_t)1 please explain why
in more details.


> > @@ -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.

I'll make it clear

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