On Thu, 9 May 2019, Jan Beulich wrote: > >>> On 09.05.19 at 00:47, <sstabell...@kernel.org> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel