Re: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 07/09/2022 09:36, Henry Wang wrote:
>   static int __init early_scan_node(const void *fdt,
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b76a84e8f5..0741645014 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1038,9 +1038,11 @@ static int __init make_memory_node(const struct domain 
> *d,
>       if ( mem->nr_banks == 0 )
>           return -ENOENT;
>   
> -    /* find first memory range not bound to a Xen domain */
> -    for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
> +    /* find first memory range not bound to a Xen domain nor heap */
This comment could become stale if we are adding a new type. So how about:

/* find the first memory range that is reserved for device (or firmware) */

> +    for ( i = 0; i < mem->nr_banks &&
> +                 (mem->bank[i].type != MEMBANK_DEFAULT); i++ )
>           ;
> +
>       if ( i == mem->nr_banks )
>           return 0;
>   
> @@ -1062,7 +1064,7 @@ static int __init make_memory_node(const struct domain *d,
>           u64 start = mem->bank[i].start;
>           u64 size = mem->bank[i].size;
>   
> -        if ( mem->bank[i].xen_domain )
> +        if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN )
>               continue;
>   
>           dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 5815ccf8c5..6bea15acff 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -22,11 +22,31 @@ typedef enum {
>       BOOTMOD_UNKNOWN
>   }  bootmodule_kind;
>   
> +enum membank_type {
> +    /*
> +     * The MEMBANK_DEFAULT type refers to either reserved memory for the
> +     * device (or firmware) or any memory that will be used by the allocator.
I realize the part of the 'or' is what I suggested. However, I wasn't 
correct here (sorry).
In the context of 'mem' this is referring to any RAM. The setup code 
will then find the list of the regions that doesn't overlap with the 
'reserved_mem' and then give the pages to the boot allocator (and 
subsequently the buddy allocator). Also...
> +     * The meaning depends on where the memory bank is actually used.
... this doesn't tell the reader which means applies where. So I would 
suggest the following:
The MEMBANK_DEFAULT type refers to either reserved memory for the 
device/firmware (when the bank is in 'reserved_mem') or any RAM (when 
the bank is in 'mem'
The rest of the code looks good to me. So once we settled on the two 
comments above:
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

--
Julien Grall