[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.