Re: [PATCH] xen/arm: Sanity test specified domain's memory for being a multiple of a page size

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 07-Oct-26 15:40, Alejandro Vallejo wrote:
> On Wed Oct 7, 2026 at 2:48 PM CEST, Michal Orzel wrote:
>> We require memory to allocate for a domain as RAM to be a multiple of a
>> page size. However, we neither document this nor sanity test. Specifying
>> memory size that does not conform to this requirement fails the domain
>> memory allocation in the non-obvious way that is difficult to parse for
>> the user (printing over-allocation messages followed by the panic that Xen
>> could not allocate the requested amount of memory).
>>
>> While there, fix indentation from tabs to spaces for "memory" parameter in
>> booting.txt.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>  docs/misc/arm/device-tree/booting.txt   | 4 ++--
>>  docs/misc/xen-command-line.pandoc       | 5 +++--
>>  xen/arch/arm/domain_build.c             | 6 ++++++
>>  xen/common/device-tree/dom0less-build.c | 7 +++++++
>>  4 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/misc/arm/device-tree/booting.txt 
>> b/docs/misc/arm/device-tree/booting.txt
>> index bcb06bc796bf..7dfac2062a37 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -155,8 +155,8 @@ with the following properties:
>>  
>>  - memory
>>  
>> -    A 64-bit integer specifying the amount of kilobytes of RAM to
>> -    allocate to the guest.
>> +    A 64-bit integer specifying the amount of kilobytes of RAM to
>> +    allocate to the guest. Must be a multiple of 4KB (the page size).
> 
> nit: s/KB/KiB
Is it? I don't think it matters at all seeing the number of occurrences in the 
code.

> 
> But page size isn't always 4KiB on all ports. PowerPC has 64KiB pages.
> IMO, 4KB shouldn't be here or elsewhere in documentation.
This is Arm doc and on Arm we only support 4KB at the moment. User should not
need to dig into the code to check what is the page size. It's better imo to
give this information right away and change in the future when we add support
for 16 and 64KB pages on Arm.

> 
>>  
>>  - cpus
>>  
>> diff --git a/docs/misc/xen-command-line.pandoc 
>> b/docs/misc/xen-command-line.pandoc
>> index ea0e3368b1d6..d49db353d8a8 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1020,8 +1020,9 @@ and architecture-specific domain limits.
>>  > `= <size>`
>>  
>>  Set the amount of memory for the initial domain (dom0). It must be
>> -greater than zero. This parameter is required (and only used) when the 
>> initial
>> -domain is not described in the Device-Tree.
>> +greater than zero and a multiple of the page size (4KB). This parameter is
>> +required (and only used) when the initial domain is not described in the
>> +Device-Tree.
> 
> Same 4KB remark.
Still Arm only section and see my reasoning above.

> 
>>  
>>  ### dom0_mem (x86)
>>  > `= List of ( min:<sz> | max:<sz> | <sz> )`
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 72d531618045..bc3d1ebad1c4 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1881,6 +1881,12 @@ static int __init construct_dom0(struct domain *d)
>>          warning_add("PLEASE SPECIFY dom0_mem PARAMETER - USING 512M FOR 
>> NOW\n");
>>          dom0_mem = MB(512);
>>      }
>> +    else if ( dom0_mem % PAGE_SIZE )
>> +    {
>> +        printk("%pd: specified dom0_mem (%"PRIu64"B) is not a multiple of a 
>> page size\n",
>> +               d, dom0_mem);
>> +        return -EINVAL;
>> +    }
> 
> nit: XENLOG_WARNING?
I can use XENLOG_ERR.

> 
> Also, is this is meant to be self-healing? If so the return should just be:
> 
>     dom0_mem = round_pgdown(dom0_mem);
> 
> and continue. And if it isn't meant to be self-healing, why not just panic()?
1. It should not be self-healing. On Arm and in DT subsystem in general we chose
to follow the contract to bail out on unsatisfied user requests rather than
silently adjusting them.
2. I think it's always better to propagate the error to the caller if possible.
Sometimes you don't know where this function end up being used in the future. In
general if the function returns non-void I tend to leave the decision (here
panic will be executed anyway) to the caller. Printk explains why and the panic
follow. This also matches the existing paths nearby this code.

> 
>>  
>>      d->max_pages = dom0_mem >> PAGE_SHIFT;
>>  
>> diff --git a/xen/common/device-tree/dom0less-build.c 
>> b/xen/common/device-tree/dom0less-build.c
>> index fcbeb8adbd73..b3f24e5d555f 100644
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -789,6 +789,13 @@ static int __init construct_domU(struct kernel_info 
>> *kinfo,
>>      }
>>      kinfo->unassigned_mem = (paddr_t)mem * SZ_1K;
>>  
>> +    if ( kinfo->unassigned_mem % PAGE_SIZE )
>> +    {
>> +        printk("%pd: specified \"memory\" (%"PRIu64"KB) is not a multiple 
>> of a page size\n",
> 
> Seeing how PowerPC has 64KiB it might be prudent to print PAGE_SIZE to
> avoid surprises later.
Yes, I can print page size here.

> 
> Same remark about panic() vs self-healing (either is fine. It's this
> middle ground that looks off).
> 
>> +               d, mem);
>> +        return -EINVAL;
>> +    }
>> +
>>      rc = domain_p2m_set_allocation(d, mem, node);
>>      if ( rc != 0 )
>>          return rc;
> 
> Cheers,
> Alejandro

~Michal