[PATCH v3 0/4] Introduce static heap

[PATCH v3 0/4] Introduce static heap

[Henry Wang]

The static heap, or statically configured heap, refers to parts
of RAM reserved in the beginning for heap. Like the static memory
allocation, such static heap regions are reserved by configuration
in the device tree using physical address ranges.

This feature is useful to run Xen on Arm MPU systems, where only a
finite number of memory protection regions are available. The limited
number of protection regions places requirement on planning the use
of MPU protection regions and one or more MPU protection regions needs
to be reserved only for heap.

The first patch enhances the error handling in processing the dts
chosen node. The second patch introduces the static heap and the
device tree parsing code. The third patch renames xenheap_* to
directmap_* for better readablity. The fourth patch adds the
implementation of the static heap pages handling in boot and heap
allocator for Arm.

Changes from v2 to v3:
- Adjust the order of patches.
- Define `enum membank_type` properly, drop the typedef.
- Rename the feature terminology to static heap.
- Rename MEMBANK_MEMORY to MEMBANK_DEFAULT and MEMBANK_XEN_DOMAIN to
  MEMBANK_STATIC_DOMAIN. Add comments to `enum membank_type`.
- Correct typo, add the clarification of the static heap region
  should contain enough memory below 4GB to cater 32-bit DMA for Arm32,
  and add the 64KB alignment requirement in doc.
- Add Stefano's Acked-by for device tree interface.
- Adjustment of the terminology change to "static heap".
- Change of wording in comments.
- int i -> unsigned int i.
- Avoid extra indentation by reverting the check of MEMBANK_RSVD_HEAP.
- Use MB(32).
- Drop unnecessary panic and unused variables.
- Avoid the ternary operation in assigning the heap_pages.
- Rework populate_boot_allocator() for static heap.

Henry Wang (4):
  xen/arm: bootfdt: Make process_chosen_node() return int
  docs, xen/arm: Introduce static heap memory
  xen/arm: mm: Rename xenheap_* variable to directmap_*
  xen/arm: Handle static heap pages in boot and heap allocator

 docs/misc/arm/device-tree/booting.txt |  48 +++++++++
 xen/arch/arm/bootfdt.c                |  55 +++++++---
 xen/arch/arm/domain_build.c           |   8 +-
 xen/arch/arm/include/asm/config.h     |   2 +-
 xen/arch/arm/include/asm/mm.h         |  22 ++--
 xen/arch/arm/include/asm/setup.h      |  23 +++-
 xen/arch/arm/mm.c                     |  24 ++---
 xen/arch/arm/setup.c                  | 147 +++++++++++++++++++++-----
 8 files changed, 260 insertions(+), 69 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] xen/arm: bootfdt: Make process_chosen_node() return int

[Henry Wang]

At the boot time, it is saner to stop booting early if an error occurs
when parsing the device tree chosen node, rather than seeing random
behavior afterwards. Therefore, this commit changes the return type of
the process_chosen_node() from void to int, and return correct errno
based on the error type.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
Changes from v2 to v3:
- Adjust the order of this patch, make it the #1.
Changes from v1 to v2:
- New commit.
---
 xen/arch/arm/bootfdt.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ec81a45de9..1a79b969af 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -293,9 +293,9 @@ static void __init process_multiboot_node(const void *fdt, int node,
                      kind, start, domU);
 }
 
-static void __init process_chosen_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
+static int __init process_chosen_node(const void *fdt, int node,
+                                      const char *name,
+                                      u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
     paddr_t start, end;
@@ -306,11 +306,11 @@ static void __init process_chosen_node(const void *fdt, int node,
     prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
     if ( !prop )
         /* No initrd present. */
-        return;
+        return 0;
     if ( len != sizeof(u32) && len != sizeof(u64) )
     {
         printk("linux,initrd-start property has invalid length %d\n", len);
-        return;
+        return -EINVAL;
     }
     start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
 
@@ -318,12 +318,12 @@ static void __init process_chosen_node(const void *fdt, int node,
     if ( !prop )
     {
         printk("linux,initrd-end not present but -start was\n");
-        return;
+        return -EINVAL;
     }
     if ( len != sizeof(u32) && len != sizeof(u64) )
     {
         printk("linux,initrd-end property has invalid length %d\n", len);
-        return;
+        return -EINVAL;
     }
     end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
 
@@ -331,12 +331,14 @@ static void __init process_chosen_node(const void *fdt, int node,
     {
         printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
                   start, end);
-        return;
+        return -EINVAL;
     }
 
     printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
 
     add_boot_module(BOOTMOD_RAMDISK, start, end-start, false);
+
+    return 0;
 }
 
 static int __init process_domain_node(const void *fdt, int node,
@@ -383,7 +385,7 @@ static int __init early_scan_node(const void *fdt,
               device_tree_node_compatible(fdt, node, "multiboot,module" )))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
-        process_chosen_node(fdt, node, name, address_cells, size_cells);
+        rc = process_chosen_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 2 && device_tree_node_compatible(fdt, node, "xen,domain") )
         rc = process_domain_node(fdt, node, name, address_cells, size_cells);
 
-- 
2.17.1

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

[Henry Wang]

This commit introduces the static 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 static heap
configuration in device tree. If the memory bank is reserved as heap
through `xen,static-heap` property in device tree `chosen` node, the
memory will be marked as static heap type.

A documentation section is added, describing the definition of static
heap memory and the method of enabling the static 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>
Acked-by: Stefano Stabellini <sstabellini@kernel.org> # DT interface
---
Changes from v2 to v3:
- Define `enum membank_type` properly, drop the typedef.
- Rename the feature terminology to static heap.
- Rename MEMBANK_MEMORY to MEMBANK_DEFAULT and MEMBANK_XEN_DOMAIN to
  MEMBANK_STATIC_DOMAIN. Add comments to `enum membank_type`.
- Correct typo, add the clarification of the static heap region
  should contain enough memory below 4GB to cater 32-bit DMA for Arm32,
  and add the 64KB alignment requirement in doc.
- Add Stefano's Acked-by for device tree interface.
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 | 48 +++++++++++++++++++++++++++
 xen/arch/arm/bootfdt.c                | 33 +++++++++++++++---
 xen/arch/arm/domain_build.c           |  8 +++--
 xen/arch/arm/include/asm/setup.h      | 22 +++++++++++-
 xen/arch/arm/setup.c                  |  2 +-
 5 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..5ba7d186aa 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -378,3 +378,51 @@ device-tree:
 
 This will reserve a 512MB region starting at the host physical address
 0x30000000 to be exclusively used by DomU1.
+
+
+Static Heap Memory
+==================
+
+The static heap memory 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 static 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 separated heaps, the static heap will be used
+for both domheap and xenheap. The admin should make sure that the static
+heap region should contain enough memory below 4GB to cater 32-bit DMA.
+
+- For Arm64, since there is a single heap, the defined static heap areas
+shall always go to the heap allocator.
+
+The static 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 static heap memory. Note that at least a 64KB
+    alignment is required.
+
+- #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 static 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 static heap.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 1a79b969af..cb23fad571 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, enum 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_DEFAULT);
 }
 
 static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -301,6 +301,30 @@ static int __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) )
+    {
+        int rc;
+        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 static 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 -EINVAL;
+        }
+
+        rc = device_tree_get_meminfo(fdt, node, "xen,static-heap",
+                                     address_cells, size_cells,
+                                     &bootinfo.reserved_mem,
+                                     MEMBANK_STATIC_HEAP);
+        if ( rc )
+            return rc;
+    }
+
     printk("Checking for initrd in /chosen\n");
 
     prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
@@ -360,7 +384,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_STATIC_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 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 */
+    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.
+     * The meaning depends on where the memory bank is actually used.
+     */
+    MEMBANK_DEFAULT,
+    /*
+     * The MEMBANK_STATIC_DOMAIN type is used to indicate whether the memory
+     * bank is bound to a static Xen domain. It is only valid when the bank
+     * is in reserved_mem.
+     */
+    MEMBANK_STATIC_DOMAIN,
+    /*
+     * The MEMBANK_STATIC_HEAP type is used to indicate whether the memory
+     * bank is reserved as static heap. It is only valid when the bank is
+     * in reserved_mem.
+     */
+    MEMBANK_STATIC_HEAP,
+};
 
 struct membank {
     paddr_t start;
     paddr_t size;
-    bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
+    enum membank_type type;
 };
 
 struct meminfo {
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7814fe323d..3c36c050bf 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_STATIC_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

[PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Henry Wang]

With the static heap setup, keep using xenheap_* in the function
setup_xenheap_mappings() will make the code confusing to read,
because we always need to map the full RAM on Arm64. Therefore,
renaming all "xenheap_*" variables to "directmap_*" to make clear
the area is used to access the RAM easily.

On Arm32, only the xenheap is direct mapped today. So the renaming
to "directmap_*" would be still valid for Arm32.

No functional change is intended.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
Changes from v2 to v3:
- Adjust the order of this patch, make it #3.
Changes from v1 to v2:
- New commit.
---
 xen/arch/arm/include/asm/config.h |  2 +-
 xen/arch/arm/include/asm/mm.h     | 22 +++++++++++-----------
 xen/arch/arm/mm.c                 | 24 ++++++++++++------------
 xen/arch/arm/setup.c              | 27 ++++++++++++++-------------
 4 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 2fafb9f228..0fefed1b8a 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -160,7 +160,7 @@
 #define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (265-256))
 #define DIRECTMAP_VIRT_END     (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
 
-#define XENHEAP_VIRT_START     xenheap_virt_start
+#define XENHEAP_VIRT_START     directmap_virt_start
 
 #define HYPERVISOR_VIRT_END    DIRECTMAP_VIRT_END
 
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 749fbefa0c..7b4f6ce233 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -154,19 +154,19 @@ struct page_info
 #define _PGC_need_scrub   _PGC_allocated
 #define PGC_need_scrub    PGC_allocated
 
-extern mfn_t xenheap_mfn_start, xenheap_mfn_end;
-extern vaddr_t xenheap_virt_end;
+extern mfn_t directmap_mfn_start, directmap_mfn_end;
+extern vaddr_t directmap_virt_end;
 #ifdef CONFIG_ARM_64
-extern vaddr_t xenheap_virt_start;
-extern unsigned long xenheap_base_pdx;
+extern vaddr_t directmap_virt_start;
+extern unsigned long directmap_base_pdx;
 #endif
 
 #ifdef CONFIG_ARM_32
 #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
 #define is_xen_heap_mfn(mfn) ({                                 \
     unsigned long mfn_ = mfn_x(mfn);                            \
-    (mfn_ >= mfn_x(xenheap_mfn_start) &&                        \
-     mfn_ < mfn_x(xenheap_mfn_end));                            \
+    (mfn_ >= mfn_x(directmap_mfn_start) &&                      \
+     mfn_ < mfn_x(directmap_mfn_end));                          \
 })
 #else
 #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
@@ -267,16 +267,16 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
 static inline void *maddr_to_virt(paddr_t ma)
 {
     ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
-    ma -= mfn_to_maddr(xenheap_mfn_start);
+    ma -= mfn_to_maddr(directmap_mfn_start);
     return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
 }
 #else
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - xenheap_base_pdx) <
+    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - directmap_base_pdx) <
            (DIRECTMAP_SIZE >> PAGE_SHIFT));
     return (void *)(XENHEAP_VIRT_START -
-                    (xenheap_base_pdx << PAGE_SHIFT) +
+                    (directmap_base_pdx << PAGE_SHIFT) +
                     ((ma & ma_va_bottom_mask) |
                      ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
 }
@@ -319,10 +319,10 @@ static inline struct page_info *virt_to_page(const void *v)
     unsigned long pdx;
 
     ASSERT(va >= XENHEAP_VIRT_START);
-    ASSERT(va < xenheap_virt_end);
+    ASSERT(va < directmap_virt_end);
 
     pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
-    pdx += mfn_to_pdx(xenheap_mfn_start);
+    pdx += mfn_to_pdx(directmap_mfn_start);
     return frame_table + pdx - frametable_base_pdx;
 }
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7f5b317d3e..4a70ed2986 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -132,12 +132,12 @@ uint64_t init_ttbr;
 static paddr_t phys_offset;
 
 /* Limits of the Xen heap */
-mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
-mfn_t xenheap_mfn_end __read_mostly;
-vaddr_t xenheap_virt_end __read_mostly;
+mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
+mfn_t directmap_mfn_end __read_mostly;
+vaddr_t directmap_virt_end __read_mostly;
 #ifdef CONFIG_ARM_64
-vaddr_t xenheap_virt_start __read_mostly;
-unsigned long xenheap_base_pdx __read_mostly;
+vaddr_t directmap_virt_start __read_mostly;
+unsigned long directmap_base_pdx __read_mostly;
 #endif
 
 unsigned long frametable_base_pdx __read_mostly;
@@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
         panic("Unable to setup the xenheap mappings.\n");
 
     /* Record where the xenheap is, for translation routines. */
-    xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
+    directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
 }
 #else /* CONFIG_ARM_64 */
 void __init setup_xenheap_mappings(unsigned long base_mfn,
@@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
     int rc;
 
     /* First call sets the xenheap physical and virtual offset. */
-    if ( mfn_eq(xenheap_mfn_start, INVALID_MFN) )
+    if ( mfn_eq(directmap_mfn_start, INVALID_MFN) )
     {
         unsigned long mfn_gb = base_mfn & ~((FIRST_SIZE >> PAGE_SHIFT) - 1);
 
-        xenheap_mfn_start = _mfn(base_mfn);
-        xenheap_base_pdx = mfn_to_pdx(_mfn(base_mfn));
+        directmap_mfn_start = _mfn(base_mfn);
+        directmap_base_pdx = mfn_to_pdx(_mfn(base_mfn));
         /*
          * The base address may not be aligned to the first level
          * size (e.g. 1GB when using 4KB pages). This would prevent
@@ -633,13 +633,13 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
          * Prevent that by offsetting the start of the xenheap virtual
          * address.
          */
-        xenheap_virt_start = DIRECTMAP_VIRT_START +
+        directmap_virt_start = DIRECTMAP_VIRT_START +
             (base_mfn - mfn_gb) * PAGE_SIZE;
     }
 
-    if ( base_mfn < mfn_x(xenheap_mfn_start) )
+    if ( base_mfn < mfn_x(directmap_mfn_start) )
         panic("cannot add xenheap mapping at %lx below heap start %lx\n",
-              base_mfn, mfn_x(xenheap_mfn_start));
+              base_mfn, mfn_x(directmap_mfn_start));
 
     rc = map_pages_to_xen((vaddr_t)__mfn_to_virt(base_mfn),
                           _mfn(base_mfn), nr_mfns,
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 3c36c050bf..4a8334c268 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -697,11 +697,11 @@ static void __init populate_boot_allocator(void)
 
 #ifdef CONFIG_ARM_32
             /* Avoid the xenheap */
-            if ( s < mfn_to_maddr(xenheap_mfn_end) &&
-                 mfn_to_maddr(xenheap_mfn_start) < e )
+            if ( s < mfn_to_maddr(directmap_mfn_end) &&
+                 mfn_to_maddr(directmap_mfn_start) < e )
             {
-                e = mfn_to_maddr(xenheap_mfn_start);
-                n = mfn_to_maddr(xenheap_mfn_end);
+                e = mfn_to_maddr(directmap_mfn_start);
+                n = mfn_to_maddr(directmap_mfn_end);
             }
 #endif
 
@@ -793,15 +793,16 @@ static void __init setup_mm(void)
      * We need some memory to allocate the page-tables used for the
      * xenheap mappings. So populate the boot allocator first.
      *
-     * This requires us to set xenheap_mfn_{start, end} first so the Xenheap
+     * Note that currently xenheap is direct mapped on Arm32.
+     * This requires us to set directmap_mfn_{start, end} first so the Xenheap
      * region can be avoided.
      */
-    xenheap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages);
-    xenheap_mfn_end = mfn_add(xenheap_mfn_start, xenheap_pages);
+    directmap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages);
+    directmap_mfn_end = mfn_add(directmap_mfn_start, xenheap_pages);
 
     populate_boot_allocator();
 
-    setup_xenheap_mappings(mfn_x(xenheap_mfn_start), xenheap_pages);
+    setup_xenheap_mappings(mfn_x(directmap_mfn_start), xenheap_pages);
 
     /* Frame table covers all of RAM region, including holes */
     setup_frametable_mappings(ram_start, ram_end);
@@ -816,8 +817,8 @@ static void __init setup_mm(void)
               smp_processor_id());
 
     /* Add xenheap memory that was not already added to the boot allocator. */
-    init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
-                       mfn_to_maddr(xenheap_mfn_end));
+    init_xenheap_pages(mfn_to_maddr(directmap_mfn_start),
+                       mfn_to_maddr(directmap_mfn_end));
 
     init_staticmem_pages();
 }
@@ -858,9 +859,9 @@ static void __init setup_mm(void)
 
     total_pages += ram_size >> PAGE_SHIFT;
 
-    xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
-    xenheap_mfn_start = maddr_to_mfn(ram_start);
-    xenheap_mfn_end = maddr_to_mfn(ram_end);
+    directmap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
+    directmap_mfn_start = maddr_to_mfn(ram_start);
+    directmap_mfn_end = maddr_to_mfn(ram_end);
 
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
-- 
2.17.1

[PATCH v3 4/4] xen/arm: Handle static heap pages in boot and heap allocator

[Henry Wang]

This commit firstly adds a bool field `static_heap` to bootinfo.
This newly introduced field is set at the device tree parsing time
if the static heap ranges are defined in the device tree chosen
node.

For Arm32, In `setup_mm`, if the static heap is enabled, we use the
static heap region for both domheap and xenheap allocation. Note
that the xenheap on Arm32 should be always contiguous, so also add
a helper fit_xenheap_in_static_heap() for Arm32 to find the required
xenheap in the static heap regions.

For Arm64, In `setup_mm`, if the static heap is enabled and used,
we make sure that only these static heap pages are added to the boot
allocator. These static heap pages in the boot allocator are
added to the heap allocator at `end_boot_allocator()`.

If the static heap is disabled, we stick to current page allocation
strategy at boot time.

Also, take the chance to correct a "double not" print in Arm32
`setup_mm()` and replace the open-coding address ~0 by INVALID_PADDR.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
Changes from v2 to v3:
- Adjustment of the terminology change to "static heap".
- Change of wording in comments.
- int i -> unsigned int i.
- Avoid extra indentation by reverting the check of MEMBANK_RSVD_HEAP.
- Use MB(32).
- Drop unnecessary panic and unused variables.
- Avoid the ternary operation in assigning the heap_pages.
- Rework populate_boot_allocator() for static heap.
Changes from v1 to v2:
- Move the global bool `reserved_heap` to bootinfo.
- Replace the open open-coding address ~0 by INVALID_PADDR.
- Do not use reverted logic in heap_pages calculation.
- Remove unused Arm32 reserved_heap_start variable.
- Decouple the arm32 reserved heap too small size check with region
  end check.
- Reuse the arm32 original xenheap finding logic with the new helper
  to make sure xenheap on arm32 is contiguous.
Changes from RFC to v1:
- Rebase on top of latest `setup_mm()` changes.
- Added Arm32 logic in `setup_mm()`.
---
 xen/arch/arm/bootfdt.c           |   2 +
 xen/arch/arm/include/asm/setup.h |   1 +
 xen/arch/arm/setup.c             | 118 +++++++++++++++++++++++++++----
 3 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index cb23fad571..787c7b41be 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -323,6 +323,8 @@ static int __init process_chosen_node(const void *fdt, int node,
                                      MEMBANK_STATIC_HEAP);
         if ( rc )
             return rc;
+
+        bootinfo.static_heap = true;
     }
 
     printk("Checking for initrd in /chosen\n");
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 6bea15acff..9374b92441 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -97,6 +97,7 @@ struct bootinfo {
 #ifdef CONFIG_ACPI
     struct meminfo acpi;
 #endif
+    bool static_heap;
 };
 
 struct map_range_data
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4a8334c268..86431eb3ea 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -556,6 +556,44 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
     }
     return e;
 }
