[PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation

Michal Orzel posted 1 patch 1 year, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230116144106.12544-2-michal.orzel@amd.com
xen/arch/arm/include/asm/config.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Michal Orzel 1 year, 2 months ago
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@amd.com>
---
 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))
 #define DIRECTMAP_VIRT_END     (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
 
 #define XENHEAP_VIRT_START     directmap_virt_start
-- 
2.25.1
Re: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Julien Grall 1 year, 2 months ago
Hi Michal,

It is not clear to me why this was sent In-reply-to the other patch. If 
they are meant to form a series, then this should have a cover letter + 
each patch should be numbered.

If they are truly separate, then please don't thread them.

On 16/01/2023 14:41, Michal Orzel wrote:
> The direct mapped area occupies L0 slots from 256 to 265 (i.e. 10 slots),

I would write "265 included"  or similar so it shows why this is a problem.

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

I would clarify that we only support up to 2TB. So this is a latent 
issue. This would make clear that...

> 
> Fixes: 5263507b1b4a ("xen: arm: Use a direct mapping of RAM on arm64")

... while this is fixing a bug, it is not going to be a candidate for 
backport.

> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>   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))
>   #define DIRECTMAP_VIRT_END     (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
>   
>   #define XENHEAP_VIRT_START     directmap_virt_start

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Michal Orzel 1 year, 2 months ago
Hi Julien,

On 17/01/2023 10:35, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> It is not clear to me why this was sent In-reply-to the other patch. If
> they are meant to form a series, then this should have a cover letter +
> each patch should be numbered.
> 
> If they are truly separate, then please don't thread them.
They were meant to be separated. I will form a series for v2 to make the commiting easier.

> 
> On 16/01/2023 14:41, Michal Orzel wrote:
>> The direct mapped area occupies L0 slots from 256 to 265 (i.e. 10 slots),
> 
> I would write "265 included"  or similar so it shows why this is a problem.
Ok.

> 
>> 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.
> 
> I would clarify that we only support up to 2TB. So this is a latent
> issue. This would make clear that...
Ok.

> 
>>
>> Fixes: 5263507b1b4a ("xen: arm: Use a direct mapping of RAM on arm64")
> 
> ... while this is fixing a bug, it is not going to be a candidate for
> backport.
> 
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>   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))
>>   #define DIRECTMAP_VIRT_END     (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
>>
>>   #define XENHEAP_VIRT_START     directmap_virt_start
> 
> Cheers,
> 
> --
> Julien Grall

~Michal
Re: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Julien Grall 1 year, 2 months ago

On 17/01/2023 09:50, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> 
> On 17/01/2023 10:35, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> It is not clear to me why this was sent In-reply-to the other patch. If
>> they are meant to form a series, then this should have a cover letter +
>> each patch should be numbered.
>>
>> If they are truly separate, then please don't thread them.
> They were meant to be separated. I will form a series for v2 to make the commiting easier.

This is only two patches. So I would be OK if you send them separately 
as well. So pick what you prefer.

Cheers,

-- 
Julien Grall
RE: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Henry Wang 1 year, 2 months ago
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@amd.com>
> ---
>  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.

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@arm.com>

Kind regards,
Henry
Re: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Michal Orzel 1 year, 2 months ago
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@amd.com>
>> ---
>>  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@arm.com>
Thanks.

> 
> Kind regards,
> Henry

~Michal
RE: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Henry Wang 1 year, 2 months ago
Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
> 
> Hi Henry,
> 
> >> -#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.

Yes indeed, so I wouldn't argue for your explanation...

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

...here and you can have my:

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

> 
> >
> > 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@arm.com>
> Thanks.

Pleasure.

Kind regards,
Henry

> 
> >
> > Kind regards,
> > Henry
> 
> ~Michal
RE: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
Posted by Henry Wang 1 year, 2 months ago
Hi Michal,

> -----Original Message-----
> Subject: RE: [PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation
> > >> -#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.
> 
> Yes indeed, so I wouldn't argue for your explanation...

Sorry I mean argue against here... :)

Kind regards,
Henry