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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan



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