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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 7 May 2019, Julien Grall wrote:
> 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.

Fair enough, but I would rather keep the original version because I don't see
how PAGE_SIZE << MAX_ORDER could be an improvement. It is also more
understandable I think.


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

It is not a problem to move the change into pdx_init_mask, and it could
make sense to do so. From init_pdx we'll just pass 0x0 to get the
behavior of the current patch.

I'll send an update

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