diff mbox series

[v2,1/4] docs, xen/arm: Introduce reserved heap memory

Message ID 20220905072635.16294-2-Henry.Wang@arm.com (mailing list archive)
State New, archived
Headers show
Series Introduce reserved heap | expand

Commit Message

Henry Wang Sept. 5, 2022, 7:26 a.m. UTC
This commit introduces the reserved heap memory, which is parts of RAM
reserved in the beginning of the boot time for heap.

Firstly, since a new type of memory bank is needed for marking the
memory bank solely as the heap, this commit defines `enum membank_type`
and use this enum in function device_tree_get_meminfo(). Changes of
code are done accordingly following the introduction of this enum.

Also, this commit introduces the logic to parse the reserved heap
configuration in device tree by reusing the device tree entry definition
of the static memory allocation feature. If the memory bank is reserved
as heap through `xen,static-heap` property in device tree `chosen` node,
the memory will be marked as heap type.

A documentation section is added, describing the definition of reserved
heap memory and the method of enabling the reserved heap memory through
device tree at boot time.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
Changes from v1 to v2:
- Rename the device tree property to xen,static-heap to avoid confusion.
- Change of commit msg and doc wording, correct typo in commit msg.
- Do not change the process_chosen_node() return type.
- Add an empty line in make_memory_node() memory type check to improve
  readability.
- Use enum membank_type to make the memory type cleaner.
Changes from RFC to v1:
- Rename the terminology to reserved heap.
---
 docs/misc/arm/device-tree/booting.txt | 45 +++++++++++++++++++++++++++
 xen/arch/arm/bootfdt.c                | 31 +++++++++++++++---
 xen/arch/arm/domain_build.c           |  8 +++--
 xen/arch/arm/include/asm/setup.h      |  7 ++++-
 xen/arch/arm/setup.c                  |  2 +-
 5 files changed, 84 insertions(+), 9 deletions(-)

Comments

Michal Orzel Sept. 5, 2022, 12:04 p.m. UTC | #1
Hi Henry,

