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