+
+/*
+ * Find a contiguous region that fits in the static heap region with
+ * required size and alignment, and return the end address of the region
+ * if found otherwise 0.
+ */
+static paddr_t __init fit_xenheap_in_static_heap(uint32_t size, paddr_t align)
+{
+    unsigned int i;
+    paddr_t end = 0, aligned_start, aligned_end;
+    paddr_t bank_start, bank_size, bank_end;
+
+    for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+    {
+        if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+            continue;
+
+        bank_start = bootinfo.reserved_mem.bank[i].start;
+        bank_size = bootinfo.reserved_mem.bank[i].size;
+        bank_end = bank_start + bank_size;
+
+        if ( bank_size < size )
+            continue;
+
+        aligned_end = bank_end & ~(align - 1);
+        aligned_start = (aligned_end - size) & ~(align - 1);
+
+        if ( aligned_start > bank_start )
+            /*
+             * Allocate the xenheap as high as possible to keep low-memory
+             * available (assuming the admin supplied region below 4GB)
+             * for other use (e.g. domain memory allocation).
+             */
+            end = max(end, aligned_end);
+    }
+
+    return end;
+}
 #endif
 
 /*
@@ -661,22 +699,51 @@ static void __init init_staticmem_pages(void)
 }
 
 /*
- * Populate the boot allocator. All the RAM but the following regions
- * will be added:
+ * Populate the boot allocator.
+ * If a static heap was not provided by the admin, all the RAM but the
+ * following regions will be added:
  *  - Modules (e.g., Xen, Kernel)
  *  - Reserved regions
  *  - Xenheap (arm32 only)
+ * If a static heap was provided by the admin, populate the boot
+ * allocator with the corresponding regions only, but with Xenheap excluded
+ * on arm32.
  */
 static void __init populate_boot_allocator(void)
 {
     unsigned int i;
     const struct meminfo *banks = &bootinfo.mem;
+    paddr_t s, e;
+
+    if ( bootinfo.static_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+                continue;
+
+            s = bootinfo.reserved_mem.bank[i].start;
+            e = s + bootinfo.reserved_mem.bank[i].size;
+#ifdef CONFIG_ARM_32
+            /* Avoid the xenheap, note that the xenheap cannot across a bank */
+            if ( s <= mfn_to_maddr(directmap_mfn_start) &&
+                 e >= mfn_to_maddr(directmap_mfn_end) )
+            {
+                init_boot_pages(s, mfn_to_maddr(directmap_mfn_start));
+                init_boot_pages(mfn_to_maddr(directmap_mfn_end), e);
+            }
+            else
+#endif
+                init_boot_pages(s, e);
+        }
+
+        return;
+    }
 
     for ( i = 0; i < banks->nr_banks; i++ )
     {
         const struct membank *bank = &banks->bank[i];
         paddr_t bank_end = bank->start + bank->size;
-        paddr_t s, e;
 
         s = bank->start;
         while ( s < bank_end )
@@ -714,8 +781,8 @@ static void __init populate_boot_allocator(void)
 #ifdef CONFIG_ARM_32
 static void __init setup_mm(void)
 {
-    paddr_t ram_start, ram_end, ram_size, e;
-    unsigned long ram_pages;
+    paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
+    paddr_t static_heap_end = 0, static_heap_size = 0;
     unsigned long heap_pages, xenheap_pages, domheap_pages;
     unsigned int i;
     const uint32_t ctr = READ_CP32(CTR);
@@ -735,30 +802,51 @@ static void __init setup_mm(void)
 
     for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
     {
-        paddr_t bank_start = bootinfo.mem.bank[i].start;
-        paddr_t bank_size = bootinfo.mem.bank[i].size;
-        paddr_t bank_end = bank_start + bank_size;
+        bank_start = bootinfo.mem.bank[i].start;
+        bank_size = bootinfo.mem.bank[i].size;
+        bank_end = bank_start + bank_size;
 
         ram_size  = ram_size + bank_size;
         ram_start = min(ram_start,bank_start);
         ram_end   = max(ram_end,bank_end);
     }
 
-    total_pages = ram_pages = ram_size >> PAGE_SHIFT;
+    total_pages = ram_size >> PAGE_SHIFT;
+
+    if ( bootinfo.static_heap )
+    {
+        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+                continue;
+
+            bank_start = bootinfo.reserved_mem.bank[i].start;
+            bank_size = bootinfo.reserved_mem.bank[i].size;
+            bank_end = bank_start + bank_size;
+
+            static_heap_size += bank_size;
+            static_heap_end = max(static_heap_end, bank_end);
+        }
+
+        heap_pages = static_heap_size >> PAGE_SHIFT;
+    }
+    else
+        heap_pages = total_pages;
 
     /*
      * If the user has not requested otherwise via the command line
      * then locate the xenheap using these constraints:
      *
+     *  - must be contiguous
      *  - must be 32 MiB aligned
      *  - must not include Xen itself or the boot modules
-     *  - must be at most 1GB or 1/32 the total RAM in the system if less
+     *  - must be at most 1GB or 1/32 the total RAM in the system (or static
+          heap if enabled) if less
      *  - must be at least 32M
      *
      * We try to allocate the largest xenheap possible within these
      * constraints.
      */
-    heap_pages = ram_pages;
     if ( opt_xenheap_megabytes )
         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
     else
@@ -770,7 +858,9 @@ static void __init setup_mm(void)
 
     do
     {
-        e = consider_modules(ram_start, ram_end,
+        e = bootinfo.static_heap ?
+            fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
+            consider_modules(ram_start, ram_end,
                              pfn_to_paddr(xenheap_pages),
                              32<<20, 0);
         if ( e )
@@ -780,7 +870,7 @@ static void __init setup_mm(void)
     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
 
     if ( ! e )
-        panic("Not not enough space for xenheap\n");
+        panic("Not enough space for xenheap\n");
 
     domheap_pages = heap_pages - xenheap_pages;
 
@@ -826,7 +916,7 @@ static void __init setup_mm(void)
 static void __init setup_mm(void)
 {
     const struct meminfo *banks = &bootinfo.mem;
-    paddr_t ram_start = ~0;
+    paddr_t ram_start = INVALID_PADDR;
     paddr_t ram_end = 0;
     paddr_t ram_size = 0;
     unsigned int i;
-- 
2.17.1

Re: [PATCH v3 1/4] xen/arm: bootfdt: Make process_chosen_node() return int

[Michal Orzel]

Hi Henry,

On 07/09/2022 10:36, Henry Wang wrote:
> 
> At the boot time, it is saner to stop booting early if an error occurs
> when parsing the device tree chosen node, rather than seeing random
> behavior afterwards. Therefore, this commit changes the return type of
> the process_chosen_node() from void to int, and return correct errno
> based on the error type.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 1/4] xen/arm: bootfdt: Make process_chosen_node() return int

[Julien Grall]

On 07/09/2022 10:19, Michal Orzel wrote:
> Hi Henry,
> 
> On 07/09/2022 10:36, Henry Wang wrote:
>>
>> At the boot time, it is saner to stop booting early if an error occurs
>> when parsing the device tree chosen node, rather than seeing random
>> behavior afterwards. Therefore, this commit changes the return type of
>> the process_chosen_node() from void to int, and return correct errno
>> based on the error type.
>>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> 
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Julien Grall]

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@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Julien Grall]

Hi Henry,

On 07/09/2022 09:36, Henry Wang wrote:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7f5b317d3e..4a70ed2986 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -132,12 +132,12 @@ uint64_t init_ttbr;
>   static paddr_t phys_offset;
>   
>   /* Limits of the Xen heap */
> -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
> -mfn_t xenheap_mfn_end __read_mostly;
> -vaddr_t xenheap_virt_end __read_mostly;
> +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
> +mfn_t directmap_mfn_end __read_mostly;
> +vaddr_t directmap_virt_end __read_mostly;
>   #ifdef CONFIG_ARM_64
> -vaddr_t xenheap_virt_start __read_mostly;
> -unsigned long xenheap_base_pdx __read_mostly;
> +vaddr_t directmap_virt_start __read_mostly;
> +unsigned long directmap_base_pdx __read_mostly;
>   #endif
>   
>   unsigned long frametable_base_pdx __read_mostly;
> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,

I think the function also want to be renamed to match the code below.

>           panic("Unable to setup the xenheap mappings.\n");

Likely, I think this wants to be s/xenheap/directmap/

>   
>       /* Record where the xenheap is, for translation routines. */

Same here because you set directmap_virt_end.

> -    xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> +    directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;

I would be OK to keep "XENHEAP_*" for now.

>   }
>   #else /* CONFIG_ARM_64 */
>   void __init setup_xenheap_mappings(unsigned long base_mfn,
> @@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>       int rc;
>   
>       /* First call sets the xenheap physical and virtual offset. */

s/xenheap/directmap/ I haven't looked if there are other instances in 
the function.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 4/4] xen/arm: Handle static heap pages in boot and heap allocator

[Julien Grall]

Hi Henry,

On 07/09/2022 09:36, Henry Wang wrote:
> This commit firstly adds a bool field `static_heap` to bootinfo.
> This newly introduced field is set at the device tree parsing time
> if the static heap ranges are defined in the device tree chosen
> node.
> 
> For Arm32, In `setup_mm`, if the static heap is enabled, we use the
> static heap region for both domheap and xenheap allocation. Note
> that the xenheap on Arm32 should be always contiguous, so also add
> a helper fit_xenheap_in_static_heap() for Arm32 to find the required
> xenheap in the static heap regions.
> 
> For Arm64, In `setup_mm`, if the static heap is enabled and used,
> we make sure that only these static heap pages are added to the boot
> allocator. These static heap pages in the boot allocator are
> added to the heap allocator at `end_boot_allocator()`.
> 
> If the static heap is disabled, we stick to current page allocation
> strategy at boot time.
> 
> Also, take the chance to correct a "double not" print in Arm32
> `setup_mm()` and replace the open-coding address ~0 by INVALID_PADDR.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> > -    /* 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) */

Ok, will use this one.

> 
> > +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).

No problem, actually, I've learned a lot about how Xen does the memory
management from your comments. So I should say thank you.

> 
> 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'

Ok will follow that.

> 
> The rest of the code looks good to me. So once we settled on the two
> comments above:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to
> directmap_*
> 
> Hi Henry,
> 
> On 07/09/2022 09:36, Henry Wang wrote:
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 7f5b317d3e..4a70ed2986 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -132,12 +132,12 @@ uint64_t init_ttbr;
> >   static paddr_t phys_offset;
> >
> >   /* Limits of the Xen heap */
> > -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
> > -mfn_t xenheap_mfn_end __read_mostly;
> > -vaddr_t xenheap_virt_end __read_mostly;
> > +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
> > +mfn_t directmap_mfn_end __read_mostly;
> > +vaddr_t directmap_virt_end __read_mostly;
> >   #ifdef CONFIG_ARM_64
> > -vaddr_t xenheap_virt_start __read_mostly;
> > -unsigned long xenheap_base_pdx __read_mostly;
> > +vaddr_t directmap_virt_start __read_mostly;
> > +unsigned long directmap_base_pdx __read_mostly;
> >   #endif
> >
> >   unsigned long frametable_base_pdx __read_mostly;
> > @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned
> long base_mfn,
> 
> I think the function also want to be renamed to match the code below.

Hmmm, renaming the name to "setup_directmap_mappings" would
somehow lead me to think of we are getting rid of the name "xenheap"
completely in the code, which seems a little bit scary to me...

But I just checked there is a comment
"/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory."
above the function and the declaration so I guess we are fine?

> 
> >           panic("Unable to setup the xenheap mappings.\n");
> 
> Likely, I think this wants to be s/xenheap/directmap/

Ok.

> 
> >
> >       /* Record where the xenheap is, for translation routines. */
> 
> Same here because you set directmap_virt_end.

Ok.

> 
> > -    xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> > +    directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> 
> I would be OK to keep "XENHEAP_*" for now.

Thanks for your confirmation.

> 
> >   }
> >   #else /* CONFIG_ARM_64 */
> >   void __init setup_xenheap_mappings(unsigned long base_mfn,
> > @@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned
> long base_mfn,
> >       int rc;
> >
> >       /* First call sets the xenheap physical and virtual offset. */
> 
> s/xenheap/directmap/ I haven't looked if there are other instances in
> the function.

Don't bother, I will take care of the rest.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Julien Grall]

On 07/09/2022 11:53, Henry Wang wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to
>> directmap_*
>>
>> Hi Henry,
>>
>> On 07/09/2022 09:36, Henry Wang wrote:
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index 7f5b317d3e..4a70ed2986 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -132,12 +132,12 @@ uint64_t init_ttbr;
>>>    static paddr_t phys_offset;
>>>
>>>    /* Limits of the Xen heap */
>>> -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
>>> -mfn_t xenheap_mfn_end __read_mostly;
>>> -vaddr_t xenheap_virt_end __read_mostly;
>>> +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
>>> +mfn_t directmap_mfn_end __read_mostly;
>>> +vaddr_t directmap_virt_end __read_mostly;
>>>    #ifdef CONFIG_ARM_64
>>> -vaddr_t xenheap_virt_start __read_mostly;
>>> -unsigned long xenheap_base_pdx __read_mostly;
>>> +vaddr_t directmap_virt_start __read_mostly;
>>> +unsigned long directmap_base_pdx __read_mostly;
>>>    #endif
>>>
>>>    unsigned long frametable_base_pdx __read_mostly;
>>> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned
>> long base_mfn,
>>
>> I think the function also want to be renamed to match the code below.
> 
> Hmmm, renaming the name to "setup_directmap_mappings" would
> somehow lead me to think of we are getting rid of the name "xenheap"
> completely in the code, which seems a little bit scary to me...
> 
> But I just checked there is a comment
> "/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory."
> above the function and the declaration so I guess we are fine?

We are not getting rid of "xenheap". In fact the common code will 
continue to use the concept.

What we make clear is this function is not only here to map the xenheap 
but other memory (e.g. static domain memory on arm64).

Cheers,

-- 
Julien Grall

RE: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> >>> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned
> >> long base_mfn,
> >>
> >> I think the function also want to be renamed to match the code below.
> >
> > Hmmm, renaming the name to "setup_directmap_mappings" would
> > somehow lead me to think of we are getting rid of the name "xenheap"
> > completely in the code, which seems a little bit scary to me...
> >
> > But I just checked there is a comment
> > "/* Set up the xenheap: up to 1GB of contiguous, always-mapped
> memory."
> > above the function and the declaration so I guess we are fine?
> 
> We are not getting rid of "xenheap". In fact the common code will
> continue to use the concept.

Ack.

> 
> What we make clear is this function is not only here to map the xenheap
> but other memory (e.g. static domain memory on arm64).

In that case I think the comment in function declaration (attached below)
```
/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory.
 * Base must be 32MB aligned and size a multiple of 32MB. */
extern void setup_xenheap_mappings(unsigned long base_mfn, unsigned long nr_mfns);
```
would also need changes, as I think it only refers to the Arm32.

How about
/*
 * For Arm32, set up the xenheap: up to 1GB of contiguous,
 * always-mapped memory. Base must be 32MB aligned and size
 * a multiple of 32MB.
 * For Arm64, set up the directmap area of memory.
 */

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Julien Grall]

