[PATCH] xen/arm: Harden setup_frametable_mappings

[PATCH] xen/arm: Harden setup_frametable_mappings

[Michal Orzel]

The amount of supported physical memory depends on the frametable size
and the number of struct page_info entries that can fit into it. Define
a macro PAGE_INFO_SIZE to store the current size of the struct page_info
(i.e. 56B for arm64 and 32B for arm32) and add a sanity check in
setup_frametable_mappings to be notified whenever the size of the
structure changes. Also call a panic if the calculated frametable_size
exceeds the limit defined by FRAMETABLE_SIZE macro.

Update the comments regarding the frametable in asm/config.h and take
the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/include/asm/config.h |  5 ++---
 xen/arch/arm/include/asm/mm.h     | 11 +++++++++++
 xen/arch/arm/mm.c                 |  5 +++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 16213c8b677f..d8f99776986f 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -82,7 +82,7 @@
  * ARM32 layout:
  *   0  -  12M   <COMMON>
  *
- *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
+ *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
  * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
  *                    space
  *
@@ -95,7 +95,7 @@
  *
  *   1G -   2G   VMAP: ioremap and early_ioremap
  *
- *  32G -  64G   Frametable: 24 bytes per page for 5.3TB of RAM
+ *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
  *
  * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255])
  *  Unused
@@ -127,7 +127,6 @@
 #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
 #define FRAMETABLE_SIZE        MB(128-32)
 #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
-#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
 
 #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
 #define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 68adcac9fa8d..23dec574eb31 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -26,6 +26,17 @@
  */
 #define PFN_ORDER(_pfn) ((_pfn)->v.free.order)
 
+/*
+ * The size of struct page_info impacts the number of entries that can fit
+ * into the frametable area and thus it affects the amount of physical memory
+ * we claim to support. Define PAGE_INFO_SIZE to be used for sanity checking.
+*/
+#ifdef CONFIG_ARM_64
+#define PAGE_INFO_SIZE 56
+#else
+#define PAGE_INFO_SIZE 32
+#endif
+
 struct page_info
 {
     /* Each frame can be threaded onto a doubly-linked list. */
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0fc6f2992dd1..a8c28fa5b768 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
     int rc;
 
+    BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
+
+    if ( frametable_size > FRAMETABLE_SIZE )
+        panic("RAM size is too big to fit in a frametable area\n");
+
     frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
     /* Round up to 2M or 32M boundary, as appropriate. */
     frametable_size = ROUNDUP(frametable_size, mapping_size);
-- 
2.25.1

[PATCH] xen/arm64: Fix incorrect DIRECTMAP_SIZE calculation

[Michal Orzel]

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/arm: Harden setup_frametable_mappings

[Henry Wang]

Hi Michal,

> -----Original Message-----
> Subject: [PATCH] xen/arm: Harden setup_frametable_mappings
> 
> The amount of supported physical memory depends on the frametable size
> and the number of struct page_info entries that can fit into it. Define
> a macro PAGE_INFO_SIZE to store the current size of the struct page_info
> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in
> setup_frametable_mappings to be notified whenever the size of the
> structure changes. Also call a panic if the calculated frametable_size
> exceeds the limit defined by FRAMETABLE_SIZE macro.
> 
> Update the comments regarding the frametable in asm/config.h and take
> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

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

I've also used XTP to test this patch on FVP 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

[Henry Wang]

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

[Michal Orzel]

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

[Henry Wang]

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

[Henry Wang]

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

Re: [PATCH] xen/arm: Harden setup_frametable_mappings

[Julien Grall]

Hi Michal,

On 16/01/2023 14:41, Michal Orzel wrote:
> The amount of supported physical memory depends on the frametable size
> and the number of struct page_info entries that can fit into it. Define
> a macro PAGE_INFO_SIZE to store the current size of the struct page_info
> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in
> setup_frametable_mappings to be notified whenever the size of the
> structure changes. Also call a panic if the calculated frametable_size
> exceeds the limit defined by FRAMETABLE_SIZE macro.
> 
> Update the comments regarding the frametable in asm/config.h and take
> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>   xen/arch/arm/include/asm/config.h |  5 ++---
>   xen/arch/arm/include/asm/mm.h     | 11 +++++++++++
>   xen/arch/arm/mm.c                 |  5 +++++
>   3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 16213c8b677f..d8f99776986f 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -82,7 +82,7 @@
>    * ARM32 layout:
>    *   0  -  12M   <COMMON>
>    *
> - *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
> + *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
>    * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>    *                    space
>    *
> @@ -95,7 +95,7 @@
>    *
>    *   1G -   2G   VMAP: ioremap and early_ioremap
>    *
> - *  32G -  64G   Frametable: 24 bytes per page for 5.3TB of RAM
> + *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>    *
>    * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255])
>    *  Unused
> @@ -127,7 +127,6 @@
>   #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
>   #define FRAMETABLE_SIZE        MB(128-32)
>   #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
> -#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)

