Re: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Henry,

On 17/01/2023 04:10, Henry Wang wrote:
> 
> 
> Hi Michal,
> 
>> -----Original Message-----
>> Subject: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
>>
>> The direct mapped area occupies L0 slots from 256 to 265 (i.e. 10 slots),
>> resulting in 5TB (512GB * 10) of virtual address space. However, due to
>> incorrect slot subtraction (we take 9 slots into account) we set
>> DIRECTMAP_SIZE to 4.5TB instead. Fix it.
>>
>> Fixes: 5263507b1b4a ("xen: arm: Use a direct mapping of RAM on arm64")
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>  xen/arch/arm/include/asm/config.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/include/asm/config.h
>> b/xen/arch/arm/include/asm/config.h
>> index 0fefed1b8aa9..16213c8b677f 100644
>> --- a/xen/arch/arm/include/asm/config.h
>> +++ b/xen/arch/arm/include/asm/config.h
>> @@ -157,7 +157,7 @@
>>  #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>>
>>  #define DIRECTMAP_VIRT_START   SLOT0(256)
>> -#define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (265-256))
>> +#define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (266 - 256))
> 
> From the commit message "L0 slots from 256 to 265 (i.e. 10 slots)", I think
> the actual range is [256, 265] so probably using "(265 - 256 + 1)" here is a
> bit better? It seems to me that the number 266 looks like a magic number
> because 266 is not in the range. But this is my personal taste though and I
> am open to discussion if you or maintainers have other opinions.
I think this is a matter of taste. I prefer it the way it is because at least 
it matches
how x86 defines the DIRECTMAP_SIZE and it also matches the usual way of 
calculating the size
which is subtracting the start address of that region from the start address of 
the next region
(e.g. VMAP_VIRT_SIZE calculation on arm32).

> 
> Maybe we can also putting a comment on top of the macro to explain this
> calculation.
> 
> I did test this patch on FVP using XTP in both arm32 and arm64 execution
> mode, and this patch is good, so:
> 
> Tested-by: Henry Wang <Henry.Wang@xxxxxxx>
Thanks.

> 
> Kind regards,
> Henry

~Michal