On 07/09/2022 12:09, Henry Wang wrote:
> Hi Julien,

Hi Henry,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>>>>> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned
>>>> long base_mfn,
>>>>
>>>> I think the function also want to be renamed to match the code below.
>>>
>>> Hmmm, renaming the name to "setup_directmap_mappings" would
>>> somehow lead me to think of we are getting rid of the name "xenheap"
>>> completely in the code, which seems a little bit scary to me...
>>>
>>> But I just checked there is a comment
>>> "/* Set up the xenheap: up to 1GB of contiguous, always-mapped
>> memory."
>>> above the function and the declaration so I guess we are fine?
>>
>> We are not getting rid of "xenheap". In fact the common code will
>> continue to use the concept.
> 
> Ack.
> 
>>
>> What we make clear is this function is not only here to map the xenheap
>> but other memory (e.g. static domain memory on arm64).
> 
> In that case I think the comment in function declaration (attached below)
> ```
> /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory.
>   * Base must be 32MB aligned and size a multiple of 32MB. */
> extern void setup_xenheap_mappings(unsigned long base_mfn, unsigned long nr_mfns);
> ```
> would also need changes, as I think it only refers to the Arm32.
> 
> How about
> /*
>   * For Arm32, set up the xenheap: up to 1GB of contiguous,
>   * always-mapped memory. Base must be 32MB aligned and size
>   * a multiple of 32MB.
>   * For Arm64, set up the directmap area of memory.

One remark. I would say: "map the region in the directmap area"

Cheers,

-- 
Julien Grall

RE: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Henry Wang]

Hi Julien,

> -----Original Message-----
> > How about
> > /*
> >   * For Arm32, set up the xenheap: up to 1GB of contiguous,
> >   * always-mapped memory. Base must be 32MB aligned and size
> >   * a multiple of 32MB.
> >   * For Arm64, set up the directmap area of memory.
> 
> One remark. I would say: "map the region in the directmap area"

No problem :)) Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Julien Grall]

Hi Henry,

While reviewing the binding sent by Penny I noticed some inconsistency 
with the one you introduced. See below.

On 07/09/2022 09:36, Henry Wang wrote:
> +- xen,static-heap
> +
> +    Property under the top-level "chosen" node. It specifies the address
> +    and size of Xen static heap memory. Note that at least a 64KB
> +    alignment is required.
> +
> +- #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 static heap in device tree:
> +
> +    / {
> +        chosen {
> +            #xen,static-heap-address-cells = <0x2>;
> +            #xen,static-heap-size-cells = <0x2>;

Your binding, is introduce #xen,static-heap-{address, size}-cells 
whereas Penny's one is using #{address, size}-cells even if the property 
is not "reg".

I would like some consistency in the way we define bindings. Looking at 
the tree, we already seem to have introduced 
#xen-static-mem-address-cells. So maybe we should follow your approach?

That said, I am wondering whether we should just use one set of property 
name.

I am open to suggestion here. My only request is we are consistent (i.e. 
this doesn't depend on who wrote the bindings).

Cheers,

-- 
Julien Grall

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

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory
> 
> Hi Henry,
> 
> While reviewing the binding sent by Penny I noticed some inconsistency
> with the one you introduced. See below.
> 
> On 07/09/2022 09:36, Henry Wang wrote:
> > +- xen,static-heap
> > +
> > +    Property under the top-level "chosen" node. It specifies the address
> > +    and size of Xen static heap memory. Note that at least a 64KB
> > +    alignment is required.
> > +
> > +- #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 static heap in device tree:
> > +
> > +    / {
> > +        chosen {
> > +            #xen,static-heap-address-cells = <0x2>;
> > +            #xen,static-heap-size-cells = <0x2>;
> 
> Your binding, is introduce #xen,static-heap-{address, size}-cells
> whereas Penny's one is using #{address, size}-cells even if the property
> is not "reg".
> 
> I would like some consistency in the way we define bindings. Looking at
> the tree, we already seem to have introduced
> #xen-static-mem-address-cells. So maybe we should follow your approach?
> 
> That said, I am wondering whether we should just use one set of property
> name.

IMO now we have the pair
#xen,static-heap-{address, size}-cells and xen,static-heap for static heap.
and the pair
#xen,static-mem-{address, size}-cells and xen,static-mem for static
memory allocation for dom0less.

So at least these two are consistent.

I guess the concern you raised is related to the static shared memory for
dom0less,
...

> 
> I am open to suggestion here. My only request is we are consistent (i.e.
> this doesn't depend on who wrote the bindings).

I am not sure if Penny and Stefano have some specific requirements
regarding the static shared memory usage. So I will wait for Stefano's input.
But yeah we need to keep the consistency so if we are agreed that I need to
change the binding, I will do the corresponding change.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Bertrand Marquis]

Hi,

> On 7 Sep 2022, at 12:48, Henry Wang <Henry.Wang@arm.com> wrote:
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory
>> 
>> Hi Henry,
>> 
>> While reviewing the binding sent by Penny I noticed some inconsistency
>> with the one you introduced. See below.
>> 
>> On 07/09/2022 09:36, Henry Wang wrote:
>>> +- xen,static-heap
>>> +
>>> +    Property under the top-level "chosen" node. It specifies the address
>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>> +    alignment is required.
>>> +
>>> +- #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 static heap in device tree:
>>> +
>>> +    / {
>>> +        chosen {
>>> +            #xen,static-heap-address-cells = <0x2>;
>>> +            #xen,static-heap-size-cells = <0x2>;
>> 
>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>> whereas Penny's one is using #{address, size}-cells even if the property
>> is not "reg".
>> 
>> I would like some consistency in the way we define bindings. Looking at
>> the tree, we already seem to have introduced
>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>> 
>> That said, I am wondering whether we should just use one set of property
>> name.

The more I dig, the less I find a use case where we could need different values here.
Maybe just:
#xen,address-cells = <2>
#xen,size-cells = <2>
Could be enough. If some parameter needs a different value it could introduce a specific name.

Or maybe just memory-address-cells and memory-size-cells if we see a possibility to require a different value for an other address or size.

This would also make life easier for users as we could just always put those 2 in our examples.

Cheers
Bertrand

> 
> IMO now we have the pair
> #xen,static-heap-{address, size}-cells and xen,static-heap for static heap.
> and the pair
> #xen,static-mem-{address, size}-cells and xen,static-mem for static
> memory allocation for dom0less.
> 
> So at least these two are consistent.
> 
> I guess the concern you raised is related to the static shared memory for
> dom0less,
> ...
> 
>> 
>> I am open to suggestion here. My only request is we are consistent (i.e.
>> this doesn't depend on who wrote the bindings).
> 
> I am not sure if Penny and Stefano have some specific requirements
> regarding the static shared memory usage. So I will wait for Stefano's input.
> But yeah we need to keep the consistency so if we are agreed that I need to
> change the binding, I will do the corresponding change.
> 
> Kind regards,
> Henry
> 
>> 
>> Cheers,
>> 
>> --
>> Julien Grall

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

[Michal Orzel]

Hi Julien,

On 07/09/2022 13:36, Julien Grall wrote:
> 
> Hi Henry,
> 
> While reviewing the binding sent by Penny I noticed some inconsistency
> with the one you introduced. See below.
> 
> On 07/09/2022 09:36, Henry Wang wrote:
>> +- xen,static-heap
>> +
>> +    Property under the top-level "chosen" node. It specifies the address
>> +    and size of Xen static heap memory. Note that at least a 64KB
>> +    alignment is required.
>> +
>> +- #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 static heap in device tree:
>> +
>> +    / {
>> +        chosen {
>> +            #xen,static-heap-address-cells = <0x2>;
>> +            #xen,static-heap-size-cells = <0x2>;
> 
> Your binding, is introduce #xen,static-heap-{address, size}-cells
> whereas Penny's one is using #{address, size}-cells even if the property
> is not "reg".
> 
> I would like some consistency in the way we define bindings. Looking at
> the tree, we already seem to have introduced
> #xen-static-mem-address-cells. So maybe we should follow your approach?
> 
> That said, I am wondering whether we should just use one set of property
> name.
> 
> I am open to suggestion here. My only request is we are consistent (i.e.
> this doesn't depend on who wrote the bindings).
> 
In my opinion we should follow the device tree specification which states
that the #address-cells and #size-cells correspond to the reg property.
This would mean that for all the custom properties we introduce we need
custom address and size properties (just like for static-mem/static-heap).

~Michal

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

[Julien Grall]

On 07/09/2022 13:12, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 07/09/2022 13:36, Julien Grall wrote:
>>
>> Hi Henry,
>>
>> While reviewing the binding sent by Penny I noticed some inconsistency
>> with the one you introduced. See below.
>>
>> On 07/09/2022 09:36, Henry Wang wrote:
>>> +- xen,static-heap
>>> +
>>> +    Property under the top-level "chosen" node. It specifies the address
>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>> +    alignment is required.
>>> +
>>> +- #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 static heap in device tree:
>>> +
>>> +    / {
>>> +        chosen {
>>> +            #xen,static-heap-address-cells = <0x2>;
>>> +            #xen,static-heap-size-cells = <0x2>;
>>
>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>> whereas Penny's one is using #{address, size}-cells even if the property
>> is not "reg".
>>
>> I would like some consistency in the way we define bindings. Looking at
>> the tree, we already seem to have introduced
>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>
>> That said, I am wondering whether we should just use one set of property
>> name.
>>
>> I am open to suggestion here. My only request is we are consistent (i.e.
>> this doesn't depend on who wrote the bindings).
>>
> In my opinion we should follow the device tree specification which states
> that the #address-cells and #size-cells correspond to the reg property.

Hmmm.... Looking at [1], the two properties are not exclusive to 'reg' 
Furthermore, I am not aware of any restriction for us to re-use them. Do 
you have a pointer?

Cheers,

[1] https://elinux.org/Device_Tree_Mysteries#.23xxx-cells_property_name

> 
> ~Michal

-- 
Julien Grall

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

[Julien Grall]

On 07/09/2022 13:12, Bertrand Marquis wrote:
> Hi,

Hi Bertrand,

>> On 7 Sep 2022, at 12:48, Henry Wang <Henry.Wang@arm.com> wrote:
>>
>> Hi Julien,
>>
>>> -----Original Message-----
>>> From: Julien Grall <julien@xen.org>
>>> Subject: Re: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory
>>>
>>> Hi Henry,
>>>
>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>> with the one you introduced. See below.
>>>
>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>> +- xen,static-heap
>>>> +
>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>> +    alignment is required.
>>>> +
>>>> +- #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 static heap in device tree:
>>>> +
>>>> +    / {
>>>> +        chosen {
>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>
>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>> whereas Penny's one is using #{address, size}-cells even if the property
>>> is not "reg".
>>>
>>> I would like some consistency in the way we define bindings. Looking at
>>> the tree, we already seem to have introduced
>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>
>>> That said, I am wondering whether we should just use one set of property
>>> name.
> 
> The more I dig, the less I find a use case where we could need different values here.

This is what I thought as well..

> Maybe just:
> #xen,address-cells = <2>
> #xen,size-cells = <2>
> Could be enough. If some parameter needs a different value it could introduce a specific name.

I think '#xen,...' is ambiguous because it doesn't tell you whether it 
applies to the memory range or interrupt range. So I would got with

> 
> Or maybe just memory-address-cells and memory-size-cells if we see a possibility to require a different value for an other address or size.

"#xen,memory-*".

That said, any reason to not reuse #address-cells and #size-cells here?

Cheers,

-- 
Julien Grall

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

[Michal Orzel]

On 07/09/2022 14:32, Julien Grall wrote:
> [CAUTION: External Email]
> 
> On 07/09/2022 13:12, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 07/09/2022 13:36, Julien Grall wrote:
>>>
>>> Hi Henry,
>>>
>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>> with the one you introduced. See below.
>>>
>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>> +- xen,static-heap
>>>> +
>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>> +    alignment is required.
>>>> +
>>>> +- #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 static heap in device tree:
>>>> +
>>>> +    / {
>>>> +        chosen {
>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>
>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>> whereas Penny's one is using #{address, size}-cells even if the property
>>> is not "reg".
>>>
>>> I would like some consistency in the way we define bindings. Looking at
>>> the tree, we already seem to have introduced
>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>
>>> That said, I am wondering whether we should just use one set of property
>>> name.
>>>
>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>> this doesn't depend on who wrote the bindings).
>>>
>> In my opinion we should follow the device tree specification which states
>> that the #address-cells and #size-cells correspond to the reg property.
> 
> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
> Furthermore, I am not aware of any restriction for us to re-use them. Do
> you have a pointer?

As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
I took this info from the latest device tree specs found under https://www.devicetree.org/specifications/:
"The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
and
"The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"

> 
> Cheers,
> 
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felinux.org%2FDevice_Tree_Mysteries%23.23xxx-cells_property_name&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C40290431f16748808b6308da90ccfc53%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981507324472512%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=okN60ULg2Dx3cnlA5vPLMR%2F8QAKnbGmBpz7goXb5usw%3D&amp;reserved=0
> 
>>
>> ~Michal
> 
> --
> Julien Grall
> 

~Michal

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

[Julien Grall]

On 07/09/2022 13:41, Michal Orzel wrote:
> 
> 
> On 07/09/2022 14:32, Julien Grall wrote:
>> [CAUTION: External Email]
>>
>> On 07/09/2022 13:12, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>
>>>> Hi Henry,
>>>>
>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>> with the one you introduced. See below.
>>>>
>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>> +- xen,static-heap
>>>>> +
>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>> +    alignment is required.
>>>>> +
>>>>> +- #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 static heap in device tree:
>>>>> +
>>>>> +    / {
>>>>> +        chosen {
>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>
>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>> is not "reg".
>>>>
>>>> I would like some consistency in the way we define bindings. Looking at
>>>> the tree, we already seem to have introduced
>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>
>>>> That said, I am wondering whether we should just use one set of property
>>>> name.
>>>>
>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>> this doesn't depend on who wrote the bindings).
>>>>
>>> In my opinion we should follow the device tree specification which states
>>> that the #address-cells and #size-cells correspond to the reg property.
>>
>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>> you have a pointer?
> 
> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
> I took this info from the latest device tree specs found under https://www.devicetree.org/specifications/:
> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
> and
> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"

Right. But there is nothing in the wording suggesting that 
#address-cells and #size-cells can't be re-used. From [1], it is clear 
that the meaning has changed.

So why can't we do the same?

Cheers,

-- 
Julien Grall

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

[Bertrand Marquis]

Hi,

> On 7 Sep 2022, at 13:45, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 07/09/2022 13:41, Michal Orzel wrote:
>> On 07/09/2022 14:32, Julien Grall wrote:
>>> [CAUTION: External Email]
>>> 
>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>> Hi Julien,
>>> 
>>> Hi Michal,
>>> 
>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>> 
>>>>> Hi Henry,
>>>>> 
>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>> with the one you introduced. See below.
>>>>> 
>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>> +- xen,static-heap
>>>>>> +
>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>> +    alignment is required.
>>>>>> +
>>>>>> +- #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 static heap in device tree:
>>>>>> +
>>>>>> +    / {
>>>>>> +        chosen {
>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>> 
>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>> is not "reg".
>>>>> 
>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>> the tree, we already seem to have introduced
>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>> 
>>>>> That said, I am wondering whether we should just use one set of property
>>>>> name.
>>>>> 
>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>> this doesn't depend on who wrote the bindings).
>>>>> 
>>>> In my opinion we should follow the device tree specification which states
>>>> that the #address-cells and #size-cells correspond to the reg property.
>>> 
>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>> you have a pointer?
>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>> I took this info from the latest device tree specs found under https://www.devicetree.org/specifications/:
>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>> and
>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
> 
> Right. But there is nothing in the wording suggesting that #address-cells and #size-cells can't be re-used. From [1], it is clear that the meaning has changed.
> 
> So why can't we do the same?

I agree here, those are used for how reg is encoded but nothing says that we cannot reuse them for the encoding of something else.
Even if we do not use “reg” for those sets, they are still defining memory addresses and sizes which is coherent.

In some cases reg is used to encode something different so those could have different values that we could not use but for the chosen node, they should always set the encoding for something addressing a memory area.

So if we have a good reason then I would vote for xen,memory-* proposal but so far I could not find a reason not to use the standard ones.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

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

[Michal Orzel]

On 07/09/2022 14:45, Julien Grall wrote:
> 
> On 07/09/2022 13:41, Michal Orzel wrote:
>>
>>
>> On 07/09/2022 14:32, Julien Grall wrote:
>>> [CAUTION: External Email]
>>>
>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>> Hi Julien,
>>>
>>> Hi Michal,
>>>
>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>
>>>>> Hi Henry,
>>>>>
>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>> with the one you introduced. See below.
>>>>>
>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>> +- xen,static-heap
>>>>>> +
>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>> +    alignment is required.
>>>>>> +
>>>>>> +- #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 static heap in device tree:
>>>>>> +
>>>>>> +    / {
>>>>>> +        chosen {
>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>
>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>> is not "reg".
>>>>>
>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>> the tree, we already seem to have introduced
>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>
>>>>> That said, I am wondering whether we should just use one set of property
>>>>> name.
>>>>>
>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>> this doesn't depend on who wrote the bindings).
>>>>>
>>>> In my opinion we should follow the device tree specification which states
>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>
>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>> you have a pointer?
>>
>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>> I took this info from the latest device tree specs found under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C4f35e9f93b7443ac73c808da90cecc22%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981515122993111%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TiESYS6RXdiPLX8WFUV0CsztAvK7mHSud%2B0xoJqwAw0%3D&amp;reserved=0:
>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>> and
>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
> 
> Right. But there is nothing in the wording suggesting that
> #address-cells and #size-cells can't be re-used. From [1], it is clear
> that the meaning has changed.
> 
> So why can't we do the same?
I think this is a matter of how someone reads these sentences.
I do not think that such documents need to state:
"This property is for the reg. Do not use it for other purposes."
The first part of the sentence is enough to inform what is supported.

On the other hand, looking at [1] these properties got new purposes
so I think we could do the same. Now the question is whether we want that.
I think it is doable to just have a single pair of #address/#size properties.
For instance xen,shared-mem requiring just 0x1 for address/size
and reg requiring 0x2. This would just imply putting additional 0x00.

> 
> Cheers,
> 
> --
> Julien Grall

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

[Bertrand Marquis]

Hi Michal,

> On 7 Sep 2022, at 14:09, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> On 07/09/2022 14:45, Julien Grall wrote:
>> 
>> On 07/09/2022 13:41, Michal Orzel wrote:
>>> 
>>> 
>>> On 07/09/2022 14:32, Julien Grall wrote:
>>>> [CAUTION: External Email]
>>>> 
>>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>>> Hi Julien,
>>>> 
>>>> Hi Michal,
>>>> 
>>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>> 
>>>>>> Hi Henry,
>>>>>> 
>>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>>> with the one you introduced. See below.
>>>>>> 
>>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>>> +- xen,static-heap
>>>>>>> +
>>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>>> +    alignment is required.
>>>>>>> +
>>>>>>> +- #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 static heap in device tree:
>>>>>>> +
>>>>>>> +    / {
>>>>>>> +        chosen {
>>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>> 
>>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>>> is not "reg".
>>>>>> 
>>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>>> the tree, we already seem to have introduced
>>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>> 
>>>>>> That said, I am wondering whether we should just use one set of property
>>>>>> name.
>>>>>> 
>>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>>> this doesn't depend on who wrote the bindings).
>>>>>> 
>>>>> In my opinion we should follow the device tree specification which states
>>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>> 
>>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>>> you have a pointer?
>>> 
>>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>>> I took this info from the latest device tree specs found under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C4f35e9f93b7443ac73c808da90cecc22%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981515122993111%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TiESYS6RXdiPLX8WFUV0CsztAvK7mHSud%2B0xoJqwAw0%3D&amp;reserved=0:
>>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>>> and
>>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
>> 
>> Right. But there is nothing in the wording suggesting that
>> #address-cells and #size-cells can't be re-used. From [1], it is clear
>> that the meaning has changed.
>> 
>> So why can't we do the same?
> I think this is a matter of how someone reads these sentences.
> I do not think that such documents need to state:
> "This property is for the reg. Do not use it for other purposes."
> The first part of the sentence is enough to inform what is supported.
> 
> On the other hand, looking at [1] these properties got new purposes
> so I think we could do the same. Now the question is whether we want that.
> I think it is doable to just have a single pair of #address/#size properties.
> For instance xen,shared-mem requiring just 0x1 for address/size
> and reg requiring 0x2. This would just imply putting additional 0x00.

I think we want in general to reduce complexity when possible.
Here we are adding a lot of entries in the device tree where we know that
in all cases having only 2 will work all the time.

I am not convinced by the arguments on not using #address-cells and will
leave that one to Stefano

But in any case we should only add one pair here for sure, as you say the
only implication is to add a couple of 0 in the worst case.

Cheers
Bertrand

> 
>> 
>> Cheers,
>> 
>> --
>> Julien Grall

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

[Michal Orzel]

On 07/09/2022 15:28, Bertrand Marquis wrote:
> 
> Hi Michal,
> 
>> On 7 Sep 2022, at 14:09, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>>
>> On 07/09/2022 14:45, Julien Grall wrote:
>>>
>>> On 07/09/2022 13:41, Michal Orzel wrote:
>>>>
>>>>
>>>> On 07/09/2022 14:32, Julien Grall wrote:
>>>>> [CAUTION: External Email]
>>>>>
>>>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>>>> Hi Julien,
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>>>
>>>>>>> Hi Henry,
>>>>>>>
>>>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>>>> with the one you introduced. See below.
>>>>>>>
>>>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>>>> +- xen,static-heap
>>>>>>>> +
>>>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>>>> +    alignment is required.
>>>>>>>> +
>>>>>>>> +- #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 static heap in device tree:
>>>>>>>> +
>>>>>>>> +    / {
>>>>>>>> +        chosen {
>>>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>>>
>>>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>>>> is not "reg".
>>>>>>>
>>>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>>>> the tree, we already seem to have introduced
>>>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>>>
>>>>>>> That said, I am wondering whether we should just use one set of property
>>>>>>> name.
>>>>>>>
>>>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>>>> this doesn't depend on who wrote the bindings).
>>>>>>>
>>>>>> In my opinion we should follow the device tree specification which states
>>>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>>>
>>>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>>>> you have a pointer?
>>>>
>>>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>>>> I took this info from the latest device tree specs found under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C83da1eb9d32441cb9e8108da90d4f2d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981541539851438%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=3M9aT3LjCEOhZUHWSbgSSmKppY1Wion4TT3BeKLnWSo%3D&amp;reserved=0:
>>>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>>>> and
>>>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
>>>
>>> Right. But there is nothing in the wording suggesting that
>>> #address-cells and #size-cells can't be re-used. From [1], it is clear
>>> that the meaning has changed.
>>>
>>> So why can't we do the same?
>> I think this is a matter of how someone reads these sentences.
>> I do not think that such documents need to state:
>> "This property is for the reg. Do not use it for other purposes."
>> The first part of the sentence is enough to inform what is supported.
>>
>> On the other hand, looking at [1] these properties got new purposes
>> so I think we could do the same. Now the question is whether we want that.
>> I think it is doable to just have a single pair of #address/#size properties.
>> For instance xen,shared-mem requiring just 0x1 for address/size
>> and reg requiring 0x2. This would just imply putting additional 0x00.
> 
> I think we want in general to reduce complexity when possible.
> Here we are adding a lot of entries in the device tree where we know that
> in all cases having only 2 will work all the time.
> 
> I am not convinced by the arguments on not using #address-cells and will
> leave that one to Stefano
> 
> But in any case we should only add one pair here for sure, as you say the
> only implication is to add a couple of 0 in the worst case.
I agree. The only drawback is the need to modify the already introduced properties
to be coherent.

> 
> Cheers
> Bertrand
> 
>>
>>>
>>> Cheers,
>>>
>>> --
>>> Julien Grall
> 

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

[Bertrand Marquis]

> On 7 Sep 2022, at 14:31, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 07/09/2022 15:28, Bertrand Marquis wrote:
>> 
>> Hi Michal,
>> 
>>> On 7 Sep 2022, at 14:09, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> 
>>> On 07/09/2022 14:45, Julien Grall wrote:
>>>> 
>>>> On 07/09/2022 13:41, Michal Orzel wrote:
>>>>> 
>>>>> 
>>>>> On 07/09/2022 14:32, Julien Grall wrote:
>>>>>> [CAUTION: External Email]
>>>>>> 
>>>>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>>>>> Hi Julien,
>>>>>> 
>>>>>> Hi Michal,
>>>>>> 
>>>>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>>>> 
>>>>>>>> Hi Henry,
>>>>>>>> 
>>>>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>>>>> with the one you introduced. See below.
>>>>>>>> 
>>>>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>>>>> +- xen,static-heap
>>>>>>>>> +
>>>>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>>>>> +    alignment is required.
>>>>>>>>> +
>>>>>>>>> +- #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 static heap in device tree:
>>>>>>>>> +
>>>>>>>>> +    / {
>>>>>>>>> +        chosen {
>>>>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>>>> 
>>>>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>>>>> is not "reg".
>>>>>>>> 
>>>>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>>>>> the tree, we already seem to have introduced
>>>>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>>>> 
>>>>>>>> That said, I am wondering whether we should just use one set of property
>>>>>>>> name.
>>>>>>>> 
>>>>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>>>>> this doesn't depend on who wrote the bindings).
>>>>>>>> 
>>>>>>> In my opinion we should follow the device tree specification which states
>>>>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>>>> 
>>>>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>>>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>>>>> you have a pointer?
>>>>> 
>>>>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>>>>> I took this info from the latest device tree specs found under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C83da1eb9d32441cb9e8108da90d4f2d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981541539851438%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=3M9aT3LjCEOhZUHWSbgSSmKppY1Wion4TT3BeKLnWSo%3D&amp;reserved=0:
>>>>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>>>>> and
>>>>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
>>>> 
>>>> Right. But there is nothing in the wording suggesting that
>>>> #address-cells and #size-cells can't be re-used. From [1], it is clear
>>>> that the meaning has changed.
>>>> 
>>>> So why can't we do the same?
>>> I think this is a matter of how someone reads these sentences.
>>> I do not think that such documents need to state:
>>> "This property is for the reg. Do not use it for other purposes."
>>> The first part of the sentence is enough to inform what is supported.
>>> 
>>> On the other hand, looking at [1] these properties got new purposes
>>> so I think we could do the same. Now the question is whether we want that.
>>> I think it is doable to just have a single pair of #address/#size properties.
>>> For instance xen,shared-mem requiring just 0x1 for address/size
>>> and reg requiring 0x2. This would just imply putting additional 0x00.
>> 
>> I think we want in general to reduce complexity when possible.
>> Here we are adding a lot of entries in the device tree where we know that
>> in all cases having only 2 will work all the time.
>> 
>> I am not convinced by the arguments on not using #address-cells and will
>> leave that one to Stefano
>> 
>> But in any case we should only add one pair here for sure, as you say the
>> only implication is to add a couple of 0 in the worst case.
> I agree. The only drawback is the need to modify the already introduced properties
> to be coherent.

Agree, someone will need to do a pass on the whole doc which might be easier with all things in.

Cheers
Bertrand

> 
>> 
>> Cheers
>> Bertrand
>> 
>>> 
>>>> 
>>>> Cheers,
>>>> 
>>>> --
>>>> Julien Grall

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

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> > I am not convinced by the arguments on not using #address-cells and will
> > leave that one to Stefano
> >
> > But in any case we should only add one pair here for sure, as you say the
> > only implication is to add a couple of 0 in the worst case.
> I agree. The only drawback is the need to modify the already introduced
> properties
> to be coherent.

You mean the #xen,static-mem-address/size-cells? I think this is the only one
existing. I can add another prerequisite patch in my series after we reach an
agreement.

Kind regards,
Henry 

> 
> >
> > Cheers
> > Bertrand
> >
> >>
> >>>
> >>> Cheers,
> >>>
> >>> --
> >>> Julien Grall
> >

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

[Michal Orzel]

On 07/09/2022 15:33, Bertrand Marquis wrote:
> 
>> On 7 Sep 2022, at 14:31, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 07/09/2022 15:28, Bertrand Marquis wrote:
>>>
>>> Hi Michal,
>>>
>>>> On 7 Sep 2022, at 14:09, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>
>>>>
>>>> On 07/09/2022 14:45, Julien Grall wrote:
>>>>>
>>>>> On 07/09/2022 13:41, Michal Orzel wrote:
>>>>>>
>>>>>>
>>>>>> On 07/09/2022 14:32, Julien Grall wrote:
>>>>>>> [CAUTION: External Email]
>>>>>>>
>>>>>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>>>>>
>>>>>>>>> Hi Henry,
>>>>>>>>>
>>>>>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>>>>>> with the one you introduced. See below.
>>>>>>>>>
>>>>>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>>>>>> +- xen,static-heap
>>>>>>>>>> +
>>>>>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>>>>>> +    alignment is required.
>>>>>>>>>> +
>>>>>>>>>> +- #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 static heap in device tree:
>>>>>>>>>> +
>>>>>>>>>> +    / {
>>>>>>>>>> +        chosen {
>>>>>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>>>>>
>>>>>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>>>>>> is not "reg".
>>>>>>>>>
>>>>>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>>>>>> the tree, we already seem to have introduced
>>>>>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>>>>>
>>>>>>>>> That said, I am wondering whether we should just use one set of property
>>>>>>>>> name.
>>>>>>>>>
>>>>>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>>>>>> this doesn't depend on who wrote the bindings).
>>>>>>>>>
>>>>>>>> In my opinion we should follow the device tree specification which states
>>>>>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>>>>>
>>>>>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>>>>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>>>>>> you have a pointer?
>>>>>>
>>>>>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>>>>>> I took this info from the latest device tree specs found under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7Cc677a7983cd94e48620708da90d5a15f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981544487489692%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=1uwtf%2F6shf2PiKu0XZPFNQ%2B6iyhLrMsYb1XEru3IGlg%3D&amp;reserved=0:
>>>>>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>>>>>> and
>>>>>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
>>>>>
>>>>> Right. But there is nothing in the wording suggesting that
>>>>> #address-cells and #size-cells can't be re-used. From [1], it is clear
>>>>> that the meaning has changed.
>>>>>
>>>>> So why can't we do the same?
>>>> I think this is a matter of how someone reads these sentences.
>>>> I do not think that such documents need to state:
>>>> "This property is for the reg. Do not use it for other purposes."
>>>> The first part of the sentence is enough to inform what is supported.
>>>>
>>>> On the other hand, looking at [1] these properties got new purposes
>>>> so I think we could do the same. Now the question is whether we want that.
>>>> I think it is doable to just have a single pair of #address/#size properties.
>>>> For instance xen,shared-mem requiring just 0x1 for address/size
>>>> and reg requiring 0x2. This would just imply putting additional 0x00.
>>>
>>> I think we want in general to reduce complexity when possible.
>>> Here we are adding a lot of entries in the device tree where we know that
>>> in all cases having only 2 will work all the time.
>>>
>>> I am not convinced by the arguments on not using #address-cells and will
>>> leave that one to Stefano
>>>
>>> But in any case we should only add one pair here for sure, as you say the
>>> only implication is to add a couple of 0 in the worst case.
>> I agree. The only drawback is the need to modify the already introduced properties
>> to be coherent.
> 
> Agree, someone will need to do a pass on the whole doc which might be easier with all things in.
> 
Well, not only docs. If we decide to use a single pair of #address-cells and #size-cells, then
we need to modify the code that expects different properties e.g. xen,static-mem-{address/size}-cells.

> Cheers
> Bertrand
> 
>>
>>>
>>> Cheers
>>> Bertrand
>>>
>>>>
>>>>>
>>>>> Cheers,
>>>>>
>>>>> --
>>>>> Julien Grall
> 

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

[Bertrand Marquis]

Hi Henry,

> On 7 Sep 2022, at 14:35, Henry Wang <Henry.Wang@arm.com> wrote:
> 
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>>> I am not convinced by the arguments on not using #address-cells and will
>>> leave that one to Stefano
>>> 
>>> But in any case we should only add one pair here for sure, as you say the
>>> only implication is to add a couple of 0 in the worst case.
>> I agree. The only drawback is the need to modify the already introduced
>> properties
>> to be coherent.
> 
> You mean the #xen,static-mem-address/size-cells? I think this is the only one
> existing. I can add another prerequisite patch in my series after we reach an
> agreement.

No the standard device tree ones:
#address-cells
#size-cells

Those have a default value if unspecified.

Bertrand

> 
> Kind regards,
> Henry
> 
>> 
>>> 
>>> Cheers
>>> Bertrand
>>> 
>>>> 
>>>>> 
>>>>> Cheers,
>>>>> 
>>>>> --
>>>>> Julien Grall
>>> 

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

[Bertrand Marquis]

Hi,

> On 7 Sep 2022, at 14:37, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 07/09/2022 15:33, Bertrand Marquis wrote:
>> 
>>> On 7 Sep 2022, at 14:31, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 07/09/2022 15:28, Bertrand Marquis wrote:
>>>> 
>>>> Hi Michal,
>>>> 
>>>>> On 7 Sep 2022, at 14:09, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>> 
>>>>> 
>>>>> On 07/09/2022 14:45, Julien Grall wrote:
>>>>>> 
>>>>>> On 07/09/2022 13:41, Michal Orzel wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> On 07/09/2022 14:32, Julien Grall wrote:
>>>>>>>> [CAUTION: External Email]
>>>>>>>> 
>>>>>>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>>>>>>> Hi Julien,
>>>>>>>> 
>>>>>>>> Hi Michal,
>>>>>>>> 
>>>>>>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>>>>>> 
>>>>>>>>>> Hi Henry,
>>>>>>>>>> 
>>>>>>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>>>>>>> with the one you introduced. See below.
>>>>>>>>>> 
>>>>>>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>>>>>>> +- xen,static-heap
>>>>>>>>>>> +
>>>>>>>>>>> +    Property under the top-level "chosen" node. It specifies the address
>>>>>>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>>>>>>> +    alignment is required.
>>>>>>>>>>> +
>>>>>>>>>>> +- #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 static heap in device tree:
>>>>>>>>>>> +
>>>>>>>>>>> +    / {
>>>>>>>>>>> +        chosen {
>>>>>>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>>>>>> 
>>>>>>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>>>>>>> whereas Penny's one is using #{address, size}-cells even if the property
>>>>>>>>>> is not "reg".
>>>>>>>>>> 
>>>>>>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>>>>>>> the tree, we already seem to have introduced
>>>>>>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>>>>>> 
>>>>>>>>>> That said, I am wondering whether we should just use one set of property
>>>>>>>>>> name.
>>>>>>>>>> 
>>>>>>>>>> I am open to suggestion here. My only request is we are consistent (i.e.
>>>>>>>>>> this doesn't depend on who wrote the bindings).
>>>>>>>>>> 
>>>>>>>>> In my opinion we should follow the device tree specification which states
>>>>>>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>>>>>> 
>>>>>>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>>>>>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>>>>>>> you have a pointer?
>>>>>>> 
>>>>>>> As we are discussing re-usage of #address-cells and #size-cells for custom properties that are not "reg",
>>>>>>> I took this info from the latest device tree specs found under https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7Cc677a7983cd94e48620708da90d5a15f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981544487489692%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=1uwtf%2F6shf2PiKu0XZPFNQ%2B6iyhLrMsYb1XEru3IGlg%3D&amp;reserved=0:
>>>>>>> "The #address-cells property defines the number of <u32> cells used to encode the address field in a child node's reg property"
>>>>>>> and
>>>>>>> "The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property"
>>>>>> 
>>>>>> Right. But there is nothing in the wording suggesting that
>>>>>> #address-cells and #size-cells can't be re-used. From [1], it is clear
>>>>>> that the meaning has changed.
>>>>>> 
>>>>>> So why can't we do the same?
>>>>> I think this is a matter of how someone reads these sentences.
>>>>> I do not think that such documents need to state:
>>>>> "This property is for the reg. Do not use it for other purposes."
>>>>> The first part of the sentence is enough to inform what is supported.
>>>>> 
>>>>> On the other hand, looking at [1] these properties got new purposes
>>>>> so I think we could do the same. Now the question is whether we want that.
>>>>> I think it is doable to just have a single pair of #address/#size properties.
>>>>> For instance xen,shared-mem requiring just 0x1 for address/size
>>>>> and reg requiring 0x2. This would just imply putting additional 0x00.
>>>> 
>>>> I think we want in general to reduce complexity when possible.
>>>> Here we are adding a lot of entries in the device tree where we know that
>>>> in all cases having only 2 will work all the time.
>>>> 
>>>> I am not convinced by the arguments on not using #address-cells and will
>>>> leave that one to Stefano
>>>> 
>>>> But in any case we should only add one pair here for sure, as you say the
>>>> only implication is to add a couple of 0 in the worst case.
>>> I agree. The only drawback is the need to modify the already introduced properties
>>> to be coherent.
>> 
>> Agree, someone will need to do a pass on the whole doc which might be easier with all things in.
>> 
> Well, not only docs. If we decide to use a single pair of #address-cells and #size-cells, then
> we need to modify the code that expects different properties e.g. xen,static-mem-{address/size}-cells.

Right I forgot that some parts are already in.
So we will need an extra patch to handle those.

Bertrand

> 
>> Cheers
>> Bertrand
>> 
>>> 
>>>> 
>>>> Cheers
>>>> Bertrand
>>>> 
>>>>> 
>>>>>> 
>>>>>> Cheers,
>>>>>> 
>>>>>> --
>>>>>> Julien Grall

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

[Henry Wang]

Hi Bertrand and Michal,

I don't want to spam the email so I just reply here...

> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> >>>> But in any case we should only add one pair here for sure, as you say
> the
> >>>> only implication is to add a couple of 0 in the worst case.
> >>> I agree. The only drawback is the need to modify the already introduced
> properties
> >>> to be coherent.
> >>
> >> Agree, someone will need to do a pass on the whole doc which might be
> easier with all things in.
> >>
> > Well, not only docs. If we decide to use a single pair of #address-cells and
> #size-cells, then
> > we need to modify the code that expects different properties e.g.
> xen,static-mem-{address/size}-cells.
> 
> Right I forgot that some parts are already in.
> So we will need an extra patch to handle those.

I think I've addressed all comments from Julien regarding my series,
so I think I've got some bandwidth to do the clean-up patch tomorrow
after the agreement, unless someone would like to do it himself?

Kind regards,
Henry

> 
> Bertrand
> 
> >
> >> Cheers
> >> Bertrand
> >>
> >>>
> >>>>
> >>>> Cheers
> >>>> Bertrand
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Cheers,
> >>>>>>
> >>>>>> --
> >>>>>> Julien Grall
> 

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

[Bertrand Marquis]

Hi Henry,

> On 7 Sep 2022, at 14:49, Henry Wang <Henry.Wang@arm.com> wrote:
> 
> Hi Bertrand and Michal,
> 
> I don't want to spam the email so I just reply here...
> 
>> -----Original Message-----
>> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
>>>>>> But in any case we should only add one pair here for sure, as you say
>> the
>>>>>> only implication is to add a couple of 0 in the worst case.
>>>>> I agree. The only drawback is the need to modify the already introduced
>> properties
>>>>> to be coherent.
>>>> 
>>>> Agree, someone will need to do a pass on the whole doc which might be
>> easier with all things in.
>>>> 
>>> Well, not only docs. If we decide to use a single pair of #address-cells and
>> #size-cells, then
>>> we need to modify the code that expects different properties e.g.
>> xen,static-mem-{address/size}-cells.
>> 
>> Right I forgot that some parts are already in.
>> So we will need an extra patch to handle those.
> 
> I think I've addressed all comments from Julien regarding my series,
> so I think I've got some bandwidth to do the clean-up patch tomorrow
> after the agreement, unless someone would like to do it himself?

This is very nice of you.
We need to confirm first if something is actually needed or not so we need Stefano’s view here.
If so then please do it :-)

Cheers
Bertrand

> 
> Kind regards,
> Henry
> 
>> 
>> Bertrand
>> 
>>> 
>>>> Cheers
>>>> Bertrand
>>>> 
>>>>> 
>>>>>> 
>>>>>> Cheers
>>>>>> Bertrand
>>>>>> 
>>>>>>> 
>>>>>>>> 
>>>>>>>> Cheers,
>>>>>>>> 
>>>>>>>> --
>>>>>>>> Julien Grall

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

[Julien Grall]

Hi Henry,

On 07/09/2022 14:49, Henry Wang wrote:
>> -----Original Message-----
>> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
>>>>>> But in any case we should only add one pair here for sure, as you say
>> the
>>>>>> only implication is to add a couple of 0 in the worst case.
>>>>> I agree. The only drawback is the need to modify the already introduced
>> properties
>>>>> to be coherent.
>>>>
>>>> Agree, someone will need to do a pass on the whole doc which might be
>> easier with all things in.
>>>>
>>> Well, not only docs. If we decide to use a single pair of #address-cells and
>> #size-cells, then
>>> we need to modify the code that expects different properties e.g.
>> xen,static-mem-{address/size}-cells.
>>
>> Right I forgot that some parts are already in.
>> So we will need an extra patch to handle those.
> 
> I think I've addressed all comments from Julien regarding my series,

If it is not too late for you would you be able to resend your series 
without the 'address-cells'/'size-cells' change? This will give me the 
opportunity to have an other review today.

> so I think I've got some bandwidth to do the clean-up patch tomorrow
> after the agreement, unless someone would like to do it himself?

Renaming "xen,static-mem-..." is a bit tricky because they have been 
defined in Xen 4.16.

I couldn't find any support statement specific to the static memory 
feature. So it would technically fall under the "dom0less" section which 
is security supported.

That said, I don't think we can consider that the static memory feature 
is even supported because, until yesterday, the code wasn't properly 
handling request to balloon in/out. So I would view this is a tech 
preview (Could someone send a patch to clarify SUPPORT.MD)?

This would mean that would be that we could consider the binding 
unstable and we could do a straight renaming. That said, I can 
understand this may be undesirable.

If that's the case then we would need to keep the current binding as-is. 
So we would have two options:
   1) Provide a new compatible so #address-cells #size-cells can be 
used. The current binding can be deprecated
   2) Leave as-is and accept the difference

I don't have a strong opinion on which way to go. Whichever, it would be 
good to write down the rationale in the commit message of the "future" 
patch.

I would not block this series on the renaming for existing property 
(what matter is the new ones are consistent with the discussion). The 
renaming could be done afterwards. I would even say post the feature 
freeze on Friday because this could be considered as a bug fix (assuming 
you agree as the release manager :)).

Cheers,

-- 
Julien Grall

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

[Julien Grall]

On 07/09/2022 15:04, Julien Grall wrote:
> Hi Henry,
> 
> On 07/09/2022 14:49, Henry Wang wrote:
>>> -----Original Message-----
>>> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
>>>>>>> But in any case we should only add one pair here for sure, as you 
>>>>>>> say
>>> the
>>>>>>> only implication is to add a couple of 0 in the worst case.
>>>>>> I agree. The only drawback is the need to modify the already 
>>>>>> introduced
>>> properties
>>>>>> to be coherent.
>>>>>
>>>>> Agree, someone will need to do a pass on the whole doc which might be
>>> easier with all things in.
>>>>>
>>>> Well, not only docs. If we decide to use a single pair of 
>>>> #address-cells and
>>> #size-cells, then
>>>> we need to modify the code that expects different properties e.g.
>>> xen,static-mem-{address/size}-cells.
>>>
>>> Right I forgot that some parts are already in.
>>> So we will need an extra patch to handle those.
>>
>> I think I've addressed all comments from Julien regarding my series,
> 
> If it is not too late for you would you be able to resend your series 
> without the 'address-cells'/'size-cells' change? This will give me the 
> opportunity to have an other review today.
> 
>> so I think I've got some bandwidth to do the clean-up patch tomorrow
>> after the agreement, unless someone would like to do it himself?
> 
> Renaming "xen,static-mem-..." is a bit tricky because they have been 
> defined in Xen 4.16.
> 
> I couldn't find any support statement specific to the static memory 
> feature. So it would technically fall under the "dom0less" section which 
> is security supported.
> 
> That said, I don't think we can consider that the static memory feature 
> is even supported because, until yesterday, the code wasn't properly 
> handling request to balloon in/out. So I would view this is a tech 
> preview (Could someone send a patch to clarify SUPPORT.MD)?

Ah. I was using the 4.16 branch. There is a patch for 4.17 to confirm 
this is tech preview. I think we should consider to backport it.

Stefano can you add it in your backport list?

Cheers,

-- 
Julien Grall

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

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> > I think I've addressed all comments from Julien regarding my series,
> 
> If it is not too late for you would you be able to resend your series
> without the 'address-cells'/'size-cells' change? This will give me the
> opportunity to have an other review today.

I will be off after resending this so you can have another look today.

> 
> > so I think I've got some bandwidth to do the clean-up patch tomorrow
> > after the agreement, unless someone would like to do it himself?
> 
> Renaming "xen,static-mem-..." is a bit tricky because they have been
> defined in Xen 4.16.
> 
> I couldn't find any support statement specific to the static memory
> feature. So it would technically fall under the "dom0less" section which
> is security supported.
> 
> That said, I don't think we can consider that the static memory feature
> is even supported because, until yesterday, the code wasn't properly
> handling request to balloon in/out. So I would view this is a tech
> preview (Could someone send a patch to clarify SUPPORT.MD)?

In current code, the static allocation is in SUPPORT.md as tech preview.

> 
> This would mean that would be that we could consider the binding
> unstable and we could do a straight renaming. That said, I can
> understand this may be undesirable.
> 
> If that's the case then we would need to keep the current binding as-is.
> So we would have two options:
>    1) Provide a new compatible so #address-cells #size-cells can be
> used. The current binding can be deprecated
>    2) Leave as-is and accept the difference
> 
> I don't have a strong opinion on which way to go. Whichever, it would be
> good to write down the rationale in the commit message of the "future"
> patch.
> 
> I would not block this series on the renaming for existing property
> (what matter is the new ones are consistent with the discussion). The
> renaming could be done afterwards. I would even say post the feature
> freeze on Friday because this could be considered as a bug fix (assuming
> you agree as the release manager :)).

Actually this is the one I want to discuss with you, I am good with considering
this clean-up patch as a bug fix.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Stefano Stabellini]

On Wed, 7 Sep 2022, Julien Grall wrote:
> On 07/09/2022 14:49, Henry Wang wrote:
> > > -----Original Message-----
> > > From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> > > > > > > But in any case we should only add one pair here for sure, as you
> > > > > > > say
> > > the
> > > > > > > only implication is to add a couple of 0 in the worst case.
> > > > > > I agree. The only drawback is the need to modify the already
> > > > > > introduced
> > > properties
> > > > > > to be coherent.
> > > > > 
> > > > > Agree, someone will need to do a pass on the whole doc which might be
> > > easier with all things in.
> > > > > 
> > > > Well, not only docs. If we decide to use a single pair of #address-cells
> > > > and
> > > #size-cells, then
> > > > we need to modify the code that expects different properties e.g.
> > > xen,static-mem-{address/size}-cells.
> > > 
> > > Right I forgot that some parts are already in.
> > > So we will need an extra patch to handle those.
> > 
> > I think I've addressed all comments from Julien regarding my series,
> 
> If it is not too late for you would you be able to resend your series without
> the 'address-cells'/'size-cells' change? This will give me the opportunity to
> have an other review today.
> 
> > so I think I've got some bandwidth to do the clean-up patch tomorrow
> > after the agreement, unless someone would like to do it himself?
> 
> Renaming "xen,static-mem-..." is a bit tricky because they have been defined
> in Xen 4.16.
> 
> I couldn't find any support statement specific to the static memory feature.
> So it would technically fall under the "dom0less" section which is security
> supported.
> 
> That said, I don't think we can consider that the static memory feature is
> even supported because, until yesterday, the code wasn't properly handling
> request to balloon in/out. So I would view this is a tech preview (Could
> someone send a patch to clarify SUPPORT.MD)?
> 
> This would mean that would be that we could consider the binding unstable and
> we could do a straight renaming. That said, I can understand this may be
> undesirable.
> 
> If that's the case then we would need to keep the current binding as-is. So we
> would have two options:
>   1) Provide a new compatible so #address-cells #size-cells can be used. The
> current binding can be deprecated
>   2) Leave as-is and accept the difference
> 
> I don't have a strong opinion on which way to go. Whichever, it would be good
> to write down the rationale in the commit message of the "future" patch.
> 
> I would not block this series on the renaming for existing property (what
> matter is the new ones are consistent with the discussion). The renaming could
> be done afterwards. I would even say post the feature freeze on Friday because
> this could be considered as a bug fix (assuming you agree as the release
> manager :)).

I very much agree that we should be consistent. Consistency aside, I
would prefer *not* to introduce #xen,static-heap-address-cells and
#xen,static-heap-size-cells and instead reuse the regular #address-cells
and #size-cells. I think there is no reason why we shouldn't.

I was about to write something about it a couple of days ago but then I
noticed that we had already introduced #xen,static-mem-address-cells and
#xen,static-mem-size-cells. In order to be consistent I didn't say
anything and gave my ack.

But actually I think it is better to get rid of them all. I think we
should:

1) do not introduce #xen,static-heap-address-cells and
#xen,static-heap-size-cells in this series, instead rely on
#address-cells and #size-cells. Please write in the binding that the
number of address cells and size cells of xen,static-heap is determined
by the parent #address-cells and #size-cells. (It has to be the parent
because that is how #address-cells and #size-cells are defined.)

2) Also remove "#xen,static-mem-address-cells" and
"#xen,static-mem-size-cells", and also use #address-cells and
#size-cells for xen,static-mem as well. I think we should do that in
this release for consistency. Any volunteers? :-)

It is not going to break anything because, not only static-mem is tech
preview, but also it is very likely that if someone was using
#xen,static-heap-address-cells it would be setting it to the same value
as #address-cells. So in the vast majority of cases it would continue to
work as expected (not that we couldn't change it anyway, given that it
is a tech preview.)

So I am aligned with Julien on this.

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

[Stefano Stabellini]

On Wed, 7 Sep 2022, Stefano Stabellini wrote:
> On Wed, 7 Sep 2022, Julien Grall wrote:
> > On 07/09/2022 14:49, Henry Wang wrote:
> > > > -----Original Message-----
> > > > From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> > > > > > > > But in any case we should only add one pair here for sure, as you
> > > > > > > > say
> > > > the
> > > > > > > > only implication is to add a couple of 0 in the worst case.
> > > > > > > I agree. The only drawback is the need to modify the already
> > > > > > > introduced
> > > > properties
> > > > > > > to be coherent.
> > > > > > 
> > > > > > Agree, someone will need to do a pass on the whole doc which might be
> > > > easier with all things in.
> > > > > > 
> > > > > Well, not only docs. If we decide to use a single pair of #address-cells
> > > > > and
> > > > #size-cells, then
> > > > > we need to modify the code that expects different properties e.g.
> > > > xen,static-mem-{address/size}-cells.
> > > > 
> > > > Right I forgot that some parts are already in.
> > > > So we will need an extra patch to handle those.
> > > 
> > > I think I've addressed all comments from Julien regarding my series,
> > 
> > If it is not too late for you would you be able to resend your series without
> > the 'address-cells'/'size-cells' change? This will give me the opportunity to
> > have an other review today.
> > 
> > > so I think I've got some bandwidth to do the clean-up patch tomorrow
> > > after the agreement, unless someone would like to do it himself?
> > 
> > Renaming "xen,static-mem-..." is a bit tricky because they have been defined
> > in Xen 4.16.
> > 
> > I couldn't find any support statement specific to the static memory feature.
> > So it would technically fall under the "dom0less" section which is security
> > supported.
> > 
> > That said, I don't think we can consider that the static memory feature is
> > even supported because, until yesterday, the code wasn't properly handling
> > request to balloon in/out. So I would view this is a tech preview (Could
> > someone send a patch to clarify SUPPORT.MD)?
> > 
> > This would mean that would be that we could consider the binding unstable and
> > we could do a straight renaming. That said, I can understand this may be
> > undesirable.
> > 
> > If that's the case then we would need to keep the current binding as-is. So we
> > would have two options:
> >   1) Provide a new compatible so #address-cells #size-cells can be used. The
> > current binding can be deprecated
> >   2) Leave as-is and accept the difference
> > 
> > I don't have a strong opinion on which way to go. Whichever, it would be good
> > to write down the rationale in the commit message of the "future" patch.
> > 
> > I would not block this series on the renaming for existing property (what
> > matter is the new ones are consistent with the discussion). The renaming could
> > be done afterwards. I would even say post the feature freeze on Friday because
> > this could be considered as a bug fix (assuming you agree as the release
> > manager :)).
> 
> I very much agree that we should be consistent. Consistency aside, I
> would prefer *not* to introduce #xen,static-heap-address-cells and
> #xen,static-heap-size-cells and instead reuse the regular #address-cells
> and #size-cells. I think there is no reason why we shouldn't.
> 
> I was about to write something about it a couple of days ago but then I
> noticed that we had already introduced #xen,static-mem-address-cells and
> #xen,static-mem-size-cells. In order to be consistent I didn't say
> anything and gave my ack.
> 
> But actually I think it is better to get rid of them all. I think we
> should:
> 
> 1) do not introduce #xen,static-heap-address-cells and
> #xen,static-heap-size-cells in this series, instead rely on
> #address-cells and #size-cells. Please write in the binding that the
> number of address cells and size cells of xen,static-heap is determined
> by the parent #address-cells and #size-cells. (It has to be the parent
> because that is how #address-cells and #size-cells are defined.)
> 
> 2) Also remove "#xen,static-mem-address-cells" and
> "#xen,static-mem-size-cells", and also use #address-cells and
> #size-cells for xen,static-mem as well. I think we should do that in
> this release for consistency. Any volunteers? :-)


I should also add that I don't think we should change the compatible
string for xen,static-mem. If the feature is tech preview, maybe we
can consider the device tree bindings tech preview as well? I think that
would be OK.

If you don't think is OK, then I suggest to:
- first check "#xen,static-mem-address-cells" and
  "#xen,static-mem-size-cells" for compatibility
- if missing, use the regular #address-cells and #size-cells

This way we retain compatibility. Either way, I wouldn't change the
compatible string for xen,static-mem.

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

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> > I would not block this series on the renaming for existing property (what
> > matter is the new ones are consistent with the discussion). The renaming
> could
> > be done afterwards. I would even say post the feature freeze on Friday
> because
> > this could be considered as a bug fix (assuming you agree as the release
> > manager :)).
> 
> I very much agree that we should be consistent. Consistency aside, I
> would prefer *not* to introduce #xen,static-heap-address-cells and
> #xen,static-heap-size-cells and instead reuse the regular #address-cells
> and #size-cells. I think there is no reason why we shouldn't.
> 
> I was about to write something about it a couple of days ago but then I
> noticed that we had already introduced #xen,static-mem-address-cells and
> #xen,static-mem-size-cells. In order to be consistent I didn't say
> anything and gave my ack.
> 
> But actually I think it is better to get rid of them all. I think we
> should:
> 
> 1) do not introduce #xen,static-heap-address-cells and
> #xen,static-heap-size-cells in this series, instead rely on
> #address-cells and #size-cells. Please write in the binding that the
> number of address cells and size cells of xen,static-heap is determined
> by the parent #address-cells and #size-cells. (It has to be the parent
> because that is how #address-cells and #size-cells are defined.)

Ack, I will do in v5, also drop your previous ack so you can take a look
again. 

> 
> 2) Also remove "#xen,static-mem-address-cells" and
> "#xen,static-mem-size-cells", and also use #address-cells and
> #size-cells for xen,static-mem as well. I think we should do that in
> this release for consistency. Any volunteers? :-)

I will add a new patch in the end of this series for static-mem cleanup.
This can be merged later as a bug fix according to the discussion with
Julien.

Kind regards,
Henry

> 
> It is not going to break anything because, not only static-mem is tech
> preview, but also it is very likely that if someone was using
> #xen,static-heap-address-cells it would be setting it to the same value
> as #address-cells. So in the vast majority of cases it would continue to
> work as expected (not that we couldn't change it anyway, given that it
> is a tech preview.)
> 
> So I am aligned with Julien on this.