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