Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 07/06/2021 03:43, Penny Zheng wrote:
> This commit checks `xen,static-mem` device tree property in /domUx node,
> to determine whether domain is on Static Allocation, when constructing
> domain during boot-up.
>
> Right now, the implementation of allocate_static_memory is missing, and
> will be introduced later. It just BUG() out at the moment.
>
> And if the `memory` property and `xen,static-mem` are both set, it shall
> be verified that if the memory size defined in both is consistent.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> changes v2:
> - remove parsing procedure here
> - check the consistency when `xen,static-mem` and `memory` are both defined
> ---
>   xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 282416e74d..4166d7993c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2424,23 +2424,47 @@ static int __init construct_domU(struct domain *d,
>   {
>       struct kernel_info kinfo = {};
>       int rc;
> -    u64 mem;
> +    u64 mem, static_mem_size = 0;
> +    const struct dt_property *prop;
> +    bool static_mem = false;
> +
> +    d->max_pages = ~0U;
> +    /*
> +     * Guest RAM could be of static memory from static allocation,
> +     * which will be specified through "xen,static-mem" phandle.
> +     */
> +    prop = dt_find_property(node, "xen,static-mem", NULL);
> +    if ( prop )
> +    {
> +        static_mem = true;
> +        /* static_mem_size = allocate_static_memory(...); */
> +        BUG();
> +    }
I would prefer if the static memory is allocated close to 
allocate_memory() below. AFAICT, the reason you allocate here is because 
you want to have the property "memory" optional.
However, I am not entirely convinced this is a good idea to make 
optional. It would be easier for a reader to figure out from the 
device-tree how much memory we give to the guest.
>   
>       rc = dt_property_read_u64(node, "memory", &mem);
> -    if ( !rc )
> +    if ( !static_mem && !rc )
>       {
>           printk("Error building DomU: cannot read \"memory\" property\n");
>           return -EINVAL;
> +    } else if ( rc && static_mem )
> +    {
> +        if ( static_mem_size != mem * SZ_1K )
> +        {
> +            printk("Memory size in \"memory\" property isn't consistent with"
> +                   "the ones defined in \"xen,static-mem\".\n");
> +            return -EINVAL;
> +        }
>       } > -    kinfo.unassigned_mem = (paddr_t)mem * SZ_1K;
> +    kinfo.unassigned_mem = static_mem ? 0 : (paddr_t)mem * SZ_1K; >
> -    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d->max_vcpus, 
> mem);
> +    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n",
> +            d->max_vcpus,
> +            static_mem ? static_mem_size : (kinfo.unassigned_mem) >> 10);

If we mandate the property "memory", then kinfo.unassigned_mem doesn't 
need to be touched. Instead, you could simply check the 
kinfo.unassigned_mem is equivalent to static_mem_size.
>   
>       kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
>   
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
> -    d->max_pages = ~0U;
>   
>       kinfo.d = d;
>   
> @@ -2452,7 +2476,8 @@ static int __init construct_domU(struct domain *d,
>       /* type must be set before allocate memory */
>       d->arch.type = kinfo.type;
>   #endif
> -    allocate_memory(d, &kinfo);
> +    if ( !static_mem )
> +        allocate_memory(d, &kinfo);
>   
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
Cheers,

--
Julien Grall