This is somewhat unrelated to the goal of the patch. We do clean-up in 
the same patch, but they tend to be in the same of already modified hunk 
(which is not the case here).

So I would prefer if this is split. This would make this patch a 
potential candidate for backport.

>   
>   #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
>   #define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 68adcac9fa8d..23dec574eb31 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -26,6 +26,17 @@
>    */
>   #define PFN_ORDER(_pfn) ((_pfn)->v.free.order)
>   
> +/*
> + * The size of struct page_info impacts the number of entries that can fit
> + * into the frametable area and thus it affects the amount of physical memory
> + * we claim to support. Define PAGE_INFO_SIZE to be used for sanity checking.
> +*/
> +#ifdef CONFIG_ARM_64
> +#define PAGE_INFO_SIZE 56
> +#else
> +#define PAGE_INFO_SIZE 32
> +#endif
> +
>   struct page_info
>   {
>       /* Each frame can be threaded onto a doubly-linked list. */
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0fc6f2992dd1..a8c28fa5b768 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>       const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
>       int rc;
>   
> +    BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
> +
> +    if ( frametable_size > FRAMETABLE_SIZE )
> +        panic("RAM size is too big to fit in a frametable area\n");

This is not correct. Depending on the PDX compression, the frametable 
may end up to cover non-RAM. So I would write:

"The cannot frametable cannot cover the physical region 0x%PRIpaddr - 
0x%PRIpaddr".

> +
>       frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
>       /* Round up to 2M or 32M boundary, as appropriate. */
>       frametable_size = ROUNDUP(frametable_size, mapping_size);

Cheers,

-- 
Julien Grall

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

[Julien Grall]

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/arm: Harden setup_frametable_mappings

[Michal Orzel]

Hi Julien,

On 17/01/2023 10:29, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 16/01/2023 14:41, Michal Orzel wrote:
>> The amount of supported physical memory depends on the frametable size
>> and the number of struct page_info entries that can fit into it. Define
>> a macro PAGE_INFO_SIZE to store the current size of the struct page_info
>> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in
>> setup_frametable_mappings to be notified whenever the size of the
>> structure changes. Also call a panic if the calculated frametable_size
>> exceeds the limit defined by FRAMETABLE_SIZE macro.
>>
>> Update the comments regarding the frametable in asm/config.h and take
>> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>   xen/arch/arm/include/asm/config.h |  5 ++---
>>   xen/arch/arm/include/asm/mm.h     | 11 +++++++++++
>>   xen/arch/arm/mm.c                 |  5 +++++
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
>> index 16213c8b677f..d8f99776986f 100644
>> --- a/xen/arch/arm/include/asm/config.h
>> +++ b/xen/arch/arm/include/asm/config.h
>> @@ -82,7 +82,7 @@
>>    * ARM32 layout:
>>    *   0  -  12M   <COMMON>
>>    *
>> - *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>> + *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
>>    * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>    *                    space
>>    *
>> @@ -95,7 +95,7 @@
>>    *
>>    *   1G -   2G   VMAP: ioremap and early_ioremap
>>    *
>> - *  32G -  64G   Frametable: 24 bytes per page for 5.3TB of RAM
>> + *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>>    *
>>    * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255])
>>    *  Unused
>> @@ -127,7 +127,6 @@
>>   #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
>>   #define FRAMETABLE_SIZE        MB(128-32)
>>   #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>> -#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
> 
> This is somewhat unrelated to the goal of the patch. We do clean-up in
> the same patch, but they tend to be in the same of already modified hunk
> (which is not the case here).
> 
> So I would prefer if this is split. This would make this patch a
> potential candidate for backport.
Just for clarity. Do you mean to separate all the config.h changes or only
the FRAMETABLE_VIRT_END removal? I guess the former.

