Re: [Xen-devel] [PATCH v4 2/2] xen/arm: fix mask calculation in pdx_init_mask

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.19 at 15:20, <julien.grall@xxxxxxx> wrote:
> On 6/17/19 7:50 PM, Stefano Stabellini wrote:
>> The mask calculation in pdx_init_mask is wrong when the first bank
>> starts at address 0x0. The reason is that pdx_init_mask will do '0 - 1'
>> causing an underflow. As a result, the mask becomes 0xffffffffffffffff
>> which is the biggest possible mask and ends up causing a significant
>> memory waste in the frametable size computation.
>> 
>> For instance, on platforms that have a low memory bank starting at 0x0
>> and a high memory bank, the frametable will end up covering all the
>> holes in between.
>> 
>> The purpose of the mask is to be passed as a parameter to
>> pfn_pdx_hole_setup, which based on the mask parameter calculates
>> pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the
>> important masks for frametable initialization later on.
>> 
>> pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB
>> on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER
>> + PAGE_SHIFT) as start address to pdx_init_mask.
>> 
>> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> 
> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> 
> Ideally, I would like an ack from Andrew or Jan.

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
with one more minor remark:

>> --- a/xen/common/pdx.c
>> +++ b/xen/common/pdx.c
>> @@ -50,9 +50,11 @@ static u64 __init fill_mask(u64 mask)
>>       return mask;
>>   }
>>   
>> +/* We don't compress the first MAX_ORDER bit of the addresses. */
>>   uint64_t __init pdx_init_mask(uint64_t base_addr)

I think the comment would benefit from "want to" getting inserted.
And as a nit, it should be "bits", not "bit", plus perhaps "low" instead
of "first".

Jan



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