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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 5/7/19 10:35 AM, Jan Beulich wrote:
> > > > On 07.05.19 at 10:59, <julien.grall@xxxxxxx> wrote:
> > On 5/7/19 8:40 AM, Jan Beulich wrote:
> > > > > > On 06.05.19 at 17:26, <julien.grall@xxxxxxx> wrote:
> > > > On 5/6/19 10:06 AM, Jan Beulich wrote:
> > > > > > > > On 03.05.19 at 22:50, <sstabellini@xxxxxxxxxx> wrote:
> > > > > > +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
> > > > > PAGE_SIZE << MAX_ORDER?
> > > > Hmmm, I am not entirely convince this will give the correct value
> > > > because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
> > > Oh, indeed, for an abstract 32-bit arch this would de-generate, due
> > > to MAX_ORDER being 20. Nevertheless I think the expression used
> > > invites for "cleaning up" (making the same mistake I've made), and
> > > since this is in Arm-specific code (where MAX_ORDER is 18) I think it
> > > would still be better to use the suggested alternative.
> > The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that
> > PAGE_SIZE should use signed quantities. So I am not sure whether it is
> > safe to switch to UL here.
> It's not (at least when keeping past x86-32 in the picture): Extending
> to unsigned long long works differently when the type is "unsigned long".
> This matters when using things like ~(PAGE_SIZE - 1).
>
> > If we keep PAGE_SIZE as signed quantities, then I would rather not used
> > your suggestion because this may introduce buggy code if MAX_ORDER is
> > ever updated on Arm.
> A BUILD_BUG_ON() could help prevent this.
Good point.

> > > > > I wonder whether pdx_init_mask() itself wouldn't better apply this
> > > > > (lower) cap
> > > > Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
> > > > init mask?
> > > >
> > > > Note that the later will not produce the same behavior as this patch.
> > > Since I'm not sure I understand what you mean with "capping the
> > > init mask", I'm also uncertain what alternative behavior you're
> > > thinking of. What I'm suggesting is
> > >
> > > u64 __init pdx_init_mask(u64 base_addr)
> > > {
> > >       return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
> > > }
> > As I pointed out in the original thread, there are a couple of issues
> > with this suggestion:
> >         1) banks are not ordered on Arm, so the pdx mask may get initialized
> > with a higher bank address preventing the PDX compression to work
> This is orthogonal to my suggestion here. It's up to Arm code to
> call the function with the lowest bank's base address instead of
> the first one. >
> >         2) the PDX will not be able to compress any bits between 0 and the MSB
> > 1' in the base_addr. In other word, for a base address 0x200000000
> > (8GB), the initial mask will be  0x1ffffffff. I am aware of some
> > platforms with some interesting first bank base address (i.e above 4GB).
> Well, we'd been there before: More "interesting" layouts may
> indeed require adjustments to the logic. The particular case
> we've been talking about was there not being _any_ RAM
> below a certain boundary.
Yes this is unrelated to the case Stefano is trying to fix, however 
Stefano & I have also been discussing of other potential issues with PDX.
I would rather try to address the most important/concerning one at the 
same time. Stefano's patch is actually fixing all the known issues with 
PDX on Arm.
> > 2) is not overly critical, but I think 1) should be addressed.
> >
> > At least on Arm, I don't see any real value to initialize the PDX mask
> > with a base address. I would be more keen to implement pdx_init_mask() as:
> >
> > return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);
> But (besides the missing closing parenthese) that's not what x86 wants.
Could you remind me why?

Cheers,

--
Julien Grall

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