> 
>>
>>   #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
>>   #define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
>> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
>> index 68adcac9fa8d..23dec574eb31 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -26,6 +26,17 @@
>>    */
>>   #define PFN_ORDER(_pfn) ((_pfn)->v.free.order)
>>
>> +/*
>> + * The size of struct page_info impacts the number of entries that can fit
>> + * into the frametable area and thus it affects the amount of physical memory
>> + * we claim to support. Define PAGE_INFO_SIZE to be used for sanity checking.
>> +*/
>> +#ifdef CONFIG_ARM_64
>> +#define PAGE_INFO_SIZE 56
>> +#else
>> +#define PAGE_INFO_SIZE 32
>> +#endif
>> +
>>   struct page_info
>>   {
>>       /* Each frame can be threaded onto a doubly-linked list. */
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 0fc6f2992dd1..a8c28fa5b768 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -676,6 +676,11 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>>       const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
>>       int rc;
>>
>> +    BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
>> +
>> +    if ( frametable_size > FRAMETABLE_SIZE )
>> +        panic("RAM size is too big to fit in a frametable area\n");
> 
> This is not correct. Depending on the PDX compression, the frametable
> may end up to cover non-RAM. So I would write:
> 
> "The cannot frametable cannot cover the physical region 0x%PRIpaddr -
> 0x%PRIpaddr".
Yes, you're right.

> 
>> +
>>       frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
>>       /* Round up to 2M or 32M boundary, as appropriate. */
>>       frametable_size = ROUNDUP(frametable_size, mapping_size);
> 
> Cheers,
> 
> --
> Julien Grall

~Michal

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

[Michal Orzel]

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

[Julien Grall]

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/arm: Harden setup_frametable_mappings

[Julien Grall]

Hi Michal,

On 17/01/2023 09:49, Michal Orzel wrote:
> Hi Julien,
> 
> On 17/01/2023 10:29, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> On 16/01/2023 14:41, Michal Orzel wrote:
>>> The amount of supported physical memory depends on the frametable size
>>> and the number of struct page_info entries that can fit into it. Define
>>> a macro PAGE_INFO_SIZE to store the current size of the struct page_info
>>> (i.e. 56B for arm64 and 32B for arm32) and add a sanity check in
>>> setup_frametable_mappings to be notified whenever the size of the
>>> structure changes. Also call a panic if the calculated frametable_size
>>> exceeds the limit defined by FRAMETABLE_SIZE macro.
>>>
>>> Update the comments regarding the frametable in asm/config.h and take
>>> the opportunity to remove unused macro FRAMETABLE_VIRT_END on arm32.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>>    xen/arch/arm/include/asm/config.h |  5 ++---
>>>    xen/arch/arm/include/asm/mm.h     | 11 +++++++++++
>>>    xen/arch/arm/mm.c                 |  5 +++++
>>>    3 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
>>> index 16213c8b677f..d8f99776986f 100644
>>> --- a/xen/arch/arm/include/asm/config.h
>>> +++ b/xen/arch/arm/include/asm/config.h
>>> @@ -82,7 +82,7 @@
>>>     * ARM32 layout:
>>>     *   0  -  12M   <COMMON>
>>>     *
>>> - *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>>> + *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
>>>     * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>>     *                    space
>>>     *
>>> @@ -95,7 +95,7 @@
>>>     *
>>>     *   1G -   2G   VMAP: ioremap and early_ioremap
>>>     *
>>> - *  32G -  64G   Frametable: 24 bytes per page for 5.3TB of RAM
>>> + *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>>>     *
>>>     * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255])
>>>     *  Unused
>>> @@ -127,7 +127,6 @@
>>>    #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
>>>    #define FRAMETABLE_SIZE        MB(128-32)
>>>    #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>>> -#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>>
>> This is somewhat unrelated to the goal of the patch. We do clean-up in
>> the same patch, but they tend to be in the same of already modified hunk
>> (which is not the case here).
>>
>> So I would prefer if this is split. This would make this patch a
>> potential candidate for backport.
> Just for clarity. Do you mean to separate all the config.h changes or only
> the FRAMETABLE_VIRT_END removal? I guess the former.

The latter. The comment update makes sense here because this is what 
actually triggered this patch.

Cheers,

-- 
Julien Grall