On 05/09/2022 09:26, Henry Wang wrote:
> 
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 5815ccf8c5..d0cc556833 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -22,11 +22,16 @@ typedef enum {
>      BOOTMOD_UNKNOWN
>  }  bootmodule_kind;
> 
> +typedef enum {
> +    MEMBANK_MEMORY,
> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a Xen domain. */
> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as heap. */
> +} membank_type;
Whereas the patch itself looks ok (it must be modified anyway given the comments for patch #2),
MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is part of membank_type enum.
Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be much cleaner in my opinion
as it would directly indicate what type of memory we are talking about.

~Michal
Henry Wang Sept. 5, 2022, 12:10 p.m. UTC | #2
Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> > +typedef enum {
> > +    MEMBANK_MEMORY,
> > +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a
> Xen domain. */
> > +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as
> heap. */
> > +} membank_type;
> Whereas the patch itself looks ok (it must be modified anyway given the
> comments for patch #2),
> MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is
> part of membank_type enum.
> Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be
> much cleaner in my opinion
> as it would directly indicate what type of memory we are talking about.

Thanks for the suggestion. I am pretty bad in naming things so in this patch
I simply reused the original name for static memory banks.

I prefer MEMBANK_STATICMEM and will change in v3. I will check the
80 char limit in current code, if STATICMEM does not fit, I will go STATIC.

Kind regards,
Henry

> 
> ~Michal
Julien Grall Sept. 5, 2022, 5:20 p.m. UTC | #3
Hi Henry,

On 05/09/2022 08:26, Henry Wang wrote:
> This commit introduces the reserved heap memory, which is parts of RAM
> reserved in the beginning of the boot time for heap.
> 
> Firstly, since a new type of memory bank is needed for marking the
> memory bank solely as the heap, this commit defines `enum membank_type`

The wording is a bit confusing. I read this as the code will use "enum 
membank_type" but this is not possible as your enum is anonymous.

My suggestion would be to avoid creating a typedef (see below).

> and use this enum in function device_tree_get_meminfo(). Changes of
> code are done accordingly following the introduction of this enum.
> 
> Also, this commit introduces the logic to parse the reserved heap
> configuration in device tree by reusing the device tree entry definition
> of the static memory allocation feature. If the memory bank is reserved
> as heap through `xen,static-heap` property in device tree `chosen` node,
> the memory will be marked as heap type.
> 
> A documentation section is added, describing the definition of reserved
> heap memory and the method of enabling the reserved heap memory through
> device tree at boot time.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> Changes from v1 to v2:
> - Rename the device tree property to xen,static-heap to avoid confusion.
> - Change of commit msg and doc wording, correct typo in commit msg.
> - Do not change the process_chosen_node() return type.
> - Add an empty line in make_memory_node() memory type check to improve
>    readability.
> - Use enum membank_type to make the memory type cleaner.
> Changes from RFC to v1:
> - Rename the terminology to reserved heap.
> ---
>   docs/misc/arm/device-tree/booting.txt | 45 +++++++++++++++++++++++++++
>   xen/arch/arm/bootfdt.c                | 31 +++++++++++++++---
>   xen/arch/arm/domain_build.c           |  8 +++--
>   xen/arch/arm/include/asm/setup.h      |  7 ++++-
>   xen/arch/arm/setup.c                  |  2 +-
>   5 files changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..ff7ca36715 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,48 @@ device-tree:
>   
>   This will reserve a 512MB region starting at the host physical address
>   0x30000000 to be exclusively used by DomU1.
> +
> +
> +Reserved Heap Memory
> +====================
> +
> +The reserved heap memory (also known as the statically-configured heap) refers
> +to parts of RAM reserved in the beginning of boot time for heap. The memory is
> +reserved by configuration in the device tree using physical address ranges.
> +
> +The reserved heap memory declared in the device tree defines the memory areas
> +that will be reserved to be used exclusively as heap.
> +
> +- For Arm32, since there are seperated heaps, the reserved heap will be used

type: s/seperated/separated/

> +for both domheap and xenheap.
> +- For Arm64, since there is a single heap, the defined reserved heap areas
> +shall always go to the heap allocator.
> +
> +The reserved heap memory is an optional feature and can be enabled by adding
> +below device tree properties in the `chosen` node.
> +
> +The dtb should have the following properties:
> +
> +- xen,static-heap
> +
> +    Property under the top-level "chosen" node. It specifies the address
> +    and size of Xen reserved heap memory.
> +
> +- #xen,static-heap-address-cells and #xen,static-heap-size-cells
> +
> +    Specify the number of cells used for the address and size of the
> +    "xen,static-heap" property under "chosen".
> +
> +Below is an example on how to specify the reserved heap in device tree:
> +
> +    / {
> +        chosen {
> +            #xen,static-heap-address-cells = <0x2>;
> +            #xen,static-heap-size-cells = <0x2>;
> +            xen,static-heap = <0x0 0x30000000 0x0 0x40000000>;
> +            ...
> +        };
> +    };
> +
> +RAM starting from the host physical address 0x30000000 of 1GB size will
> +be reserved as heap.
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ec81a45de9..5af71dc8ba 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -64,7 +64,7 @@ void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>   static int __init device_tree_get_meminfo(const void *fdt, int node,
>                                             const char *prop_name,
>                                             u32 address_cells, u32 size_cells,
> -                                          void *data, bool xen_domain)
> +                                          void *data, membank_type type)
>   {
>       const struct fdt_property *prop;
>       unsigned int i, banks;
> @@ -95,7 +95,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>               continue;
>           mem->bank[mem->nr_banks].start = start;
>           mem->bank[mem->nr_banks].size = size;
> -        mem->bank[mem->nr_banks].xen_domain = xen_domain;
> +        mem->bank[mem->nr_banks].type = type;
>           mem->nr_banks++;
>       }
>   
> @@ -185,7 +185,7 @@ static int __init process_memory_node(const void *fdt, int node,
>                                         void *data)
>   {
>       return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells,
> -                                   data, false);
> +                                   data, MEMBANK_MEMORY);
>   }
>   
>   static int __init process_reserved_memory_node(const void *fdt, int node,
> @@ -301,6 +301,28 @@ static void __init process_chosen_node(const void *fdt, int node,
>       paddr_t start, end;
>       int len;
>   
> +    if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) )
> +    {
> +        u32 address_cells = device_tree_get_u32(fdt, node,
> +                                "#xen,static-heap-address-cells", 0);
> +        u32 size_cells = device_tree_get_u32(fdt, node,
> +                                             "#xen,static-heap-size-cells", 0);
> +
> +        printk("Checking for reserved heap in /chosen\n");
> +        if ( address_cells < 1 || size_cells < 1 )
> +        {
> +            printk("fdt: node `%s': invalid #xen,static-heap-address-cells or #xen,static-heap-size-cells\n",
> +                   name);
> +            return;
> +        }
> +
> +        if ( device_tree_get_meminfo(fdt, node, "xen,static-heap",
> +                                     address_cells, size_cells,
> +                                     &bootinfo.reserved_mem,
> +                                     MEMBANK_RSVD_HEAP) )
> +            return;
> +    }
> +
>       printk("Checking for initrd in /chosen\n");
>   
>       prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
> @@ -358,7 +380,8 @@ static int __init process_domain_node(const void *fdt, int node,
>                                        "#xen,static-mem-size-cells", 0);
>   
>       return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
> -                                   size_cells, &bootinfo.reserved_mem, true);
> +                                   size_cells, &bootinfo.reserved_mem,
> +                                   MEMBANK_XEN_DOMAIN);
>   }
>   
>   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 3fd1186b53..1e46b95f0b 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 */
> +    for ( i = 0; i < mem->nr_banks &&
> +                 (mem->bank[i].type != MEMBANK_MEMORY); 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_XEN_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..d0cc556833 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -22,11 +22,16 @@ typedef enum {
>       BOOTMOD_UNKNOWN
>   }  bootmodule_kind;
>   
> +typedef enum {
> +    MEMBANK_MEMORY,

Technically everything is memory :). I think here you are referring to 
either:
    - Reserved memory for the device (or firmware)
    - Any memory that will be used by the allocator.

I would consider to name the field MEMBANK_UNKNOWN or MEMBANK_DEFAULT 
with a comment explaining the meaning depends where it used (we have 
several arrays using struct membank).

> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a Xen domain. */
> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as heap. */
I would clarify the two values are only valid when the bank in in 
reserved_mem.

> +} membank_type;

I would prefer if if we don't define any typedef for this enum. But if 
you want to keep it, then please suffix with _t.

>   
>   struct membank {
>       paddr_t start;
>       paddr_t size;
> -    bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
> +    membank_type type;
>   };
>   
>   struct meminfo {
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6e0398f3f6..8d3f859982 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -644,7 +644,7 @@ static void __init init_staticmem_pages(void)
>   
>       for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
>       {
> -        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
> +        if ( bootinfo.reserved_mem.bank[bank].type == MEMBANK_XEN_DOMAIN )
>           {
>               mfn_t bank_start = _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
>               unsigned long bank_pages = PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);

Cheers,
Julien Grall Sept. 5, 2022, 5:24 p.m. UTC | #4
Hi Michal,

On 05/09/2022 13:04, Michal Orzel wrote:
> On 05/09/2022 09:26, Henry Wang wrote:
>>
>> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
>> index 5815ccf8c5..d0cc556833 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -22,11 +22,16 @@ typedef enum {
>>       BOOTMOD_UNKNOWN
>>   }  bootmodule_kind;
>>
>> +typedef enum {
>> +    MEMBANK_MEMORY,
>> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a Xen domain. */
>> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as heap. */
>> +} membank_type;
> Whereas the patch itself looks ok (it must be modified anyway given the comments for patch #2),
> MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is part of membank_type enum.
> Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be much cleaner in my opinion
> as it would directly indicate what type of memory we are talking about.

I am not sure. Technically the reserved heap is static memory that has 
been allocated for the heap. In fact, I think thn name "staticmem" is 
now becoming quite confusing because we are referring to a very specific 
use case (i.e. memory that has been reserved for domain use).

So I would prefer if we keep "domain" in the name. Maybe 
MEMBANK_STATIC_DOMAIN or MEMBANK_RESERVED_DOMAIN.

Cheers,
Stefano Stabellini Sept. 5, 2022, 10:47 p.m. UTC | #5
On Mon, 5 Sep 2022, Henry Wang wrote:
> This commit introduces the reserved heap memory, which is parts of RAM
> reserved in the beginning of the boot time for heap.
> 
> Firstly, since a new type of memory bank is needed for marking the
> memory bank solely as the heap, this commit defines `enum membank_type`
> and use this enum in function device_tree_get_meminfo(). Changes of
> code are done accordingly following the introduction of this enum.
> 
> Also, this commit introduces the logic to parse the reserved heap
> configuration in device tree by reusing the device tree entry definition
> of the static memory allocation feature. If the memory bank is reserved
> as heap through `xen,static-heap` property in device tree `chosen` node,
> the memory will be marked as heap type.
> 
> A documentation section is added, describing the definition of reserved
> heap memory and the method of enabling the reserved heap memory through
> device tree at boot time.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

I think the device tree interface is good and you can consider it acked
by me. I'll let you follow up on the other comments by others on the
code changes.


> ---
> Changes from v1 to v2:
> - Rename the device tree property to xen,static-heap to avoid confusion.
> - Change of commit msg and doc wording, correct typo in commit msg.
> - Do not change the process_chosen_node() return type.
> - Add an empty line in make_memory_node() memory type check to improve
>   readability.
> - Use enum membank_type to make the memory type cleaner.
> Changes from RFC to v1:
> - Rename the terminology to reserved heap.
> ---
>  docs/misc/arm/device-tree/booting.txt | 45 +++++++++++++++++++++++++++
>  xen/arch/arm/bootfdt.c                | 31 +++++++++++++++---
>  xen/arch/arm/domain_build.c           |  8 +++--
>  xen/arch/arm/include/asm/setup.h      |  7 ++++-
>  xen/arch/arm/setup.c                  |  2 +-
>  5 files changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..ff7ca36715 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,48 @@ device-tree:
>  
>  This will reserve a 512MB region starting at the host physical address
>  0x30000000 to be exclusively used by DomU1.
> +
> +
> +Reserved Heap Memory
> +====================
> +
> +The reserved heap memory (also known as the statically-configured heap) refers
> +to parts of RAM reserved in the beginning of boot time for heap. The memory is
> +reserved by configuration in the device tree using physical address ranges.
> +
> +The reserved heap memory declared in the device tree defines the memory areas
> +that will be reserved to be used exclusively as heap.
> +
> +- For Arm32, since there are seperated heaps, the reserved heap will be used
> +for both domheap and xenheap.
> +- For Arm64, since there is a single heap, the defined reserved heap areas
> +shall always go to the heap allocator.
> +
> +The reserved heap memory is an optional feature and can be enabled by adding
> +below device tree properties in the `chosen` node.
> +
> +The dtb should have the following properties:
> +
> +- xen,static-heap
> +
> +    Property under the top-level "chosen" node. It specifies the address
> +    and size of Xen reserved heap memory.
> +
> +- #xen,static-heap-address-cells and #xen,static-heap-size-cells
> +
> +    Specify the number of cells used for the address and size of the
> +    "xen,static-heap" property under "chosen".
> +
> +Below is an example on how to specify the reserved heap in device tree:
> +
> +    / {
> +        chosen {
> +            #xen,static-heap-address-cells = <0x2>;
> +            #xen,static-heap-size-cells = <0x2>;
> +            xen,static-heap = <0x0 0x30000000 0x0 0x40000000>;
> +            ...
> +        };
> +    };
> +
> +RAM starting from the host physical address 0x30000000 of 1GB size will
> +be reserved as heap.
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ec81a45de9..5af71dc8ba 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -64,7 +64,7 @@ void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>  static int __init device_tree_get_meminfo(const void *fdt, int node,
>                                            const char *prop_name,
>                                            u32 address_cells, u32 size_cells,
> -                                          void *data, bool xen_domain)
> +                                          void *data, membank_type type)
>  {
>      const struct fdt_property *prop;
>      unsigned int i, banks;
> @@ -95,7 +95,7 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>              continue;
>          mem->bank[mem->nr_banks].start = start;
>          mem->bank[mem->nr_banks].size = size;
> -        mem->bank[mem->nr_banks].xen_domain = xen_domain;
> +        mem->bank[mem->nr_banks].type = type;
>          mem->nr_banks++;
>      }
>  
> @@ -185,7 +185,7 @@ static int __init process_memory_node(const void *fdt, int node,
>                                        void *data)
>  {
>      return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells,
> -                                   data, false);
> +                                   data, MEMBANK_MEMORY);
>  }
>  
>  static int __init process_reserved_memory_node(const void *fdt, int node,
> @@ -301,6 +301,28 @@ static void __init process_chosen_node(const void *fdt, int node,
>      paddr_t start, end;
>      int len;
>  
> +    if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) )
> +    {
> +        u32 address_cells = device_tree_get_u32(fdt, node,
> +                                "#xen,static-heap-address-cells", 0);
> +        u32 size_cells = device_tree_get_u32(fdt, node,
> +                                             "#xen,static-heap-size-cells", 0);
> +
> +        printk("Checking for reserved heap in /chosen\n");
> +        if ( address_cells < 1 || size_cells < 1 )
> +        {
> +            printk("fdt: node `%s': invalid #xen,static-heap-address-cells or #xen,static-heap-size-cells\n",
> +                   name);
> +            return;
> +        }
> +
> +        if ( device_tree_get_meminfo(fdt, node, "xen,static-heap",
> +                                     address_cells, size_cells,
> +                                     &bootinfo.reserved_mem,
> +                                     MEMBANK_RSVD_HEAP) )
> +            return;
> +    }
> +
>      printk("Checking for initrd in /chosen\n");
>  
>      prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
> @@ -358,7 +380,8 @@ static int __init process_domain_node(const void *fdt, int node,
>                                       "#xen,static-mem-size-cells", 0);
>  
>      return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
> -                                   size_cells, &bootinfo.reserved_mem, true);
> +                                   size_cells, &bootinfo.reserved_mem,
> +                                   MEMBANK_XEN_DOMAIN);
>  }
>  
>  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 3fd1186b53..1e46b95f0b 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 */
> +    for ( i = 0; i < mem->nr_banks &&
> +                 (mem->bank[i].type != MEMBANK_MEMORY); 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_XEN_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..d0cc556833 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -22,11 +22,16 @@ typedef enum {
>      BOOTMOD_UNKNOWN
>  }  bootmodule_kind;
>  
> +typedef enum {
> +    MEMBANK_MEMORY,
> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a Xen domain. */
> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as heap. */
> +} membank_type;
>  
>  struct membank {
>      paddr_t start;
>      paddr_t size;
> -    bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
> +    membank_type type;
>  };
>  
>  struct meminfo {
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6e0398f3f6..8d3f859982 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -644,7 +644,7 @@ static void __init init_staticmem_pages(void)
>  
>      for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
>      {
> -        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
> +        if ( bootinfo.reserved_mem.bank[bank].type == MEMBANK_XEN_DOMAIN )
>          {
>              mfn_t bank_start = _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
>              unsigned long bank_pages = PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
> -- 
> 2.17.1
>
Henry Wang Sept. 6, 2022, 1:04 a.m. UTC | #6
Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory
> 
> On Mon, 5 Sep 2022, Henry Wang wrote:
> > This commit introduces the reserved heap memory, which is parts of RAM
> > reserved in the beginning of the boot time for heap.
> >
> > Firstly, since a new type of memory bank is needed for marking the
> > memory bank solely as the heap, this commit defines `enum
> membank_type`
> > and use this enum in function device_tree_get_meminfo(). Changes of
> > code are done accordingly following the introduction of this enum.
> >
> > Also, this commit introduces the logic to parse the reserved heap
> > configuration in device tree by reusing the device tree entry definition
> > of the static memory allocation feature. If the memory bank is reserved
> > as heap through `xen,static-heap` property in device tree `chosen` node,
> > the memory will be marked as heap type.
> >
> > A documentation section is added, describing the definition of reserved
> > heap memory and the method of enabling the reserved heap memory
> through
> > device tree at boot time.
> >
> > Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> I think the device tree interface is good and you can consider it acked
> by me. I'll let you follow up on the other comments by others on the
> code changes.

Thanks, I will fix the comments from Michal and Julien.

Kind regards,
Henry
Henry Wang Sept. 6, 2022, 1:09 a.m. UTC | #7
Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory
> 
> Hi Henry,
> 
> On 05/09/2022 08:26, Henry Wang wrote:
> > This commit introduces the reserved heap memory, which is parts of RAM
> > reserved in the beginning of the boot time for heap.
> >
> > Firstly, since a new type of memory bank is needed for marking the
> > memory bank solely as the heap, this commit defines `enum
> membank_type`
> 
> The wording is a bit confusing. I read this as the code will use "enum
> membank_type" but this is not possible as your enum is anonymous.
> 
> My suggestion would be to avoid creating a typedef (see below).

Yeah I think you are correct. The typedef is not really necessary.

> 
> > +- For Arm32, since there are seperated heaps, the reserved heap will be
> used
> 
> type: s/seperated/separated/

Oops, sorry again..

> 
> > +typedef enum {
> > +    MEMBANK_MEMORY,
> 
> Technically everything is memory :). I think here you are referring to
> either:
>     - Reserved memory for the device (or firmware)
>     - Any memory that will be used by the allocator.
> 
> I would consider to name the field MEMBANK_UNKNOWN or
> MEMBANK_DEFAULT
> with a comment explaining the meaning depends where it used (we have
> several arrays using struct membank).

MEMBANK_DEFAULT sounds good, and I will add the comment.

> 
> > +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a
> Xen domain. */
> > +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as
> heap. */
> I would clarify the two values are only valid when the bank in in
> reserved_mem.

Good point, will do.

> 
> > +} membank_type;
> 
> I would prefer if if we don't define any typedef for this enum. But if
> you want to keep it, then please suffix with _t.

No I think you are correct, a enum membank_type instead of a typedef
would be enough here.

Kind regards,
Henry

> 
> >
> >   struct membank {
> >       paddr_t start;
> >       paddr_t size;
> > -    bool xen_domain; /* whether the memory bank is bound to a Xen
> domain. */
> > +    membank_type type;
> >   };
> >
> >   struct meminfo {
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 6e0398f3f6..8d3f859982 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -644,7 +644,7 @@ static void __init init_staticmem_pages(void)
> >
> >       for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> >       {
> > -        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
> > +        if ( bootinfo.reserved_mem.bank[bank].type ==
> MEMBANK_XEN_DOMAIN )
> >           {
> >               mfn_t bank_start =
> _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
> >               unsigned long bank_pages =
> PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
> 
> Cheers,
> 
> --
> Julien Grall
Michal Orzel Sept. 6, 2022, 6:34 a.m. UTC | #8
Hi Julien,

On 05/09/2022 19:24, Julien Grall wrote:
> 
> Hi Michal,
> 
> On 05/09/2022 13:04, Michal Orzel wrote:
>> On 05/09/2022 09:26, Henry Wang wrote:
>>>
>>> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
>>> index 5815ccf8c5..d0cc556833 100644
>>> --- a/xen/arch/arm/include/asm/setup.h
>>> +++ b/xen/arch/arm/include/asm/setup.h
>>> @@ -22,11 +22,16 @@ typedef enum {
>>>       BOOTMOD_UNKNOWN
>>>   }  bootmodule_kind;
>>>
>>> +typedef enum {
>>> +    MEMBANK_MEMORY,
>>> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a Xen domain. */
>>> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as heap. */
>>> +} membank_type;
>> Whereas the patch itself looks ok (it must be modified anyway given the comments for patch #2),
>> MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is part of membank_type enum.
>> Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be much cleaner in my opinion
>> as it would directly indicate what type of memory we are talking about.
> 
> I am not sure. Technically the reserved heap is static memory that has
> been allocated for the heap. In fact, I think thn name "staticmem" is
> now becoming quite confusing because we are referring to a very specific
> use case (i.e. memory that has been reserved for domain use).
> 
> So I would prefer if we keep "domain" in the name. Maybe
> MEMBANK_STATIC_DOMAIN or MEMBANK_RESERVED_DOMAIN.
> 
Personally I would drop completely using the "reserved heap" naming in favor
of "static heap" because "staticmem" is also something we reserve at boot time for a domain use.
This would also directly correlate to the device tree property "static-heap" and "static-mem".
Then such enum would be created as follows and for me this is the cleanest solution:
MEMBANK_DEFAULT
MEMBANK_STATIC_DOMAIN
MEMBANK_STATIC_HEAP

But I think we are already too late in this series to request such changes, so
with the current naming we can go for:
MEMBANK_DEFAULT
MEMBANK_RSVD_DOMAIN /* memory reserved for a domain use */
MEMBANK_RSVD_HEAP   /* memory reserved for a heap use */

> Cheers,
> 
> --
> Julien Grall

~Michal
Henry Wang Sept. 6, 2022, 6:41 a.m. UTC | #9
Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory
> 
> Hi Julien,
> 
> On 05/09/2022 19:24, Julien Grall wrote:
> >
> > Hi Michal,
> >
> > On 05/09/2022 13:04, Michal Orzel wrote:
> >> On 05/09/2022 09:26, Henry Wang wrote:
> >>>
> >>> diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> >>> index 5815ccf8c5..d0cc556833 100644
> >>> --- a/xen/arch/arm/include/asm/setup.h
> >>> +++ b/xen/arch/arm/include/asm/setup.h
> >>> @@ -22,11 +22,16 @@ typedef enum {
> >>>       BOOTMOD_UNKNOWN
> >>>   }  bootmodule_kind;
> >>>
> >>> +typedef enum {
> >>> +    MEMBANK_MEMORY,
> >>> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to
> a Xen domain. */
> >>> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as
> heap. */
> >>> +} membank_type;
> >> Whereas the patch itself looks ok (it must be modified anyway given the
> comments for patch #2),
> >> MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is
> part of membank_type enum.
> >> Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be
> much cleaner in my opinion
> >> as it would directly indicate what type of memory we are talking about.
> >
> > I am not sure. Technically the reserved heap is static memory that has
> > been allocated for the heap. In fact, I think thn name "staticmem" is
> > now becoming quite confusing because we are referring to a very specific
> > use case (i.e. memory that has been reserved for domain use).
> >
> > So I would prefer if we keep "domain" in the name. Maybe
> > MEMBANK_STATIC_DOMAIN or MEMBANK_RESERVED_DOMAIN.
> >
> Personally I would drop completely using the "reserved heap" naming in
> favor
> of "static heap" because "staticmem" is also something we reserve at boot
> time for a domain use.
> This would also directly correlate to the device tree property "static-heap"
> and "static-mem".
> Then such enum would be created as follows and for me this is the cleanest
> solution:
> MEMBANK_DEFAULT
> MEMBANK_STATIC_DOMAIN
> MEMBANK_STATIC_HEAP
> 
> But I think we are already too late in this series to request such changes,

I am ok with a pure renaming to static heap if Julien is ok with that. I think
Julien has done most of the code review and we still have 2~3 days for it.

Kind regards,
Henry

> So with the current naming we can go for:
> MEMBANK_DEFAULT
> MEMBANK_RSVD_DOMAIN /* memory reserved for a domain use */
> MEMBANK_RSVD_HEAP   /* memory reserved for a heap use */
> 
> > Cheers,
> >
> > --
> > Julien Grall
> 
> ~Michal
Julien Grall Sept. 6, 2022, 8:02 a.m. UTC | #10
Hi Henry and Michal,

On 06/09/2022 07:41, Henry Wang wrote:
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>> Subject: Re: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory
>>
>> Hi Julien,
>>
>> On 05/09/2022 19:24, Julien Grall wrote:
>>>
>>> Hi Michal,
>>>
>>> On 05/09/2022 13:04, Michal Orzel wrote:
>>>> On 05/09/2022 09:26, Henry Wang wrote:
>>>>>
>>>>> diff --git a/xen/arch/arm/include/asm/setup.h
>> b/xen/arch/arm/include/asm/setup.h
>>>>> index 5815ccf8c5..d0cc556833 100644
>>>>> --- a/xen/arch/arm/include/asm/setup.h
>>>>> +++ b/xen/arch/arm/include/asm/setup.h
>>>>> @@ -22,11 +22,16 @@ typedef enum {
>>>>>        BOOTMOD_UNKNOWN
>>>>>    }  bootmodule_kind;
>>>>>
>>>>> +typedef enum {
>>>>> +    MEMBANK_MEMORY,
>>>>> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to
>> a Xen domain. */
>>>>> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as
>> heap. */
>>>>> +} membank_type;
>>>> Whereas the patch itself looks ok (it must be modified anyway given the
>> comments for patch #2),
>>>> MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is
>> part of membank_type enum.
>>>> Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be
>> much cleaner in my opinion
>>>> as it would directly indicate what type of memory we are talking about.
>>>
>>> I am not sure. Technically the reserved heap is static memory that has
>>> been allocated for the heap. In fact, I think thn name "staticmem" is
>>> now becoming quite confusing because we are referring to a very specific
>>> use case (i.e. memory that has been reserved for domain use).
>>>
>>> So I would prefer if we keep "domain" in the name. Maybe
>>> MEMBANK_STATIC_DOMAIN or MEMBANK_RESERVED_DOMAIN.
>>>
>> Personally I would drop completely using the "reserved heap" naming in
>> favor
>> of "static heap" because "staticmem" is also something we reserve at boot
>> time for a domain use.
>> This would also directly correlate to the device tree property "static-heap"
>> and "static-mem".
>> Then such enum would be created as follows and for me this is the cleanest
>> solution:
>> MEMBANK_DEFAULT
>> MEMBANK_STATIC_DOMAIN
>> MEMBANK_STATIC_HEAP
>>
>> But I think we are already too late in this series to request such changes,

The naming was introduced in this version. So I would not view this as a 
late request.

> 
> I am ok with a pure renaming to static heap if Julien is ok with that. I think
> Julien has done most of the code review and we still have 2~3 days for it.
I am fine with the version proposed by Michal. I.e.:

MEMBANK_DEFAULT
MEMBANK_STATIC_DOMAIN
MEMBANK_STATIC_HEAP

Cheers,
diff mbox series

Patch

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..ff7ca36715 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -378,3 +378,48 @@  device-tree:
 
 This will reserve a 512MB region starting at the host physical address
 0x30000000 to be exclusively used by DomU1.
+
+
+Reserved Heap Memory
+====================
+
+The reserved heap memory (also known as the statically-configured heap) refers
+to parts of RAM reserved in the beginning of boot time for heap. The memory is
+reserved by configuration in the device tree using physical address ranges.
+
+The reserved heap memory declared in the device tree defines the memory areas
+that will be reserved to be used exclusively as heap.
+
+- For Arm32, since there are seperated heaps, the reserved heap will be used
+for both domheap and xenheap.
+- For Arm64, since there is a single heap, the defined reserved heap areas
+shall always go to the heap allocator.
+
+The reserved heap memory is an optional feature and can be enabled by adding
+below device tree properties in the `chosen` node.
+
+The dtb should have the following properties:
+
+- xen,static-heap
+
+    Property under the top-level "chosen" node. It specifies the address
+    and size of Xen reserved heap memory.
+
+- #xen,static-heap-address-cells and #xen,static-heap-size-cells
+
+    Specify the number of cells used for the address and size of the
+    "xen,static-heap" property under "chosen".
+
+Below is an example on how to specify the reserved heap in device tree:
+
+    / {
+        chosen {
+            #xen,static-heap-address-cells = <0x2>;
+            #xen,static-heap-size-cells = <0x2>;
+            xen,static-heap = <0x0 0x30000000 0x0 0x40000000>;
+            ...
+        };
+    };
+
+RAM starting from the host physical address 0x30000000 of 1GB size will
+be reserved as heap.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ec81a45de9..5af71dc8ba 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -64,7 +64,7 @@  void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
 static int __init device_tree_get_meminfo(const void *fdt, int node,
                                           const char *prop_name,
                                           u32 address_cells, u32 size_cells,
-                                          void *data, bool xen_domain)
+                                          void *data, membank_type type)
 {
     const struct fdt_property *prop;
     unsigned int i, banks;
@@ -95,7 +95,7 @@  static int __init device_tree_get_meminfo(const void *fdt, int node,
             continue;
         mem->bank[mem->nr_banks].start = start;
         mem->bank[mem->nr_banks].size = size;
-        mem->bank[mem->nr_banks].xen_domain = xen_domain;
+        mem->bank[mem->nr_banks].type = type;
         mem->nr_banks++;
     }
 
@@ -185,7 +185,7 @@  static int __init process_memory_node(const void *fdt, int node,
                                       void *data)
 {
     return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells,
-                                   data, false);
+                                   data, MEMBANK_MEMORY);
 }
 
 static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -301,6 +301,28 @@  static void __init process_chosen_node(const void *fdt, int node,
     paddr_t start, end;
     int len;
 
+    if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) )
+    {
+        u32 address_cells = device_tree_get_u32(fdt, node,
+                                "#xen,static-heap-address-cells", 0);
+        u32 size_cells = device_tree_get_u32(fdt, node,
+                                             "#xen,static-heap-size-cells", 0);
+
+        printk("Checking for reserved heap in /chosen\n");
+        if ( address_cells < 1 || size_cells < 1 )
+        {
+            printk("fdt: node `%s': invalid #xen,static-heap-address-cells or #xen,static-heap-size-cells\n",
+                   name);
+            return;
+        }
+
+        if ( device_tree_get_meminfo(fdt, node, "xen,static-heap",
+                                     address_cells, size_cells,
+                                     &bootinfo.reserved_mem,
+                                     MEMBANK_RSVD_HEAP) )
+            return;
+    }
+
     printk("Checking for initrd in /chosen\n");
 
     prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
@@ -358,7 +380,8 @@  static int __init process_domain_node(const void *fdt, int node,
                                      "#xen,static-mem-size-cells", 0);
 
     return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
-                                   size_cells, &bootinfo.reserved_mem, true);
+                                   size_cells, &bootinfo.reserved_mem,
+                                   MEMBANK_XEN_DOMAIN);
 }
 
 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 3fd1186b53..1e46b95f0b 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 */
+    for ( i = 0; i < mem->nr_banks &&
+                 (mem->bank[i].type != MEMBANK_MEMORY); 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_XEN_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..d0cc556833 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -22,11 +22,16 @@  typedef enum {
     BOOTMOD_UNKNOWN
 }  bootmodule_kind;
 
+typedef enum {
+    MEMBANK_MEMORY,
+    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a Xen domain. */
+    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as heap. */
+} membank_type;
 
 struct membank {
     paddr_t start;
     paddr_t size;
-    bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
+    membank_type type;
 };
 
 struct meminfo {
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6e0398f3f6..8d3f859982 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -644,7 +644,7 @@  static void __init init_staticmem_pages(void)
 
     for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
     {
-        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
+        if ( bootinfo.reserved_mem.bank[bank].type == MEMBANK_XEN_DOMAIN )
         {
             mfn_t bank_start = _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
             unsigned long bank_pages = PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);