[PATCH 0/7] Static shared memory followup v2 - pt2

[PATCH 0/7] Static shared memory followup v2 - pt2

[Luca Fancellu]

This serie is a partial rework of this other serie:
https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-Penny.Zheng@arm.com/

The original serie is addressing an issue of the static shared memory feature
that impacts the memory footprint of other component when the feature is
enabled, another issue impacts the device tree generation for the guests when
the feature is enabled and used and the last one is a missing feature that is
the option to have a static shared memory region that is not from the host
address space.

This serie is handling some comment on the original serie and it is splitting
the rework in two part, this first part is addressing the memory footprint issue
and the device tree generation and currently is fully reviewed by Michal
(https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fancellu@arm.com/),
this serie is addressing the static shared memory allocation from the Xen heap.

This serie is meant to be applied on top of:
https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fancellu@arm.com/
where the last patch was amended in favour of:
https://patchwork.kernel.org/project/xen-devel/patch/20240422110207.204968-1-luca.fancellu@arm.com/

Luca Fancellu (5):
  xen/arm: Lookup bootinfo shm bank during the mapping
  xen/arm: Wrap shared memory mapping code in one function
  xen/arm: Parse xen,shared-mem when host phys address is not provided
  xen/arm: Rework heap page allocation outside allocate_bank_memory
  xen/arm: Implement the logic for static shared memory from Xen heap

Penny Zheng (2):
  xen/p2m: put reference for superpage
  xen/docs: Describe static shared memory when host address is not
    provided

 docs/misc/arm/device-tree/booting.txt   |  52 ++-
 xen/arch/arm/dom0less-build.c           |   4 +-
 xen/arch/arm/domain_build.c             |  77 +++--
 xen/arch/arm/include/asm/domain_build.h |   9 +-
 xen/arch/arm/mmu/p2m.c                  |  58 +++-
 xen/arch/arm/setup.c                    |   3 +-
 xen/arch/arm/static-shmem.c             | 430 +++++++++++++++++-------
 7 files changed, 463 insertions(+), 170 deletions(-)

-- 
2.34.1

[PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Luca Fancellu]

The current static shared memory code is using bootinfo banks when it
needs to find the number of borrower, so every time assign_shared_memory
is called, the bank is searched in the bootinfo.shmem structure.

There is nothing wrong with it, however the bank can be used also to
retrieve the start address and size and also to pass less argument to
assign_shared_memory. When retrieving the information from the bootinfo
bank, it's also possible to move the checks on alignment to
process_shm_node in the early stages.

So create a new function find_shm() which takes a 'struct shared_meminfo'
structure and the shared memory ID, to look for a bank with a matching ID,
take the physical host address and size from the bank, pass the bank to
assign_shared_memory() removing the now unnecessary arguments and finally
remove the acquire_nr_borrower_domain() function since now the information
can be extracted from the passed bank.
Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
case of errors (unlikely), as said above, move the checks on alignment
to process_shm_node.

Drawback of this change is that now the bootinfo are used also when the
bank doesn't need to be allocated, however it will be convinient later
to use it as an argument for assign_shared_memory when dealing with
the use case where the Host physical address is not supplied by the user.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/static-shmem.c | 105 ++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 47 deletions(-)

diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 09f474ec6050..f6cf74e58a83 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void)
                  offsetof(struct shared_meminfo, bank)));
 }
 
-static int __init acquire_nr_borrower_domain(struct domain *d,
-                                             paddr_t pbase, paddr_t psize,
-                                             unsigned long *nr_borrowers)
+static const struct membank __init *find_shm(const struct membanks *shmem,
+                                             const char *shm_id)
 {
-    const struct membanks *shmem = bootinfo_get_shmem();
     unsigned int bank;
 
-    /* Iterate reserved memory to find requested shm bank. */
+    BUG_ON(!shmem || !shm_id);
+
     for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
     {
-        paddr_t bank_start = shmem->bank[bank].start;
-        paddr_t bank_size = shmem->bank[bank].size;
-
-        if ( (pbase == bank_start) && (psize == bank_size) )
+        if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
+                     MAX_SHM_ID_LENGTH) == 0 )
             break;
     }
 
     if ( bank == shmem->nr_banks )
-        return -ENOENT;
-
-    *nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
+        return NULL;
 
-    return 0;
+    return &shmem->bank[bank];
 }
 
 /*
@@ -103,14 +98,20 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
     return smfn;
 }
 
-static int __init assign_shared_memory(struct domain *d,
-                                       paddr_t pbase, paddr_t psize,
-                                       paddr_t gbase)
+static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
+                                       const struct membank *shm_bank)
 {
     mfn_t smfn;
     int ret = 0;
     unsigned long nr_pages, nr_borrowers, i;
     struct page_info *page;
+    paddr_t pbase, psize;
+
+    BUG_ON(!shm_bank || !shm_bank->shmem_extra);
+
+    pbase = shm_bank->start;
+    psize = shm_bank->size;
+    nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
 
     printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
            d, pbase, pbase + psize);
@@ -135,14 +136,6 @@ static int __init assign_shared_memory(struct domain *d,
         }
     }
 
-    /*
-     * Get the right amount of references per page, which is the number of
-     * borrower domains.
-     */
-    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
-    if ( ret )
-        return ret;
-
     /*
      * Instead of letting borrower domain get a page ref, we add as many
      * additional reference as the number of borrowers when the owner
@@ -199,6 +192,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
 
     dt_for_each_child_node(node, shm_node)
     {
+        const struct membank *boot_shm_bank;
         const struct dt_property *prop;
         const __be32 *cells;
         uint32_t addr_cells, size_cells;
@@ -212,6 +206,23 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
             continue;
 
+        if ( dt_property_read_string(shm_node, "xen,shm-id", &shm_id) )
+        {
+            printk("%pd: invalid \"xen,shm-id\" property", d);
+            return -EINVAL;
+        }
+        BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
+
+        boot_shm_bank = find_shm(bootinfo_get_shmem(), shm_id);
+        if ( !boot_shm_bank )
+        {
+            printk("%pd: static shared memory bank not found: '%s'", d, shm_id);
+            return -ENOENT;
+        }
+
+        pbase = boot_shm_bank->start;
+        psize = boot_shm_bank->size;
+
         /*
          * xen,shared-mem = <pbase, gbase, size>;
          * TODO: pbase is optional.
@@ -221,20 +232,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
         BUG_ON(!prop);
         cells = (const __be32 *)prop->value;
-        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
-        psize = dt_read_paddr(cells, size_cells);
-        if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
-        {
-            printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
-                   d, pbase, gbase);
-            return -EINVAL;
-        }
-        if ( !IS_ALIGNED(psize, PAGE_SIZE) )
-        {
-            printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n",
-                   d, psize);
-            return -EINVAL;
-        }
+        gbase = dt_read_paddr(cells + addr_cells, addr_cells);
 
         for ( i = 0; i < PFN_DOWN(psize); i++ )
             if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
@@ -251,13 +249,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
             owner_dom_io = false;
 
-        if ( dt_property_read_string(shm_node, "xen,shm-id", &shm_id) )
-        {
-            printk("%pd: invalid \"xen,shm-id\" property", d);
-            return -EINVAL;
-        }
-        BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
-
         /*
          * DOMID_IO is a fake domain and is not described in the Device-Tree.
          * Therefore when the owner of the shared region is DOMID_IO, we will
@@ -270,8 +261,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
              * We found the first borrower of the region, the owner was not
              * specified, so they should be assigned to dom_io.
              */
-            ret = assign_shared_memory(owner_dom_io ? dom_io : d,
-                                       pbase, psize, gbase);
+            ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
+                                       boot_shm_bank);
             if ( ret )
                 return ret;
         }
@@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
     device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
     size = dt_next_cell(size_cells, &cell);
 
+    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
+    {
+        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
+               paddr);
+        return -EINVAL;
+    }
+
+    if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
+    {
+        printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
+               gaddr);
+        return -EINVAL;
+    }
+
+    if ( !IS_ALIGNED(size, PAGE_SIZE) )
+    {
+        printk("fdt: size 0x%"PRIpaddr" is not suitably aligned\n", size);
+        return -EINVAL;
+    }
+
     if ( !size )
     {
         printk("fdt: the size for static shared memory region can not be zero\n");
-- 
2.34.1

Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> The current static shared memory code is using bootinfo banks when it
> needs to find the number of borrower, so every time assign_shared_memory
s/borrower/borrowers

> is called, the bank is searched in the bootinfo.shmem structure.
> 
> There is nothing wrong with it, however the bank can be used also to
> retrieve the start address and size and also to pass less argument to
> assign_shared_memory. When retrieving the information from the bootinfo
> bank, it's also possible to move the checks on alignment to
> process_shm_node in the early stages.
Is this change really required for what you want to achieve? At the moment the alignment checks
are done before first use, which requires these values to be aligned. FDT processing part does not need it.

> 
> So create a new function find_shm() which takes a 'struct shared_meminfo'
Can we name it find_shm_bank() or find_shm_bank_by_id()?
I agree that it's better to use a unique ID rather than matching by address/size

> structure and the shared memory ID, to look for a bank with a matching ID,
> take the physical host address and size from the bank, pass the bank to
> assign_shared_memory() removing the now unnecessary arguments and finally
> remove the acquire_nr_borrower_domain() function since now the information
> can be extracted from the passed bank.
> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
> case of errors (unlikely), as said above, move the checks on alignment
> to process_shm_node.
> 
> Drawback of this change is that now the bootinfo are used also when the
> bank doesn't need to be allocated, however it will be convinient later
> to use it as an argument for assign_shared_memory when dealing with
> the use case where the Host physical address is not supplied by the user.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/static-shmem.c | 105 ++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 09f474ec6050..f6cf74e58a83 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void)
>                   offsetof(struct shared_meminfo, bank)));
>  }
> 
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> -                                             paddr_t pbase, paddr_t psize,
> -                                             unsigned long *nr_borrowers)
> +static const struct membank __init *find_shm(const struct membanks *shmem,
> +                                             const char *shm_id)
>  {
> -    const struct membanks *shmem = bootinfo_get_shmem();
>      unsigned int bank;
> 
> -    /* Iterate reserved memory to find requested shm bank. */
> +    BUG_ON(!shmem || !shm_id);
Is it really necessary? For example, before calling find_shm(), strlen is used on shm_id

> +
>      for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
>      {
> -        paddr_t bank_start = shmem->bank[bank].start;
> -        paddr_t bank_size = shmem->bank[bank].size;
> -
> -        if ( (pbase == bank_start) && (psize == bank_size) )
> +        if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
> +                     MAX_SHM_ID_LENGTH) == 0 )
Why not strcmp? AFAICS it's been validated many times already

>              break;
>      }
> 
>      if ( bank == shmem->nr_banks )
> -        return -ENOENT;
> -
> -    *nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
> +        return NULL;
> 
> -    return 0;
> +    return &shmem->bank[bank];
>  }
> 
>  /*
> @@ -103,14 +98,20 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>      return smfn;
>  }
> 
> -static int __init assign_shared_memory(struct domain *d,
> -                                       paddr_t pbase, paddr_t psize,
> -                                       paddr_t gbase)
> +static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
> +                                       const struct membank *shm_bank)
>  {
>      mfn_t smfn;
>      int ret = 0;
>      unsigned long nr_pages, nr_borrowers, i;
>      struct page_info *page;
> +    paddr_t pbase, psize;
> +
> +    BUG_ON(!shm_bank || !shm_bank->shmem_extra);
Is it really necessary? Isn't shm_bank already validated? It's not very common to have NULL checks in internal functions.

> +
> +    pbase = shm_bank->start;
> +    psize = shm_bank->size;
> +    nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
> 
>      printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
>             d, pbase, pbase + psize);
> @@ -135,14 +136,6 @@ static int __init assign_shared_memory(struct domain *d,
>          }
>      }
> 
> -    /*
> -     * Get the right amount of references per page, which is the number of
> -     * borrower domains.
> -     */
> -    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> -    if ( ret )
> -        return ret;
> -
>      /*
>       * Instead of letting borrower domain get a page ref, we add as many
>       * additional reference as the number of borrowers when the owner
> @@ -199,6 +192,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> 
>      dt_for_each_child_node(node, shm_node)
>      {
> +        const struct membank *boot_shm_bank;
>          const struct dt_property *prop;
>          const __be32 *cells;
>          uint32_t addr_cells, size_cells;
> @@ -212,6 +206,23 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>          if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
>              continue;
> 
> +        if ( dt_property_read_string(shm_node, "xen,shm-id", &shm_id) )
> +        {
> +            printk("%pd: invalid \"xen,shm-id\" property", d);
> +            return -EINVAL;
> +        }
> +        BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
> +
> +        boot_shm_bank = find_shm(bootinfo_get_shmem(), shm_id);
> +        if ( !boot_shm_bank )
> +        {
> +            printk("%pd: static shared memory bank not found: '%s'", d, shm_id);
> +            return -ENOENT;
> +        }
> +
> +        pbase = boot_shm_bank->start;
> +        psize = boot_shm_bank->size;
> +
>          /*
>           * xen,shared-mem = <pbase, gbase, size>;
>           * TODO: pbase is optional.
> @@ -221,20 +232,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>          prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
>          BUG_ON(!prop);
>          cells = (const __be32 *)prop->value;
> -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
> -        psize = dt_read_paddr(cells, size_cells);
> -        if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
> -        {
> -            printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
> -                   d, pbase, gbase);
> -            return -EINVAL;
> -        }
> -        if ( !IS_ALIGNED(psize, PAGE_SIZE) )
> -        {
> -            printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n",
> -                   d, psize);
> -            return -EINVAL;
> -        }
> +        gbase = dt_read_paddr(cells + addr_cells, addr_cells);
> 
>          for ( i = 0; i < PFN_DOWN(psize); i++ )
>              if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
> @@ -251,13 +249,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>          if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>              owner_dom_io = false;
> 
> -        if ( dt_property_read_string(shm_node, "xen,shm-id", &shm_id) )
> -        {
> -            printk("%pd: invalid \"xen,shm-id\" property", d);
> -            return -EINVAL;
> -        }
> -        BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
> -
>          /*
>           * DOMID_IO is a fake domain and is not described in the Device-Tree.
>           * Therefore when the owner of the shared region is DOMID_IO, we will
> @@ -270,8 +261,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>               * We found the first borrower of the region, the owner was not
>               * specified, so they should be assigned to dom_io.
>               */
> -            ret = assign_shared_memory(owner_dom_io ? dom_io : d,
> -                                       pbase, psize, gbase);
> +            ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
> +                                       boot_shm_bank);
>              if ( ret )
>                  return ret;
>          }
> @@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>      device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
>      size = dt_next_cell(size_cells, &cell);
> 
> +    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
> +    {
> +        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
> +               paddr);
> +        return -EINVAL;
> +    }
> +
> +    if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
> +    {
> +        printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
> +               gaddr);
> +        return -EINVAL;
> +    }
> +
> +    if ( !IS_ALIGNED(size, PAGE_SIZE) )
What sense does it make to check for size being aligned before checking for size being 0? It would pass this check.

> +    {
> +        printk("fdt: size 0x%"PRIpaddr" is not suitably aligned\n", size);
> +        return -EINVAL;
> +    }
> +
>      if ( !size )
>      {
>          printk("fdt: the size for static shared memory region can not be zero\n");
> --
> 2.34.1
> 

~Michal

Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Luca Fancellu]

Hi Michal,

Thanks for your review.

> On 6 May 2024, at 14:24, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Luca,
> 
> On 23/04/2024 10:25, Luca Fancellu wrote:
>> 
>> 
>> The current static shared memory code is using bootinfo banks when it
>> needs to find the number of borrower, so every time assign_shared_memory
> s/borrower/borrowers

Will fix

> 
>> is called, the bank is searched in the bootinfo.shmem structure.
>> 
>> There is nothing wrong with it, however the bank can be used also to
>> retrieve the start address and size and also to pass less argument to
>> assign_shared_memory. When retrieving the information from the bootinfo
>> bank, it's also possible to move the checks on alignment to
>> process_shm_node in the early stages.
> Is this change really required for what you want to achieve? At the moment the alignment checks
> are done before first use, which requires these values to be aligned. FDT processing part does not need it.

That’s true, but it would separate better the parsing part, in the end what is the point of failing later if, for example,
some value are passed but not aligned? 

> 
>> 
>> So create a new function find_shm() which takes a 'struct shared_meminfo'
> Can we name it find_shm_bank() or find_shm_bank_by_id()?
> I agree that it's better to use a unique ID rather than matching by address/size

Yes either names are good for me, I would use find_shm_bank_by_id

> 
>> structure and the shared memory ID, to look for a bank with a matching ID,
>> take the physical host address and size from the bank, pass the bank to
>> assign_shared_memory() removing the now unnecessary arguments and finally
>> remove the acquire_nr_borrower_domain() function since now the information
>> can be extracted from the passed bank.
>> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
>> case of errors (unlikely), as said above, move the checks on alignment
>> to process_shm_node.
>> 
>> Drawback of this change is that now the bootinfo are used also when the
>> bank doesn't need to be allocated, however it will be convinient later
>> to use it as an argument for assign_shared_memory when dealing with
>> the use case where the Host physical address is not supplied by the user.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> xen/arch/arm/static-shmem.c | 105 ++++++++++++++++++++----------------
>> 1 file changed, 58 insertions(+), 47 deletions(-)
>> 
>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>> index 09f474ec6050..f6cf74e58a83 100644
>> --- a/xen/arch/arm/static-shmem.c
>> +++ b/xen/arch/arm/static-shmem.c
>> @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void)
>>                  offsetof(struct shared_meminfo, bank)));
>> }
>> 
>> -static int __init acquire_nr_borrower_domain(struct domain *d,
>> -                                             paddr_t pbase, paddr_t psize,
>> -                                             unsigned long *nr_borrowers)
>> +static const struct membank __init *find_shm(const struct membanks *shmem,
>> +                                             const char *shm_id)
>> {
>> -    const struct membanks *shmem = bootinfo_get_shmem();
>>     unsigned int bank;
>> 
>> -    /* Iterate reserved memory to find requested shm bank. */
>> +    BUG_ON(!shmem || !shm_id);
> Is it really necessary? For example, before calling find_shm(), strlen is used on shm_id

So, I guess I did that to have more robust code, in case someone changes the code in the
future and perhaps removes something we rely on. If you object to them I will remove though,
here and the other related points below.

> 
>> +
>>     for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
>>     {
>> -        paddr_t bank_start = shmem->bank[bank].start;
>> -        paddr_t bank_size = shmem->bank[bank].size;
>> -
>> -        if ( (pbase == bank_start) && (psize == bank_size) )
>> +        if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
>> +                     MAX_SHM_ID_LENGTH) == 0 )
> Why not strcmp? AFAICS it's been validated many times already
> 
>>             break;
>>     }
>> 
>>     if ( bank == shmem->nr_banks )
>> -        return -ENOENT;
>> -
>> -    *nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
>> +        return NULL;
>> 
>> -    return 0;
>> +    return &shmem->bank[bank];
>> }
>> 
>> /*
>> @@ -103,14 +98,20 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>>     return smfn;
>> }
>> 
>> -static int __init assign_shared_memory(struct domain *d,
>> -                                       paddr_t pbase, paddr_t psize,
>> -                                       paddr_t gbase)
>> +static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>> +                                       const struct membank *shm_bank)
>> {
>>     mfn_t smfn;
>>     int ret = 0;
>>     unsigned long nr_pages, nr_borrowers, i;
>>     struct page_info *page;
>> +    paddr_t pbase, psize;
>> +
>> +    BUG_ON(!shm_bank || !shm_bank->shmem_extra);
> Is it really necessary? Isn't shm_bank already validated? It's not very common to have NULL checks in internal functions.
> 

[...]

>> 
>> @@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>>     device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
>>     size = dt_next_cell(size_cells, &cell);
>> 
>> +    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
>> +    {
>> +        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
>> +               paddr);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
>> +    {
>> +        printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
>> +               gaddr);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if ( !IS_ALIGNED(size, PAGE_SIZE) )
> What sense does it make to check for size being aligned before checking for size being 0? It would pass this check.

Yes, but in the end we are doing that to print a different error message, so it would pass
for 0 and it’s totally fine, but in the end it will fail afterwards. I don’t see functional disruptions
having this one before the other, what is the concern here?

> 
>> +    {
>> +        printk("fdt: size 0x%"PRIpaddr" is not suitably aligned\n", size);
>> +        return -EINVAL;
>> +    }
>> +
>>     if ( !size )
>>     {
>>         printk("fdt: the size for static shared memory region can not be zero\n");
>> --
>> 2.34.1
>> 

Cheers,
Luca

Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Michal Orzel]

On 07/05/2024 15:44, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
> Thanks for your review.
> 
>> On 6 May 2024, at 14:24, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Hi Luca,
>>
>> On 23/04/2024 10:25, Luca Fancellu wrote:
>>>
>>>
>>> The current static shared memory code is using bootinfo banks when it
>>> needs to find the number of borrower, so every time assign_shared_memory
>> s/borrower/borrowers
> 
> Will fix
> 
>>
>>> is called, the bank is searched in the bootinfo.shmem structure.
>>>
>>> There is nothing wrong with it, however the bank can be used also to
>>> retrieve the start address and size and also to pass less argument to
>>> assign_shared_memory. When retrieving the information from the bootinfo
>>> bank, it's also possible to move the checks on alignment to
>>> process_shm_node in the early stages.
>> Is this change really required for what you want to achieve? At the moment the alignment checks
>> are done before first use, which requires these values to be aligned. FDT processing part does not need it.
> 
> That’s true, but it would separate better the parsing part, in the end what is the point of failing later if, for example,
> some value are passed but not aligned?
> 
>>
>>>
>>> So create a new function find_shm() which takes a 'struct shared_meminfo'
>> Can we name it find_shm_bank() or find_shm_bank_by_id()?
>> I agree that it's better to use a unique ID rather than matching by address/size
> 
> Yes either names are good for me, I would use find_shm_bank_by_id
> 
>>
>>> structure and the shared memory ID, to look for a bank with a matching ID,
>>> take the physical host address and size from the bank, pass the bank to
>>> assign_shared_memory() removing the now unnecessary arguments and finally
>>> remove the acquire_nr_borrower_domain() function since now the information
>>> can be extracted from the passed bank.
>>> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
>>> case of errors (unlikely), as said above, move the checks on alignment
>>> to process_shm_node.
>>>
>>> Drawback of this change is that now the bootinfo are used also when the
>>> bank doesn't need to be allocated, however it will be convinient later
>>> to use it as an argument for assign_shared_memory when dealing with
>>> the use case where the Host physical address is not supplied by the user.
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>> xen/arch/arm/static-shmem.c | 105 ++++++++++++++++++++----------------
>>> 1 file changed, 58 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>>> index 09f474ec6050..f6cf74e58a83 100644
>>> --- a/xen/arch/arm/static-shmem.c
>>> +++ b/xen/arch/arm/static-shmem.c
>>> @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void)
>>>                  offsetof(struct shared_meminfo, bank)));
>>> }
>>>
>>> -static int __init acquire_nr_borrower_domain(struct domain *d,
>>> -                                             paddr_t pbase, paddr_t psize,
>>> -                                             unsigned long *nr_borrowers)
>>> +static const struct membank __init *find_shm(const struct membanks *shmem,
>>> +                                             const char *shm_id)
>>> {
>>> -    const struct membanks *shmem = bootinfo_get_shmem();
>>>     unsigned int bank;
>>>
>>> -    /* Iterate reserved memory to find requested shm bank. */
>>> +    BUG_ON(!shmem || !shm_id);
>> Is it really necessary? For example, before calling find_shm(), strlen is used on shm_id
> 
> So, I guess I did that to have more robust code, in case someone changes the code in the
> future and perhaps removes something we rely on. If you object to them I will remove though,
> here and the other related points below.
> 
>>
>>> +
>>>     for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
>>>     {
>>> -        paddr_t bank_start = shmem->bank[bank].start;
>>> -        paddr_t bank_size = shmem->bank[bank].size;
>>> -
>>> -        if ( (pbase == bank_start) && (psize == bank_size) )
>>> +        if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
>>> +                     MAX_SHM_ID_LENGTH) == 0 )
>> Why not strcmp? AFAICS it's been validated many times already
>>
>>>             break;
>>>     }
>>>
>>>     if ( bank == shmem->nr_banks )
>>> -        return -ENOENT;
>>> -
>>> -    *nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
>>> +        return NULL;
>>>
>>> -    return 0;
>>> +    return &shmem->bank[bank];
>>> }
>>>
>>> /*
>>> @@ -103,14 +98,20 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>>>     return smfn;
>>> }
>>>
>>> -static int __init assign_shared_memory(struct domain *d,
>>> -                                       paddr_t pbase, paddr_t psize,
>>> -                                       paddr_t gbase)
>>> +static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>>> +                                       const struct membank *shm_bank)
>>> {
>>>     mfn_t smfn;
>>>     int ret = 0;
>>>     unsigned long nr_pages, nr_borrowers, i;
>>>     struct page_info *page;
>>> +    paddr_t pbase, psize;
>>> +
>>> +    BUG_ON(!shm_bank || !shm_bank->shmem_extra);
>> Is it really necessary? Isn't shm_bank already validated? It's not very common to have NULL checks in internal functions.
>>
> 
> [...]
> 
>>>
>>> @@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>>>     device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
>>>     size = dt_next_cell(size_cells, &cell);
>>>
>>> +    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
>>> +    {
>>> +        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
>>> +               paddr);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
>>> +    {
>>> +        printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
>>> +               gaddr);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if ( !IS_ALIGNED(size, PAGE_SIZE) )
>> What sense does it make to check for size being aligned before checking for size being 0? It would pass this check.
> 
> Yes, but in the end we are doing that to print a different error message, so it would pass
> for 0 and it’s totally fine, but in the end it will fail afterwards. I don’t see functional disruptions
> having this one before the other, what is the concern here?
It does not cause the functional disruption. It is more about code readability and writing cleaner code.
It makes more sense to first check for size being 0 rather than whether it's page aligned, since the latter can
pass if former is true and thus not making much sense.

~Michal

Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Luca Fancellu]

Hi Michal,

>> 
>>>> 
>>>> @@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>>>>    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
>>>>    size = dt_next_cell(size_cells, &cell);
>>>> 
>>>> +    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
>>>> +    {
>>>> +        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
>>>> +               paddr);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
>>>> +    {
>>>> +        printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
>>>> +               gaddr);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if ( !IS_ALIGNED(size, PAGE_SIZE) )
>>> What sense does it make to check for size being aligned before checking for size being 0? It would pass this check.
>> 
>> Yes, but in the end we are doing that to print a different error message, so it would pass
>> for 0 and it’s totally fine, but in the end it will fail afterwards. I don’t see functional disruptions
>> having this one before the other, what is the concern here?
> It does not cause the functional disruption. It is more about code readability and writing cleaner code.
> It makes more sense to first check for size being 0 rather than whether it's page aligned, since the latter can
> pass if former is true and thus not making much sense.

Ok then I will switch them and check it being different from 0 before the alignment check.

Cheers,
Luca

[PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Luca Fancellu]

Wrap the code and logic that is calling assign_shared_memory
and map_regions_p2mt into a new function 'handle_shared_mem_bank',
it will become useful later when the code will allow the user to
don't pass the host physical address.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/static-shmem.c | 71 +++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index f6cf74e58a83..24e40495a481 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start,
     return 0;
 }
 
+static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
+                                         bool owner_dom_io,
+                                         const char *role_str,
+                                         const struct membank *shm_bank)
+{
+    paddr_t pbase, psize;
+    int ret;
+
+    BUG_ON(!shm_bank);
+
+    pbase = shm_bank->start;
+    psize = shm_bank->size;
+    /*
+     * DOMID_IO is a fake domain and is not described in the Device-Tree.
+     * Therefore when the owner of the shared region is DOMID_IO, we will
+     * only find the borrowers.
+     */
+    if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
+         (!owner_dom_io && strcmp(role_str, "owner") == 0) )
+    {
+        /*
+         * We found the first borrower of the region, the owner was not
+         * specified, so they should be assigned to dom_io.
+         */
+        ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
+        if ( ret )
+            return ret;
+    }
+
+    if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
+    {
+        /* Set up P2M foreign mapping for borrower domain. */
+        ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
+                               _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
+        if ( ret )
+            return ret;
+    }
+
+    return 0;
+}
+
 int __init process_shm(struct domain *d, struct kernel_info *kinfo,
                        const struct dt_device_node *node)
 {
@@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
             owner_dom_io = false;
 
-        /*
-         * DOMID_IO is a fake domain and is not described in the Device-Tree.
-         * Therefore when the owner of the shared region is DOMID_IO, we will
-         * only find the borrowers.
-         */
-        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
-             (!owner_dom_io && strcmp(role_str, "owner") == 0) )
-        {
-            /*
-             * We found the first borrower of the region, the owner was not
-             * specified, so they should be assigned to dom_io.
-             */
-            ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
-                                       boot_shm_bank);
-            if ( ret )
-                return ret;
-        }
-
-        if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
-        {
-            /* Set up P2M foreign mapping for borrower domain. */
-            ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
-                                   _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
-            if ( ret )
-                return ret;
-        }
+        ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str,
+                                     boot_shm_bank);
+        if ( ret )
+            return ret;
 
         /*
          * Record static shared memory region info for later setting
-- 
2.34.1

Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> Wrap the code and logic that is calling assign_shared_memory
> and map_regions_p2mt into a new function 'handle_shared_mem_bank',
> it will become useful later when the code will allow the user to
> don't pass the host physical address.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/static-shmem.c | 71 +++++++++++++++++++++++--------------
>  1 file changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index f6cf74e58a83..24e40495a481 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start,
>      return 0;
>  }
> 
> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> +                                         bool owner_dom_io,
> +                                         const char *role_str,
> +                                         const struct membank *shm_bank)
> +{
> +    paddr_t pbase, psize;
> +    int ret;
> +
> +    BUG_ON(!shm_bank);
not needed

> +
> +    pbase = shm_bank->start;
> +    psize = shm_bank->size;
please add empty line here

> +    /*
> +     * DOMID_IO is a fake domain and is not described in the Device-Tree.
> +     * Therefore when the owner of the shared region is DOMID_IO, we will
> +     * only find the borrowers.
> +     */
> +    if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> +         (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> +    {
> +        /*
> +         * We found the first borrower of the region, the owner was not
> +         * specified, so they should be assigned to dom_io.
> +         */
> +        ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
> +        if ( ret )
> +            return ret;
> +    }
> +
> +    if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
> +    {
> +        /* Set up P2M foreign mapping for borrower domain. */
> +        ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
> +                               _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
> +        if ( ret )
> +            return ret;
> +    }
> +
> +    return 0;
> +}
> +
>  int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>                         const struct dt_device_node *node)
>  {
> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>          if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>              owner_dom_io = false;
Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()?

~Michal

Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Luca Fancellu]

Hi Michal,

>> 
>> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>> +                                         bool owner_dom_io,
>> +                                         const char *role_str,
>> +                                         const struct membank *shm_bank)
>> +{
>> +    paddr_t pbase, psize;
>> +    int ret;
>> +
>> +    BUG_ON(!shm_bank);
> not needed
> 
>> +
>> +    pbase = shm_bank->start;
>> +    psize = shm_bank->size;
> please add empty line here

Will do
>> 
>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>                        const struct dt_device_node *node)
>> {
>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>         if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>>             owner_dom_io = false;
> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()?

I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could
pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive
owner_dom_io from role_str being passed or not, something like:

role_str = NULL;
dt_property_read_string(shm_node, "role", &role_str)

[inside handle_shared_mem_bank]:
If ( role_str )
    owner_dom_io = false;

And pass only role_str to handle_shared_mem_bank.

Is this comment to reduce the number of parameters passed? I guess it’s not for where we call
dt_property_read_string isn’t it?

Cheers,
Luca

Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Michal Orzel]

On 07/05/2024 15:57, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>>>
>>> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>> +                                         bool owner_dom_io,
>>> +                                         const char *role_str,
>>> +                                         const struct membank *shm_bank)
>>> +{
>>> +    paddr_t pbase, psize;
>>> +    int ret;
>>> +
>>> +    BUG_ON(!shm_bank);
>> not needed
>>
>>> +
>>> +    pbase = shm_bank->start;
>>> +    psize = shm_bank->size;
>> please add empty line here
> 
> Will do
>>>
>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>                        const struct dt_device_node *node)
>>> {
>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>         if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>>>             owner_dom_io = false;
>> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()?
> 
> I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could
> pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive
> owner_dom_io from role_str being passed or not, something like:
> 
> role_str = NULL;
> dt_property_read_string(shm_node, "role", &role_str)
> 
> [inside handle_shared_mem_bank]:
> If ( role_str )
>     owner_dom_io = false;
> 
> And pass only role_str to handle_shared_mem_bank.
> 
> Is this comment to reduce the number of parameters passed? I guess it’s not for where we call
In this series as well as the previous one you limit the number of arguments passed to quite a few functions.
So naturally I would expect it to be done here as well. owner_dom_io is used only by handle_shared_mem_bank, so it makes more sense to move parsing to this
function so that it is self-contained.

~Michal

Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Luca Fancellu]

Hi Michal,


>>>> 
>>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>>                       const struct dt_device_node *node)
>>>> {
>>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>>        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>>>>            owner_dom_io = false;
>>> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()?
>> 
>> I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could
>> pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive
>> owner_dom_io from role_str being passed or not, something like:
>> 
>> role_str = NULL;
>> dt_property_read_string(shm_node, "role", &role_str)
>> 
>> [inside handle_shared_mem_bank]:
>> If ( role_str )
>>    owner_dom_io = false;
>> 
>> And pass only role_str to handle_shared_mem_bank.
>> 
>> Is this comment to reduce the number of parameters passed? I guess it’s not for where we call
> In this series as well as the previous one you limit the number of arguments passed to quite a few functions.
> So naturally I would expect it to be done here as well. owner_dom_io is used only by handle_shared_mem_bank, so it makes more sense to move parsing to this
> function so that it is self-contained.

Ok I will, just to be on the same page here, you mean having dt_property_read_string inside handle_shared_mem_bank?
Or the above example would work for you as well? That one would have role_str passed instead of shm_node.

Cheers,
Luca

Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Michal Orzel]

On 07/05/2024 16:15, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
> 
>>>>>
>>>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>>>                       const struct dt_device_node *node)
>>>>> {
>>>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>>>        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>>>>>            owner_dom_io = false;
>>>> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()?
>>>
>>> I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could
>>> pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive
>>> owner_dom_io from role_str being passed or not, something like:
>>>
>>> role_str = NULL;
>>> dt_property_read_string(shm_node, "role", &role_str)
>>>
>>> [inside handle_shared_mem_bank]:
>>> If ( role_str )
>>>    owner_dom_io = false;
>>>
>>> And pass only role_str to handle_shared_mem_bank.
>>>
>>> Is this comment to reduce the number of parameters passed? I guess it’s not for where we call
>> In this series as well as the previous one you limit the number of arguments passed to quite a few functions.
>> So naturally I would expect it to be done here as well. owner_dom_io is used only by handle_shared_mem_bank, so it makes more sense to move parsing to this
>> function so that it is self-contained.
> 
> Ok I will, just to be on the same page here, you mean having dt_property_read_string inside handle_shared_mem_bank?
> Or the above example would work for you as well? That one would have role_str passed instead of shm_node.
I'm ok with the solution above to pass role_str.

~Michal

[PATCH 3/7] xen/p2m: put reference for superpage

[Luca Fancellu]

From: Penny Zheng <Penny.Zheng@arm.com>

We are doing foreign memory mapping for static shared memory, and
there is a great possibility that it could be super mapped.
But today, p2m_put_l3_page could not handle superpages.

This commits implements a new function p2m_put_superpage to handle superpages,
specifically for helping put extra references for foreign superpages.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v1:
 - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-Penny.Zheng@arm.com/
---
 xen/arch/arm/mmu/p2m.c | 58 +++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
index 41fcca011cf4..479a80fbd4cf 100644
--- a/xen/arch/arm/mmu/p2m.c
+++ b/xen/arch/arm/mmu/p2m.c
@@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
     return rc;
 }
 
-/*
- * Put any references on the single 4K page referenced by pte.
- * TODO: Handle superpages, for now we only take special references for leaf
- * pages (specifically foreign ones, which can't be super mapped today).
- */
-static void p2m_put_l3_page(const lpae_t pte)
+/* Put any references on the single 4K page referenced by mfn. */
+static void p2m_put_l3_page(mfn_t mfn, unsigned type)
 {
-    mfn_t mfn = lpae_get_mfn(pte);
-
-    ASSERT(p2m_is_valid(pte));
-
     /*
      * TODO: Handle other p2m types
      *
@@ -771,16 +763,53 @@ static void p2m_put_l3_page(const lpae_t pte)
      * flush the TLBs if the page is reallocated before the end of
      * this loop.
      */
-    if ( p2m_is_foreign(pte.p2m.type) )
+    if ( p2m_is_foreign(type) )
     {
         ASSERT(mfn_valid(mfn));
         put_page(mfn_to_page(mfn));
     }
     /* Detect the xenheap page and mark the stored GFN as invalid. */
-    else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
+    else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
         page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
 }
 
+/* Put any references on the superpage referenced by mfn. */
+static void p2m_put_superpage(mfn_t mfn, unsigned int next_level, unsigned type)
+{
+    unsigned int i;
+    unsigned int level_order = XEN_PT_LEVEL_ORDER(next_level);
+
+    for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
+    {
+        if ( next_level == 3 )
+            p2m_put_l3_page(mfn, type);
+        else
+            p2m_put_superpage(mfn, next_level + 1, type);
+
+        mfn = mfn_add(mfn, 1 << level_order);
+    }
+}
+
+/* Put any references on the page referenced by pte. */
+static void p2m_put_page(const lpae_t pte, unsigned int level)
+{
+    mfn_t mfn = lpae_get_mfn(pte);
+
+    ASSERT(p2m_is_valid(pte));
+
+    /*
+     * We are either having a first level 1G superpage or a
+     * second level 2M superpage.
+     */
+    if ( p2m_is_superpage(pte, level) )
+        return p2m_put_superpage(mfn, level + 1, pte.p2m.type);
+    else
+    {
+        ASSERT(level == 3);
+        return p2m_put_l3_page(mfn, pte.p2m.type);
+    }
+}
+
 /* Free lpae sub-tree behind an entry */
 static void p2m_free_entry(struct p2m_domain *p2m,
                            lpae_t entry, unsigned int level)
@@ -809,9 +838,8 @@ static void p2m_free_entry(struct p2m_domain *p2m,
 #endif
 
         p2m->stats.mappings[level]--;
-        /* Nothing to do if the entry is a super-page. */
-        if ( level == 3 )
-            p2m_put_l3_page(entry);
+        p2m_put_page(entry, level);
+
         return;
     }
 
-- 
2.34.1

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> We are doing foreign memory mapping for static shared memory, and
> there is a great possibility that it could be super mapped.
> But today, p2m_put_l3_page could not handle superpages.
> 
> This commits implements a new function p2m_put_superpage to handle superpages,
> specifically for helping put extra references for foreign superpages.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v1:
>  - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-Penny.Zheng@arm.com/
> ---
>  xen/arch/arm/mmu/p2m.c | 58 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 41fcca011cf4..479a80fbd4cf 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
>      return rc;
>  }
> 
> -/*
> - * Put any references on the single 4K page referenced by pte.
> - * TODO: Handle superpages, for now we only take special references for leaf
> - * pages (specifically foreign ones, which can't be super mapped today).
> - */
> -static void p2m_put_l3_page(const lpae_t pte)
> +/* Put any references on the single 4K page referenced by mfn. */
> +static void p2m_put_l3_page(mfn_t mfn, unsigned type)
Shouldn't type be of p2m_type_t?

>  {
> -    mfn_t mfn = lpae_get_mfn(pte);
> -
> -    ASSERT(p2m_is_valid(pte));
> -
>      /*
>       * TODO: Handle other p2m types
>       *
> @@ -771,16 +763,53 @@ static void p2m_put_l3_page(const lpae_t pte)
>       * flush the TLBs if the page is reallocated before the end of
>       * this loop.
>       */
> -    if ( p2m_is_foreign(pte.p2m.type) )
> +    if ( p2m_is_foreign(type) )
>      {
>          ASSERT(mfn_valid(mfn));
>          put_page(mfn_to_page(mfn));
>      }
>      /* Detect the xenheap page and mark the stored GFN as invalid. */
> -    else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
> +    else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
>          page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
>  }
> 
> +/* Put any references on the superpage referenced by mfn. */
> +static void p2m_put_superpage(mfn_t mfn, unsigned int next_level, unsigned type)
Shouldn't type be of p2m_type_t?

> +{
> +    unsigned int i;
> +    unsigned int level_order = XEN_PT_LEVEL_ORDER(next_level);
> +
> +    for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
> +    {
> +        if ( next_level == 3 )
> +            p2m_put_l3_page(mfn, type);
> +        else
> +            p2m_put_superpage(mfn, next_level + 1, type);
> +
> +        mfn = mfn_add(mfn, 1 << level_order);
> +    }
> +}
> +
> +/* Put any references on the page referenced by pte. */
> +static void p2m_put_page(const lpae_t pte, unsigned int level)
> +{
> +    mfn_t mfn = lpae_get_mfn(pte);
> +
> +    ASSERT(p2m_is_valid(pte));
> +
> +    /*
> +     * We are either having a first level 1G superpage or a
> +     * second level 2M superpage.
> +     */
> +    if ( p2m_is_superpage(pte, level) )
> +        return p2m_put_superpage(mfn, level + 1, pte.p2m.type);
> +    else
No need for this else

> +    {
> +        ASSERT(level == 3);
> +        return p2m_put_l3_page(mfn, pte.p2m.type);
> +    }
> +}
> +
>  /* Free lpae sub-tree behind an entry */
>  static void p2m_free_entry(struct p2m_domain *p2m,
>                             lpae_t entry, unsigned int level)
> @@ -809,9 +838,8 @@ static void p2m_free_entry(struct p2m_domain *p2m,
>  #endif
> 
>          p2m->stats.mappings[level]--;
> -        /* Nothing to do if the entry is a super-page. */
> -        if ( level == 3 )
> -            p2m_put_l3_page(entry);
> +        p2m_put_page(entry, level);
> +
>          return;
>      }
> 
> --
> 2.34.1
> 

~Michal

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Julien Grall]

Hi Luca,

On 23/04/2024 09:25, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> We are doing foreign memory mapping for static shared memory, and
> there is a great possibility that it could be super mapped.

Is this because we are mapping more than one page at the time? Can you 
point me to the code?

> But today, p2m_put_l3_page could not handle superpages.

This was done on purpose. Xen is not preemptible and therefore we need 
to be cautious how much work is done within the p2m code.

With the below proposal, for 1GB mapping, we may end up to call 
put_page() up to 512 * 512 = 262144 times. put_page() can free memory. 
This could be a very long operation.

Have you benchmark how long it would take?

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Luca Fancellu]

Hi Julien,

> On 7 May 2024, at 14:20, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 23/04/2024 09:25, Luca Fancellu wrote:
>> From: Penny Zheng <Penny.Zheng@arm.com>
>> We are doing foreign memory mapping for static shared memory, and
>> there is a great possibility that it could be super mapped.
> 
> Is this because we are mapping more than one page at the time? Can you point me to the code?

So, to be honest this patch was originally in Penny’s serie, my knowledge of this side of the codebase
is very limited and so I pushed this one basically untouched.

From what I can see in the serie the mappings are made in handle_shared_mem_bank, and map_regions_p2mt
is called for one page at the time (allocated through the function allocate_domheap_memory (new function introduced in
the serie).

So is that the case that this patch is not needed?


> 
>> But today, p2m_put_l3_page could not handle superpages.
> 
> This was done on purpose. Xen is not preemptible and therefore we need to be cautious how much work is done within the p2m code.
> 
> With the below proposal, for 1GB mapping, we may end up to call put_page() up to 512 * 512 = 262144 times. put_page() can free memory. This could be a very long operation.
> 
> Have you benchmark how long it would take?

I did not, since its purpose was unclear to me and was not commented in the last serie from Penny.

Cheers,
Luca

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Julien Grall]

On 07/05/2024 14:30, Luca Fancellu wrote:
> Hi Julien,

Hi Luca,

>> On 7 May 2024, at 14:20, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Luca,
>>
>> On 23/04/2024 09:25, Luca Fancellu wrote:
>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>> We are doing foreign memory mapping for static shared memory, and
>>> there is a great possibility that it could be super mapped.
>>
>> Is this because we are mapping more than one page at the time? Can you point me to the code?
> 
> So, to be honest this patch was originally in Penny’s serie, my knowledge of this side of the codebase
> is very limited and so I pushed this one basically untouched.
> 
>  From what I can see in the serie the mappings are made in handle_shared_mem_bank, and map_regions_p2mt
> is called for one page at the time (allocated through the function allocate_domheap_memory (new function introduced in
> the serie).
> 
> So is that the case that this patch is not needed?

I looked at the code and, if I am not mistake, we are passing 
PFN_DOWN(psize) to map_regions_p2mt. At the moment, it is unclear to me 
why would psize be < PAGE_SIZE.

>>> But today, p2m_put_l3_page could not handle superpages.
>>
>> This was done on purpose. Xen is not preemptible and therefore we need to be cautious how much work is done within the p2m code.
>>
>> With the below proposal, for 1GB mapping, we may end up to call put_page() up to 512 * 512 = 262144 times. put_page() can free memory. This could be a very long operation.
>>
>> Have you benchmark how long it would take?
> 
> I did not, since its purpose was unclear to me and was not commented in the last serie from Penny.

Honestly, I can't remember why it wasn't commented.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Julien Grall]

Hi,

CC-ing Roger as he is working on adding support for the foreign mapping 
on x86. Although, I am not expecting any implication as only 4KB mapping 
should be supported.

On 08/05/2024 22:05, Julien Grall wrote:
> On 07/05/2024 14:30, Luca Fancellu wrote:
>>> On 7 May 2024, at 14:20, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Luca,
>>>
>>> On 23/04/2024 09:25, Luca Fancellu wrote:
>>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>>> But today, p2m_put_l3_page could not handle superpages.
>>>
>>> This was done on purpose. Xen is not preemptible and therefore we 
>>> need to be cautious how much work is done within the p2m code.
>>>
>>> With the below proposal, for 1GB mapping, we may end up to call 
>>> put_page() up to 512 * 512 = 262144 times. put_page() can free 
>>> memory. This could be a very long operation.
>>>
>>> Have you benchmark how long it would take?
>>
>> I did not, since its purpose was unclear to me and was not commented 
>> in the last serie from Penny.
> 
> Honestly, I can't remember why it wasn't commented.

I skimmed through the code to check what we currently do for preemption.

{decrease, increase}_reservation() will allow to handle max_order() 
mapping at the time. On a default configuration, the max would be 4MB.

relinquish_p2m_mapping() is preempting every 512 iterations. One 
iteration is either a 4KB/2MB/1GB mapping.

relinquish_memory() is checking for preemption after every page.

So I think, it would be ok to allow 2MB mapping for static shared memory 
but not 1GB. relinquish_p2m_mapping() would also needs to be updated to 
take into account the larger foreign mapping.

I would consider to check for preemption if 't' is p2m_map_foreign and 
the order is above 9 (i.e. 2MB).

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Roger Pau Monné]

On Wed, May 08, 2024 at 11:11:04PM +0100, Julien Grall wrote:
> Hi,
> 
> CC-ing Roger as he is working on adding support for the foreign mapping on
> x86. Although, I am not expecting any implication as only 4KB mapping should
> be supported.

I don't think we have plans on x86 to support foreign mappings with
order != 0 ATM.

We would need a new interface to allow creating such mappings, and
it's also not clear to me how the domain that creates such mappings
can identify super-pages on the remote domain.  IOW: the mapping
domain could request a super-page in the foreign domain gfn space,
but that could end up being a range of lower order mappings.

Also the interactions with the remote domain would need to be audited,
as the remote domain shattering the superpage would need to be
replicated in the mapping side in order to account for the changes.

> On 08/05/2024 22:05, Julien Grall wrote:
> > On 07/05/2024 14:30, Luca Fancellu wrote:
> > > > On 7 May 2024, at 14:20, Julien Grall <julien@xen.org> wrote:
> > > > 
> > > > Hi Luca,
> > > > 
> > > > On 23/04/2024 09:25, Luca Fancellu wrote:
> > > > > From: Penny Zheng <Penny.Zheng@arm.com>
> > > > > But today, p2m_put_l3_page could not handle superpages.
> > > > 
> > > > This was done on purpose. Xen is not preemptible and therefore
> > > > we need to be cautious how much work is done within the p2m
> > > > code.
> > > > 
> > > > With the below proposal, for 1GB mapping, we may end up to call
> > > > put_page() up to 512 * 512 = 262144 times. put_page() can free
> > > > memory. This could be a very long operation.
> > > > 
> > > > Have you benchmark how long it would take?
> > > 
> > > I did not, since its purpose was unclear to me and was not commented
> > > in the last serie from Penny.
> > 
> > Honestly, I can't remember why it wasn't commented.
> 
> I skimmed through the code to check what we currently do for preemption.
> 
> {decrease, increase}_reservation() will allow to handle max_order() mapping
> at the time. On a default configuration, the max would be 4MB.
> 
> relinquish_p2m_mapping() is preempting every 512 iterations. One iteration
> is either a 4KB/2MB/1GB mapping.
> 
> relinquish_memory() is checking for preemption after every page.
> 
> So I think, it would be ok to allow 2MB mapping for static shared memory but
> not 1GB. relinquish_p2m_mapping() would also needs to be updated to take
> into account the larger foreign mapping.

FWIW, relinquish_p2m_mapping() likely does more than what's strictly
needed, as you could just remove foreign mappings while leaving other
entries as-is?  The drain of the p2m pool and release of domain pages
should take care of dropping references to the RAM domain memory?

> I would consider to check for preemption if 't' is p2m_map_foreign and the
> order is above 9 (i.e. 2MB).

How can those mappings be removed?  Is it possible for the guest to
modify such foreign super-pages?  Not sure all paths will be easy to
audit for preemption if it's more than relinquish_p2m_mapping() that
you need to adjust.

Regards, Roger.

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Julien Grall]

On 09/05/2024 09:13, Roger Pau Monné wrote:
> On Wed, May 08, 2024 at 11:11:04PM +0100, Julien Grall wrote:
>> Hi,
>>
>> CC-ing Roger as he is working on adding support for the foreign mapping on
>> x86. Although, I am not expecting any implication as only 4KB mapping should
>> be supported.
> 
> I don't think we have plans on x86 to support foreign mappings with
> order != 0 ATM.
> 
> We would need a new interface to allow creating such mappings, and
> it's also not clear to me how the domain that creates such mappings
> can identify super-pages on the remote domain.  IOW: the mapping
> domain could request a super-page in the foreign domain gfn space,
> but that could end up being a range of lower order mappings.

I agree with this. But ...

> 
> Also the interactions with the remote domain would need to be audited,
> as the remote domain shattering the superpage would need to be
> replicated in the mapping side in order to account for the changes.

... I don't understand this one. How is this different from today's 
where a domain can foreign map a 2MB which may be using a superpage in 
the remote domain?

> 
>> On 08/05/2024 22:05, Julien Grall wrote:
>>> On 07/05/2024 14:30, Luca Fancellu wrote:
>>>>> On 7 May 2024, at 14:20, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>> Hi Luca,
>>>>>
>>>>> On 23/04/2024 09:25, Luca Fancellu wrote:
>>>>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>>>>> But today, p2m_put_l3_page could not handle superpages.
>>>>>
>>>>> This was done on purpose. Xen is not preemptible and therefore
>>>>> we need to be cautious how much work is done within the p2m
>>>>> code.
>>>>>
>>>>> With the below proposal, for 1GB mapping, we may end up to call
>>>>> put_page() up to 512 * 512 = 262144 times. put_page() can free
>>>>> memory. This could be a very long operation.
>>>>>
>>>>> Have you benchmark how long it would take?
>>>>
>>>> I did not, since its purpose was unclear to me and was not commented
>>>> in the last serie from Penny.
>>>
>>> Honestly, I can't remember why it wasn't commented.
>>
>> I skimmed through the code to check what we currently do for preemption.
>>
>> {decrease, increase}_reservation() will allow to handle max_order() mapping
>> at the time. On a default configuration, the max would be 4MB.
>>
>> relinquish_p2m_mapping() is preempting every 512 iterations. One iteration
>> is either a 4KB/2MB/1GB mapping.
>>
>> relinquish_memory() is checking for preemption after every page.
>>
>> So I think, it would be ok to allow 2MB mapping for static shared memory but
>> not 1GB. relinquish_p2m_mapping() would also needs to be updated to take
>> into account the larger foreign mapping.
> 
> FWIW, relinquish_p2m_mapping() likely does more than what's strictly
> needed, as you could just remove foreign mappings while leaving other
> entries as-is?  The drain of the p2m pool and release of domain pages
> should take care of dropping references to the RAM domain memory?
I believe the code was written in a way we could easily introduce 
reference for all the mappings. This has been discussed a few times in 
the past but we never implemented it.

> 
>> I would consider to check for preemption if 't' is p2m_map_foreign and the
>> order is above 9 (i.e. 2MB).
> 
> How can those mappings be removed?

 From any p2m_* functions. On Arm we don't (yet) care about which 
mapping is replaced.

>  Is it possible for the guest to
> modify such foreign super-pages?

Yes.

>  Not sure all paths will be easy to
> audit for preemption if it's more than relinquish_p2m_mapping() that
> you need to adjust.

I thought about it yesterday. But I came to the conclusion that if we 
have any concern about removing 1GB foreign superpage then we would 
already have the problem today as a domain can map contiguously 1GB 
worth of foreign mapping using small pages.

I went through the various code paths and, I believe, the only concern 
would be if an admin decides to change the values from max_order() using 
the command line option "memop-max-order".

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Roger Pau Monné]

On Thu, May 09, 2024 at 10:50:56AM +0100, Julien Grall wrote:
> 
> 
> On 09/05/2024 09:13, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 11:11:04PM +0100, Julien Grall wrote:
> > Also the interactions with the remote domain would need to be audited,
> > as the remote domain shattering the superpage would need to be
> > replicated in the mapping side in order to account for the changes.
> 
> ... I don't understand this one. How is this different from today's where a
> domain can foreign map a 2MB which may be using a superpage in the remote
> domain?

Hm, right, I was wrong with that I think, as long as proper references
as taken for the superpage entries it should be fine.

> >  Not sure all paths will be easy to
> > audit for preemption if it's more than relinquish_p2m_mapping() that
> > you need to adjust.
> 
> I thought about it yesterday. But I came to the conclusion that if we have
> any concern about removing 1GB foreign superpage then we would already have
> the problem today as a domain can map contiguously 1GB worth of foreign
> mapping using small pages.

Yeah, but in that case addition or removal is done in 4K chunks, and
hence we can preempt during the operation.

OTOH for 1GB given the code here the page could be freed in one go,
without a chance of preempting the operation.

Maybe you have to shatter superpages into 4K entries and then remove
them individually, as to allow for preemption to be possible by
calling put_page() for each 4K chunk?

Thanks, Roger.

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Julien Grall]

Hi,

On 09/05/2024 12:28, Roger Pau Monné wrote:
> On Thu, May 09, 2024 at 10:50:56AM +0100, Julien Grall wrote:
>>
>>
>> On 09/05/2024 09:13, Roger Pau Monné wrote:
>>> On Wed, May 08, 2024 at 11:11:04PM +0100, Julien Grall wrote:
>>> Also the interactions with the remote domain would need to be audited,
>>> as the remote domain shattering the superpage would need to be
>>> replicated in the mapping side in order to account for the changes.
>>
>> ... I don't understand this one. How is this different from today's where a
>> domain can foreign map a 2MB which may be using a superpage in the remote
>> domain?
> 
> Hm, right, I was wrong with that I think, as long as proper references
> as taken for the superpage entries it should be fine.
> 
>>>   Not sure all paths will be easy to
>>> audit for preemption if it's more than relinquish_p2m_mapping() that
>>> you need to adjust.
>>
>> I thought about it yesterday. But I came to the conclusion that if we have
>> any concern about removing 1GB foreign superpage then we would already have
>> the problem today as a domain can map contiguously 1GB worth of foreign
>> mapping using small pages.
> 
> Yeah, but in that case addition or removal is done in 4K chunks, and
> hence we can preempt during the operation.

I am not entirely sure how that would work. From my understand, today, 
most of the users of the P2M code expects the operation to complete in 
one go and if preemption is needed then the caller is responsible to 
handle it by breaking up the happy.

With your suggestion, it sounds like you want to rework how the 
preemption today and push it to the P2M code. This would mean we would 
need to modify all the callers to check for -EERESTART (or similar) and 
also tell them how many pages were handled so the call can be restarted 
where it stopped. Is it what you had in mind?

I don't expect the work to be trivial, so I wonder if this is really 
worth it to try to change the way we preempt.

> 
> OTOH for 1GB given the code here the page could be freed in one go,
> without a chance of preempting the operation.
> 
> Maybe you have to shatter superpages into 4K entries and then remove
> them individually, as to allow for preemption to be possible by
> calling put_page() for each 4K chunk?
This would require to allocate some pages from the P2M pool for the 
tables. As the pool may be exhausted, it could be problematic when 
relinquishing the resources.

It may be possible to find a way to have memory available by removing 
other mappings first. But it feels a bit hackish and I would rather 
prefer if we avoid allocating any memory when relinquishing.

So I think we want to allow at most 2MB superpages for foreign mapping.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Roger Pau Monné]

On Thu, May 09, 2024 at 01:12:00PM +0100, Julien Grall wrote:
> Hi,
> 
> On 09/05/2024 12:28, Roger Pau Monné wrote:
> > On Thu, May 09, 2024 at 10:50:56AM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 09/05/2024 09:13, Roger Pau Monné wrote:
> > > > On Wed, May 08, 2024 at 11:11:04PM +0100, Julien Grall wrote:
> > > > Also the interactions with the remote domain would need to be audited,
> > > > as the remote domain shattering the superpage would need to be
> > > > replicated in the mapping side in order to account for the changes.
> > > 
> > > ... I don't understand this one. How is this different from today's where a
> > > domain can foreign map a 2MB which may be using a superpage in the remote
> > > domain?
> > 
> > Hm, right, I was wrong with that I think, as long as proper references
> > as taken for the superpage entries it should be fine.
> > 
> > > >   Not sure all paths will be easy to
> > > > audit for preemption if it's more than relinquish_p2m_mapping() that
> > > > you need to adjust.
> > > 
> > > I thought about it yesterday. But I came to the conclusion that if we have
> > > any concern about removing 1GB foreign superpage then we would already have
> > > the problem today as a domain can map contiguously 1GB worth of foreign
> > > mapping using small pages.
> > 
> > Yeah, but in that case addition or removal is done in 4K chunks, and
> > hence we can preempt during the operation.
> 
> I am not entirely sure how that would work. From my understand, today, most
> of the users of the P2M code expects the operation to complete in one go and
> if preemption is needed then the caller is responsible to handle it by
> breaking up the happy.
> 
> With your suggestion, it sounds like you want to rework how the preemption
> today and push it to the P2M code. This would mean we would need to modify
> all the callers to check for -EERESTART (or similar) and also tell them how
> many pages were handled so the call can be restarted where it stopped. Is it
> what you had in mind?

TBH, I didn't have a specific location in mind about where to do the
split.

One solution that could simplify it is allowing foreign entries to
only be removed by specific functions, so that the split required in
order to do the removal can be handled by the caller knowing it's
dealing with a foreign map superpage.

But that would require the superpage to be shattered, and hence will
require creating lower levle leaf entries in order to do the
shattering and the removal in 4K chunks.

> I don't expect the work to be trivial, so I wonder if this is really worth
> it to try to change the way we preempt.
> 
> > 
> > OTOH for 1GB given the code here the page could be freed in one go,
> > without a chance of preempting the operation.
> > 
> > Maybe you have to shatter superpages into 4K entries and then remove
> > them individually, as to allow for preemption to be possible by
> > calling put_page() for each 4K chunk?
> This would require to allocate some pages from the P2M pool for the tables.
> As the pool may be exhausted, it could be problematic when relinquishing the
> resources.

Indeed, it's not ideal.

> It may be possible to find a way to have memory available by removing other
> mappings first. But it feels a bit hackish and I would rather prefer if we
> avoid allocating any memory when relinquishing.

Maybe it could be helpful to provide a function to put a superpage,
that internally calls free_domheap_pages() with the appropriate order
so that freeing a superpage only takes a single free_domheap_pages()
call.  That could reduce some of the contention around the heap_lock
and d->page_alloc_lock locks.

Note also that a foreign unmap resulting in a page free is also not
the common case, as that should only happen when the foreign domain
has been destroyed, or the page ballooned out, so to benchmark the
worst case some effort will be needed in order to model this
scenario.

Thanks, Roger.

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Julien Grall]

Hi Roger,

On 09/05/2024 13:58, Roger Pau Monné wrote:
> On Thu, May 09, 2024 at 01:12:00PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 09/05/2024 12:28, Roger Pau Monné wrote:
>>> On Thu, May 09, 2024 at 10:50:56AM +0100, Julien Grall wrote:
>>>>
>>>>
>>>> On 09/05/2024 09:13, Roger Pau Monné wrote:
>>>>> On Wed, May 08, 2024 at 11:11:04PM +0100, Julien Grall wrote:
>>>>> Also the interactions with the remote domain would need to be audited,
>>>>> as the remote domain shattering the superpage would need to be
>>>>> replicated in the mapping side in order to account for the changes.
>>>>
>>>> ... I don't understand this one. How is this different from today's where a
>>>> domain can foreign map a 2MB which may be using a superpage in the remote
>>>> domain?
>>>
>>> Hm, right, I was wrong with that I think, as long as proper references
>>> as taken for the superpage entries it should be fine.
>>>
>>>>>    Not sure all paths will be easy to
>>>>> audit for preemption if it's more than relinquish_p2m_mapping() that
>>>>> you need to adjust.
>>>>
>>>> I thought about it yesterday. But I came to the conclusion that if we have
>>>> any concern about removing 1GB foreign superpage then we would already have
>>>> the problem today as a domain can map contiguously 1GB worth of foreign
>>>> mapping using small pages.
>>>
>>> Yeah, but in that case addition or removal is done in 4K chunks, and
>>> hence we can preempt during the operation.
>>
>> I am not entirely sure how that would work. From my understand, today, most
>> of the users of the P2M code expects the operation to complete in one go and
>> if preemption is needed then the caller is responsible to handle it by
>> breaking up the happy.
>>
>> With your suggestion, it sounds like you want to rework how the preemption
>> today and push it to the P2M code. This would mean we would need to modify
>> all the callers to check for -EERESTART (or similar) and also tell them how
>> many pages were handled so the call can be restarted where it stopped. Is it
>> what you had in mind?
> 
> TBH, I didn't have a specific location in mind about where to do the
> split.
> 
> One solution that could simplify it is allowing foreign entries to
> only be removed by specific functions, so that the split required in
> order to do the removal can be handled by the caller knowing it's
> dealing with a foreign map superpage.

That would work. It would require quite a bit of work though.

> But that would require the superpage to be shattered, and hence will
> require creating lower levle leaf entries in order to do the
> shattering and the removal in 4K chunks.
> 
>> I don't expect the work to be trivial, so I wonder if this is really worth
>> it to try to change the way we preempt.
>>
>>>
>>> OTOH for 1GB given the code here the page could be freed in one go,
>>> without a chance of preempting the operation.
>>>
>>> Maybe you have to shatter superpages into 4K entries and then remove
>>> them individually, as to allow for preemption to be possible by
>>> calling put_page() for each 4K chunk?
>> This would require to allocate some pages from the P2M pool for the tables.
>> As the pool may be exhausted, it could be problematic when relinquishing the
>> resources.
> 
> Indeed, it's not ideal.
> 
>> It may be possible to find a way to have memory available by removing other
>> mappings first. But it feels a bit hackish and I would rather prefer if we
>> avoid allocating any memory when relinquishing.
> 
> Maybe it could be helpful to provide a function to put a superpage,
> that internally calls free_domheap_pages() with the appropriate order
> so that freeing a superpage only takes a single free_domheap_pages()
> call. 

Today, free_domheap_page() is only called when the last reference is 
removed. I don't thinkt here is any guarantee that all the references 
will dropped at the same time.

 >  That could reduce some of the contention around the heap_lock
 > and d->page_alloc_lock locks.

 From previous experience (when Hongyan and I worked on optimizing 
init_heap_pages() for Live-Update), the lock is actually not the biggest 
problem. The issue is adding the pages back to the heap (which may 
requiring merging). So as long as the pages are not freed contiguously, 
we may not gain anything.

Anyway, it sound like someone needs to spend some time investigating 
this issue.

> 
> Note also that a foreign unmap resulting in a page free is also not
> the common case, as that should only happen when the foreign domain
> has been destroyed, or the page ballooned out, so to benchmark the
> worst case some effort will be needed in order to model this
> scenario.

Good callout. It may be easier to reproduce it with some XTF tests. 
Unfortunately, I don't have the bandwith to look at it. Maybe Luca can?

Otherwise, we may have to accept not supporting 1GB superpage for the 
time being for shared memory. But I am not actually sure this is a big 
problem?

Cheers,

-- 
Julien Grall

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Roger Pau Monné]

On Fri, May 10, 2024 at 10:37:53PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 09/05/2024 13:58, Roger Pau Monné wrote:
> > On Thu, May 09, 2024 at 01:12:00PM +0100, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 09/05/2024 12:28, Roger Pau Monné wrote:
> > > > OTOH for 1GB given the code here the page could be freed in one go,
> > > > without a chance of preempting the operation.
> > > > 
> > > > Maybe you have to shatter superpages into 4K entries and then remove
> > > > them individually, as to allow for preemption to be possible by
> > > > calling put_page() for each 4K chunk?
> > > This would require to allocate some pages from the P2M pool for the tables.
> > > As the pool may be exhausted, it could be problematic when relinquishing the
> > > resources.
> > 
> > Indeed, it's not ideal.
> > 
> > > It may be possible to find a way to have memory available by removing other
> > > mappings first. But it feels a bit hackish and I would rather prefer if we
> > > avoid allocating any memory when relinquishing.
> > 
> > Maybe it could be helpful to provide a function to put a superpage,
> > that internally calls free_domheap_pages() with the appropriate order
> > so that freeing a superpage only takes a single free_domheap_pages()
> > call.
> 
> Today, free_domheap_page() is only called when the last reference is
> removed. I don't thinkt here is any guarantee that all the references will
> dropped at the same time.

I see, yes, we have no guarantee that removing the superpage entry in
the mapping domain will lead to either the whole superpage freed at
once, or not freed.  The source domain may have shattered the
super-page and hence freeing might need to be done at a smaller
granularity.

> >  That could reduce some of the contention around the heap_lock
> > and d->page_alloc_lock locks.
> 
> From previous experience (when Hongyan and I worked on optimizing
> init_heap_pages() for Live-Update), the lock is actually not the biggest
> problem. The issue is adding the pages back to the heap (which may requiring
> merging). So as long as the pages are not freed contiguously, we may not
> gain anything.

Would it help to defer the merging to the idle context, kind of
similar to what we do with scrubbing?

Thanks, Roger.

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Luca Fancellu]

Hi Julien,

> 
>> Note also that a foreign unmap resulting in a page free is also not
>> the common case, as that should only happen when the foreign domain
>> has been destroyed, or the page ballooned out, so to benchmark the
>> worst case some effort will be needed in order to model this
>> scenario.
> 
> Good callout. It may be easier to reproduce it with some XTF tests. Unfortunately, I don't have the bandwith to look at it. Maybe Luca can?

Unfortunately I don’t have bandwidth for that,

> 
> Otherwise, we may have to accept not supporting 1GB superpage for the time being for shared memory.

I will try to do it. 

Cheers,
Luca

Re: [PATCH 3/7] xen/p2m: put reference for superpage

[Roger Pau Monné]

On Tue, Apr 23, 2024 at 09:25:28AM +0100, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> We are doing foreign memory mapping for static shared memory, and
> there is a great possibility that it could be super mapped.
> But today, p2m_put_l3_page could not handle superpages.
> 
> This commits implements a new function p2m_put_superpage to handle superpages,
> specifically for helping put extra references for foreign superpages.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v1:
>  - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-Penny.Zheng@arm.com/
> ---
>  xen/arch/arm/mmu/p2m.c | 58 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 41fcca011cf4..479a80fbd4cf 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
>      return rc;
>  }
>  
> -/*
> - * Put any references on the single 4K page referenced by pte.
> - * TODO: Handle superpages, for now we only take special references for leaf
> - * pages (specifically foreign ones, which can't be super mapped today).
> - */
> -static void p2m_put_l3_page(const lpae_t pte)
> +/* Put any references on the single 4K page referenced by mfn. */
> +static void p2m_put_l3_page(mfn_t mfn, unsigned type)
                                          ^ p2m_type_t?

Roger.

[PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided

[Luca Fancellu]

Handle the parsing of the 'xen,shared-mem' property when the host physical
address is not provided, this commit is introducing the logic to parse it,
but the functionality is still not implemented and will be part of future
commits.

Rework the logic inside process_shm_node to check the shm_id before doing
the other checks, because it ease the logic itself, add more comment on
the logic.
Now when the host physical address is not provided, the value
INVALID_PADDR is chosen to signal this condition and it is stored as
start of the bank, due to that change also early_print_info_shmem and
init_sharedmem_pages are changed, to don't handle banks with start equal
to INVALID_PADDR.

Another change is done inside meminfo_overlap_check, to skip banks that
are starting with the start address INVALID_PADDR, that function is used
to check banks from reserved memory and ACPI and it's unlikely for these
bank to have the start address as INVALID_PADDR. The change holds
because of this consideration.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/setup.c        |   3 +-
 xen/arch/arm/static-shmem.c | 129 +++++++++++++++++++++++++-----------
 2 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d242674381d4..f15d40a85a5f 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -297,7 +297,8 @@ static bool __init meminfo_overlap_check(const struct membanks *mem,
         bank_start = mem->bank[i].start;
         bank_end = bank_start + mem->bank[i].size;
 
-        if ( region_end <= bank_start || region_start >= bank_end )
+        if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
+             region_start >= bank_end )
             continue;
         else
         {
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 24e40495a481..1c03bb7f1882 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -264,6 +264,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         pbase = boot_shm_bank->start;
         psize = boot_shm_bank->size;
 
+        if ( INVALID_PADDR == pbase )
+        {
+            printk("%pd: host physical address must be chosen by users at the moment.", d);
+            return -EINVAL;
+        }
+
         /*
          * xen,shared-mem = <pbase, gbase, size>;
          * TODO: pbase is optional.
@@ -382,7 +388,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
 {
     const struct fdt_property *prop, *prop_id, *prop_role;
     const __be32 *cell;
-    paddr_t paddr, gaddr, size, end;
+    paddr_t paddr = INVALID_PADDR;
+    paddr_t gaddr, size, end;
     struct membanks *mem = bootinfo_get_shmem();
     struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra();
     unsigned int i;
@@ -437,24 +444,37 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
     if ( !prop )
         return -ENOENT;
 
+    cell = (const __be32 *)prop->data;
     if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
     {
-        if ( len == dt_cells_to_size(size_cells + address_cells) )
-            printk("fdt: host physical address must be chosen by users at the moment.\n");
-
-        printk("fdt: invalid `xen,shared-mem` property.\n");
-        return -EINVAL;
+        if ( len == dt_cells_to_size(address_cells + size_cells) )
+            device_tree_get_reg(&cell, address_cells, size_cells, &gaddr,
+                                &size);
+        else
+        {
+            printk("fdt: invalid `xen,shared-mem` property.\n");
+            return -EINVAL;
+        }
     }
+    else
+    {
+        device_tree_get_reg(&cell, address_cells, address_cells, &paddr,
+                            &gaddr);
+        size = dt_next_cell(size_cells, &cell);
 
-    cell = (const __be32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
-    size = dt_next_cell(size_cells, &cell);
+        if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
+        {
+            printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
+                paddr);
+            return -EINVAL;
+        }
 
-    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
-    {
-        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
-               paddr);
-        return -EINVAL;
+        end = paddr + size;
+        if ( end <= paddr )
+        {
+            printk("fdt: static shared memory region %s overflow\n", shm_id);
+            return -EINVAL;
+        }
     }
 
     if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
@@ -476,41 +496,69 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
         return -EINVAL;
     }
 
-    end = paddr + size;
-    if ( end <= paddr )
-    {
-        printk("fdt: static shared memory region %s overflow\n", shm_id);
-        return -EINVAL;
-    }
-
     for ( i = 0; i < mem->nr_banks; i++ )
     {
         /*
          * Meet the following check:
+         * when host address is provided:
          * 1) The shm ID matches and the region exactly match
          * 2) The shm ID doesn't match and the region doesn't overlap
          * with an existing one
+         * when host address is not provided:
+         * 1) The shm ID matches and the region size exactly match
          */
-        if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
+        bool paddr_assigned = INVALID_PADDR == paddr;
+        bool shm_id_match = strncmp(shm_id, shmem_extra[i].shm_id,
+                                    MAX_SHM_ID_LENGTH) == 0;
+        if ( shm_id_match )
         {
-            if ( strncmp(shm_id, shmem_extra[i].shm_id,
-                         MAX_SHM_ID_LENGTH) == 0  )
+            /*
+             * Regions have same shm_id (cases):
+             * 1) physical host address is supplied:
+             *    - OK:   paddr is equal and size is equal (same region)
+             *    - Fail: paddr doesn't match or size doesn't match (there
+             *            cannot exists two shmem regions with same shm_id)
+             * 2) physical host address is NOT supplied:
+             *    - OK:   size is equal (same region)
+             *    - Fail: size is not equal (same shm_id must identify only one
+             *            region, there can't be two different regions with same
+             *            shm_id)
+             */
+            bool start_match = paddr_assigned ? (paddr == mem->bank[i].start) :
+                                                true;
+
+            if ( start_match && size == mem->bank[i].size )
                 break;
             else
             {
-                printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
+                printk("fdt: different shared memory region could not share the same shm ID %s\n",
                        shm_id);
                 return -EINVAL;
             }
         }
-        else if ( strncmp(shm_id, shmem_extra[i].shm_id,
-                          MAX_SHM_ID_LENGTH) != 0 )
-            continue;
         else
         {
-            printk("fdt: different shared memory region could not share the same shm ID %s\n",
-                   shm_id);
-            return -EINVAL;
+            /*
+             * Regions have different shm_id (cases):
+             * 1) physical host address is supplied:
+             *    - OK:   paddr different, or size different (case where paddr
+             *            is equal but psize is different are wrong, but they
+             *            are handled later when checking for overlapping)
+             *    - Fail: paddr equal and size equal (the same region can't be
+             *            identified with different shm_id)
+             * 2) physical host address is NOT supplied:
+             *    - OK:   Both have different shm_id so even with same size they
+             *            can exists
+             */
+            if ( !paddr_assigned || paddr != mem->bank[i].start ||
+                 size != mem->bank[i].size )
+                continue;
+            else
+            {
+                printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
+                       shm_id);
+                return -EINVAL;
+            }
         }
     }
 
@@ -518,7 +566,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
     {
         if (i < mem->max_banks)
         {
-            if ( check_reserved_regions_overlap(paddr, size) )
+            if ( (paddr != INVALID_PADDR) &&
+                 check_reserved_regions_overlap(paddr, size) )
                 return -EINVAL;
 
             /* Static shared memory shall be reserved from any other use. */
@@ -588,13 +637,16 @@ void __init early_print_info_shmem(void)
 {
     const struct membanks *shmem = bootinfo_get_shmem();
     unsigned int bank;
+    unsigned int printed = 0;
 
     for ( bank = 0; bank < shmem->nr_banks; bank++ )
-    {
-        printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", bank,
-               shmem->bank[bank].start,
-               shmem->bank[bank].start + shmem->bank[bank].size - 1);
-    }
+        if ( shmem->bank[bank].start != INVALID_PADDR )
+        {
+            printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", printed,
+                shmem->bank[bank].start,
+                shmem->bank[bank].start + shmem->bank[bank].size - 1);
+            printed++;
+        }
 }
 
 void __init init_sharedmem_pages(void)
@@ -603,7 +655,8 @@ void __init init_sharedmem_pages(void)
     unsigned int bank;
 
     for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
-        init_staticmem_bank(&shmem->bank[bank]);
+        if ( shmem->bank[bank].start != INVALID_PADDR )
+            init_staticmem_bank(&shmem->bank[bank]);
 }
 
 int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
-- 
2.34.1

Re: [PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> Handle the parsing of the 'xen,shared-mem' property when the host physical
> address is not provided, this commit is introducing the logic to parse it,
> but the functionality is still not implemented and will be part of future
> commits.
> 
> Rework the logic inside process_shm_node to check the shm_id before doing
> the other checks, because it ease the logic itself, add more comment on
> the logic.
> Now when the host physical address is not provided, the value
> INVALID_PADDR is chosen to signal this condition and it is stored as
> start of the bank, due to that change also early_print_info_shmem and
> init_sharedmem_pages are changed, to don't handle banks with start equal
> to INVALID_PADDR.
> 
> Another change is done inside meminfo_overlap_check, to skip banks that
> are starting with the start address INVALID_PADDR, that function is used
> to check banks from reserved memory and ACPI and it's unlikely for these
also from shmem

> bank to have the start address as INVALID_PADDR. The change holds
> because of this consideration.
On arm64 and LPAE arm32 we don't have this problem. In theory we could have a bank
starting at INVALID_PADDR if PA range was 32bit but as the comment above the function states,
wrapping around is not handled. You might want to use it as a justification to be clear.

> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/setup.c        |   3 +-
>  xen/arch/arm/static-shmem.c | 129 +++++++++++++++++++++++++-----------
>  2 files changed, 93 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d242674381d4..f15d40a85a5f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -297,7 +297,8 @@ static bool __init meminfo_overlap_check(const struct membanks *mem,
>          bank_start = mem->bank[i].start;
>          bank_end = bank_start + mem->bank[i].size;
> 
> -        if ( region_end <= bank_start || region_start >= bank_end )
> +        if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
> +             region_start >= bank_end )
>              continue;
>          else
>          {
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 24e40495a481..1c03bb7f1882 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -264,6 +264,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>          pbase = boot_shm_bank->start;
>          psize = boot_shm_bank->size;
> 
> +        if ( INVALID_PADDR == pbase )
> +        {
> +            printk("%pd: host physical address must be chosen by users at the moment.", d);
The dot at the end is not needed.

> +            return -EINVAL;
> +        }
> +
>          /*
>           * xen,shared-mem = <pbase, gbase, size>;
>           * TODO: pbase is optional.
> @@ -382,7 +388,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>  {
>      const struct fdt_property *prop, *prop_id, *prop_role;
>      const __be32 *cell;
> -    paddr_t paddr, gaddr, size, end;
> +    paddr_t paddr = INVALID_PADDR;
> +    paddr_t gaddr, size, end;
>      struct membanks *mem = bootinfo_get_shmem();
>      struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra();
>      unsigned int i;
> @@ -437,24 +444,37 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>      if ( !prop )
>          return -ENOENT;
> 
> +    cell = (const __be32 *)prop->data;
>      if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
>      {
> -        if ( len == dt_cells_to_size(size_cells + address_cells) )
> -            printk("fdt: host physical address must be chosen by users at the moment.\n");
> -
> -        printk("fdt: invalid `xen,shared-mem` property.\n");
> -        return -EINVAL;
> +        if ( len == dt_cells_to_size(address_cells + size_cells) )
> +            device_tree_get_reg(&cell, address_cells, size_cells, &gaddr,
> +                                &size);
> +        else
> +        {
> +            printk("fdt: invalid `xen,shared-mem` property.\n");
> +            return -EINVAL;
> +        }
>      }
> +    else
> +    {
> +        device_tree_get_reg(&cell, address_cells, address_cells, &paddr,
> +                            &gaddr);
> +        size = dt_next_cell(size_cells, &cell);
> 
> -    cell = (const __be32 *)prop->data;
> -    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
> -    size = dt_next_cell(size_cells, &cell);
> +        if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
> +        {
> +            printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
> +                paddr);
> +            return -EINVAL;
> +        }
> 
> -    if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
> -    {
> -        printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
> -               paddr);
> -        return -EINVAL;
> +        end = paddr + size;
> +        if ( end <= paddr )
> +        {
> +            printk("fdt: static shared memory region %s overflow\n", shm_id);
> +            return -EINVAL;
> +        }
>      }
> 
>      if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
> @@ -476,41 +496,69 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>          return -EINVAL;
>      }
> 
> -    end = paddr + size;
> -    if ( end <= paddr )
> -    {
> -        printk("fdt: static shared memory region %s overflow\n", shm_id);
> -        return -EINVAL;
> -    }
> -
>      for ( i = 0; i < mem->nr_banks; i++ )
>      {
>          /*
>           * Meet the following check:
> +         * when host address is provided:
- when would read better

>           * 1) The shm ID matches and the region exactly match
>           * 2) The shm ID doesn't match and the region doesn't overlap
>           * with an existing one
> +         * when host address is not provided:
> +         * 1) The shm ID matches and the region size exactly match
>           */
> -        if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> +        bool paddr_assigned = INVALID_PADDR == paddr;
parenthesis around INVALID_PADDR == paddr

> +        bool shm_id_match = strncmp(shm_id, shmem_extra[i].shm_id,
> +                                    MAX_SHM_ID_LENGTH) == 0;
why not if ( strncmp... given no other use of this variable other than the one below?

> +        if ( shm_id_match )
>          {
> -            if ( strncmp(shm_id, shmem_extra[i].shm_id,
> -                         MAX_SHM_ID_LENGTH) == 0  )
> +            /*
> +             * Regions have same shm_id (cases):
> +             * 1) physical host address is supplied:
> +             *    - OK:   paddr is equal and size is equal (same region)
> +             *    - Fail: paddr doesn't match or size doesn't match (there
> +             *            cannot exists two shmem regions with same shm_id)
> +             * 2) physical host address is NOT supplied:
> +             *    - OK:   size is equal (same region)
> +             *    - Fail: size is not equal (same shm_id must identify only one
> +             *            region, there can't be two different regions with same
> +             *            shm_id)
> +             */
> +            bool start_match = paddr_assigned ? (paddr == mem->bank[i].start) :
> +                                                true;
> +
> +            if ( start_match && size == mem->bank[i].size )
>                  break;
>              else
>              {
> -                printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
> +                printk("fdt: different shared memory region could not share the same shm ID %s\n",
>                         shm_id);
>                  return -EINVAL;
>              }
>          }
> -        else if ( strncmp(shm_id, shmem_extra[i].shm_id,
> -                          MAX_SHM_ID_LENGTH) != 0 )
> -            continue;
>          else
>          {
There is no need for this else and entire block given that the block within if either calls break or return

> -            printk("fdt: different shared memory region could not share the same shm ID %s\n",
> -                   shm_id);
> -            return -EINVAL;
> +            /*
> +             * Regions have different shm_id (cases):
> +             * 1) physical host address is supplied:
> +             *    - OK:   paddr different, or size different (case where paddr
> +             *            is equal but psize is different are wrong, but they
> +             *            are handled later when checking for overlapping)
> +             *    - Fail: paddr equal and size equal (the same region can't be
> +             *            identified with different shm_id)
> +             * 2) physical host address is NOT supplied:
> +             *    - OK:   Both have different shm_id so even with same size they
> +             *            can exists
> +             */
> +            if ( !paddr_assigned || paddr != mem->bank[i].start ||
> +                 size != mem->bank[i].size )
> +                continue;
> +            else
> +            {
> +                printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
> +                       shm_id);
> +                return -EINVAL;
> +            }
>          }
>      }
> 
> @@ -518,7 +566,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>      {
>          if (i < mem->max_banks)
>          {
> -            if ( check_reserved_regions_overlap(paddr, size) )
> +            if ( (paddr != INVALID_PADDR) &&
> +                 check_reserved_regions_overlap(paddr, size) )
>                  return -EINVAL;
> 
>              /* Static shared memory shall be reserved from any other use. */
> @@ -588,13 +637,16 @@ void __init early_print_info_shmem(void)
>  {
>      const struct membanks *shmem = bootinfo_get_shmem();
>      unsigned int bank;
> +    unsigned int printed = 0;
> 
>      for ( bank = 0; bank < shmem->nr_banks; bank++ )
> -    {
> -        printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", bank,
> -               shmem->bank[bank].start,
> -               shmem->bank[bank].start + shmem->bank[bank].size - 1);
> -    }
> +        if ( shmem->bank[bank].start != INVALID_PADDR )
> +        {
> +            printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", printed,
> +                shmem->bank[bank].start,
> +                shmem->bank[bank].start + shmem->bank[bank].size - 1);
> +            printed++;
NIT: you could initialize and increment it as part of the for loop

> +        }
>  }
> 
>  void __init init_sharedmem_pages(void)
> @@ -603,7 +655,8 @@ void __init init_sharedmem_pages(void)
>      unsigned int bank;
> 
>      for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
> -        init_staticmem_bank(&shmem->bank[bank]);
> +        if ( shmem->bank[bank].start != INVALID_PADDR )
> +            init_staticmem_bank(&shmem->bank[bank]);
>  }
> 
>  int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
> --
> 2.34.1
> 

~Michal

Re: [PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided

[Luca Fancellu]

Hi Michal,

> On 8 May 2024, at 13:09, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Luca,
> 
> On 23/04/2024 10:25, Luca Fancellu wrote:
>> 
>> 
>> Handle the parsing of the 'xen,shared-mem' property when the host physical
>> address is not provided, this commit is introducing the logic to parse it,
>> but the functionality is still not implemented and will be part of future
>> commits.
>> 
>> Rework the logic inside process_shm_node to check the shm_id before doing
>> the other checks, because it ease the logic itself, add more comment on
>> the logic.
>> Now when the host physical address is not provided, the value
>> INVALID_PADDR is chosen to signal this condition and it is stored as
>> start of the bank, due to that change also early_print_info_shmem and
>> init_sharedmem_pages are changed, to don't handle banks with start equal
>> to INVALID_PADDR.
>> 
>> Another change is done inside meminfo_overlap_check, to skip banks that
>> are starting with the start address INVALID_PADDR, that function is used
>> to check banks from reserved memory and ACPI and it's unlikely for these
> also from shmem
> 
>> bank to have the start address as INVALID_PADDR. The change holds
>> because of this consideration.
> On arm64 and LPAE arm32 we don't have this problem. In theory we could have a bank
> starting at INVALID_PADDR if PA range was 32bit but as the comment above the function states,
> wrapping around is not handled. You might want to use it as a justification to be clear.

Sure, I’ll rephrase it, is it ok something like this:

[...]
Another change is done inside meminfo_overlap_check, to skip banks that
are starting with the start address INVALID_PADDR, that function is used
to check banks from reserved memory, shared memory and ACPI and since
the comment above the function states that wrapping around is not handled,
it’s unlikely for these bank to have the start address as INVALID_PADDR.
The change holds because of this consideration.

>> 
>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>> index 24e40495a481..1c03bb7f1882 100644
>> --- a/xen/arch/arm/static-shmem.c
>> +++ b/xen/arch/arm/static-shmem.c
>> @@ -264,6 +264,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>         pbase = boot_shm_bank->start;
>>         psize = boot_shm_bank->size;
>> 
>> +        if ( INVALID_PADDR == pbase )
>> +        {
>> +            printk("%pd: host physical address must be chosen by users at the moment.", d);
> The dot at the end is not needed.
Will fix


>> 
>> -    end = paddr + size;
>> -    if ( end <= paddr )
>> -    {
>> -        printk("fdt: static shared memory region %s overflow\n", shm_id);
>> -        return -EINVAL;
>> -    }
>> -
>>     for ( i = 0; i < mem->nr_banks; i++ )
>>     {
>>         /*
>>          * Meet the following check:
>> +         * when host address is provided:
> - when would read better
Ok I’ll use hyphen instead of star, here and below

> 
>>          * 1) The shm ID matches and the region exactly match
>>          * 2) The shm ID doesn't match and the region doesn't overlap
>>          * with an existing one
>> +         * when host address is not provided:
>> +         * 1) The shm ID matches and the region size exactly match
>>          */
>> -        if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
>> +        bool paddr_assigned = INVALID_PADDR == paddr;
> parenthesis around INVALID_PADDR == paddr
Ok

> 
>> +        bool shm_id_match = strncmp(shm_id, shmem_extra[i].shm_id,
>> +                                    MAX_SHM_ID_LENGTH) == 0;
> why not if ( strncmp... given no other use of this variable other than the one below?

Yeah I think in some previous rework I was using multiple times and then I forgot to
change here, I’ll fix

> 
>> +        if ( shm_id_match )
>>         {
>> -            if ( strncmp(shm_id, shmem_extra[i].shm_id,
>> -                         MAX_SHM_ID_LENGTH) == 0  )
>> +            /*
>> +             * Regions have same shm_id (cases):
>> +             * 1) physical host address is supplied:
>> +             *    - OK:   paddr is equal and size is equal (same region)
>> +             *    - Fail: paddr doesn't match or size doesn't match (there
>> +             *            cannot exists two shmem regions with same shm_id)
>> +             * 2) physical host address is NOT supplied:
>> +             *    - OK:   size is equal (same region)
>> +             *    - Fail: size is not equal (same shm_id must identify only one
>> +             *            region, there can't be two different regions with same
>> +             *            shm_id)
>> +             */
>> +            bool start_match = paddr_assigned ? (paddr == mem->bank[i].start) :
>> +                                                true;
>> +
>> +            if ( start_match && size == mem->bank[i].size )
>>                 break;
>>             else
>>             {
>> -                printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
>> +                printk("fdt: different shared memory region could not share the same shm ID %s\n",
>>                        shm_id);
>>                 return -EINVAL;
>>             }
>>         }
>> -        else if ( strncmp(shm_id, shmem_extra[i].shm_id,
>> -                          MAX_SHM_ID_LENGTH) != 0 )
>> -            continue;
>>         else
>>         {
> There is no need for this else and entire block given that the block within if either calls break or return

There was a MISRA discussion about else at the end of if ... else if ... (R15.7) and I don’t remember
the outcome
>> 
>> @@ -588,13 +637,16 @@ void __init early_print_info_shmem(void)
>> {
>>     const struct membanks *shmem = bootinfo_get_shmem();
>>     unsigned int bank;
>> +    unsigned int printed = 0;
>> 
>>     for ( bank = 0; bank < shmem->nr_banks; bank++ )
>> -    {
>> -        printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", bank,
>> -               shmem->bank[bank].start,
>> -               shmem->bank[bank].start + shmem->bank[bank].size - 1);
>> -    }
>> +        if ( shmem->bank[bank].start != INVALID_PADDR )
>> +        {
>> +            printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", printed,
>> +                shmem->bank[bank].start,
>> +                shmem->bank[bank].start + shmem->bank[bank].size - 1);
>> +            printed++;
> NIT: you could initialize and increment it as part of the for loop
Sure, I’ll do

Re: [PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided

[Luca Fancellu]

Hi Michal,

>> 
>>> +        if ( shm_id_match )
>>>        {
>>> -            if ( strncmp(shm_id, shmem_extra[i].shm_id,
>>> -                         MAX_SHM_ID_LENGTH) == 0  )
>>> +            /*
>>> +             * Regions have same shm_id (cases):
>>> +             * 1) physical host address is supplied:
>>> +             *    - OK:   paddr is equal and size is equal (same region)
>>> +             *    - Fail: paddr doesn't match or size doesn't match (there
>>> +             *            cannot exists two shmem regions with same shm_id)
>>> +             * 2) physical host address is NOT supplied:
>>> +             *    - OK:   size is equal (same region)
>>> +             *    - Fail: size is not equal (same shm_id must identify only one
>>> +             *            region, there can't be two different regions with same
>>> +             *            shm_id)
>>> +             */
>>> +            bool start_match = paddr_assigned ? (paddr == mem->bank[i].start) :
>>> +                                                true;
>>> +
>>> +            if ( start_match && size == mem->bank[i].size )
>>>                break;
>>>            else
>>>            {
>>> -                printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
>>> +                printk("fdt: different shared memory region could not share the same shm ID %s\n",
>>>                       shm_id);
>>>                return -EINVAL;
>>>            }
>>>        }
>>> -        else if ( strncmp(shm_id, shmem_extra[i].shm_id,
>>> -                          MAX_SHM_ID_LENGTH) != 0 )
>>> -            continue;
>>>        else
>>>        {
>> There is no need for this else and entire block given that the block within if either calls break or return
> 
> There was a MISRA discussion about else at the end of if ... else if ... (R15.7) and I don’t remember
> the outcome

Sorry I was misreading the code here, sure I’ll remove the else.

Cheers,
Luca

[PATCH 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory

[Luca Fancellu]

The function allocate_bank_memory allocates pages from the heap and
map them to the guest using guest_physmap_add_page.

As a preparation work to support static shared memory bank when the
host physical address is not provided, Xen needs to allocate memory
from the heap, so rework allocate_bank_memory moving out the page
allocation in a new function called allocate_domheap_memory.

The function allocate_domheap_memory takes a callback function and
a pointer to some extra information passed to the callback and this
function will be called for every page allocated, until a defined
size is reached.

In order to keep allocate_bank_memory functionality, the callback
passed to allocate_domheap_memory is a wrapper for
guest_physmap_add_page.

Let allocate_domheap_memory be externally visible, in order to use
it in the future from the static shared memory module.

Take the opportunity to change the signature of allocate_bank_memory
and remove the 'struct domain' parameter, which can be retrieved from
'struct kernel_info'.

No functional changes is intended.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/dom0less-build.c           |  4 +-
 xen/arch/arm/domain_build.c             | 77 +++++++++++++++++--------
 xen/arch/arm/include/asm/domain_build.h |  9 ++-
 3 files changed, 62 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 74f053c242f4..20ddf6f8f250 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
 
     mem->nr_banks = 0;
     bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
-    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
+    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
                                bank_size) )
         goto fail;
 
     bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
-    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
+    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
                                bank_size) )
         goto fail;
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 0784e4c5e315..148db06b8ca3 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -417,26 +417,13 @@ static void __init allocate_memory_11(struct domain *d,
 }
 
 #ifdef CONFIG_DOM0LESS_BOOT
-bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
-                                 gfn_t sgfn, paddr_t tot_size)
+bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
+                                    alloc_domheap_mem_cb cb, void *extra)
 {
-    struct membanks *mem = kernel_info_get_mem(kinfo);
-    int res;
+    unsigned int max_order = UINT_MAX;
     struct page_info *pg;
-    struct membank *bank;
-    unsigned int max_order = ~0;
 
-    /*
-     * allocate_bank_memory can be called with a tot_size of zero for
-     * the second memory bank. It is not an error and we can safely
-     * avoid creating a zero-size memory bank.
-     */
-    if ( tot_size == 0 )
-        return true;
-
-    bank = &mem->bank[mem->nr_banks];
-    bank->start = gfn_to_gaddr(sgfn);
-    bank->size = tot_size;
+    BUG_ON(!cb);
 
     while ( tot_size > 0 )
     {
@@ -463,17 +450,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
             continue;
         }
 
-        res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
-        if ( res )
-        {
-            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+        if ( cb(d, pg, order, extra) )
             return false;
-        }
 
-        sgfn = gfn_add(sgfn, 1UL << order);
         tot_size -= (1ULL << (PAGE_SHIFT + order));
     }
 
+    return true;
+}
+
+static int __init guest_map_pages(struct domain *d, struct page_info *pg,
+                                  unsigned int order, void *extra)
+{
+    gfn_t *sgfn = (gfn_t *)extra;
+    int res;
+
+    BUG_ON(!sgfn);
+    res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order);
+    if ( res )
+    {
+        dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+        return res;
+    }
+
+    *sgfn = gfn_add(*sgfn, 1UL << order);
+
+    return 0;
+}
+
+bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
+                                 paddr_t tot_size)
+{
+    struct membanks *mem = kernel_info_get_mem(kinfo);
+    struct domain *d = kinfo->d;
+    struct membank *bank;
+
+    /*
+     * allocate_bank_memory can be called with a tot_size of zero for
+     * the second memory bank. It is not an error and we can safely
+     * avoid creating a zero-size memory bank.
+     */
+    if ( tot_size == 0 )
+        return true;
+
+    bank = &mem->bank[mem->nr_banks];
+    bank->start = gfn_to_gaddr(sgfn);
+    bank->size = tot_size;
+
+    /*
+     * Allocate pages from the heap until tot_size and map them to the guest
+     * using guest_map_pages, passing the starting gfn as extra parameter for
+     * the map operation.
+     */
+    if ( !allocate_domheap_memory(d, tot_size, guest_map_pages, &sgfn) )
+        return false;
+
     mem->nr_banks++;
     kinfo->unassigned_mem -= bank->size;
 
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index 45936212ca21..9eeb5839f6ed 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -5,9 +5,12 @@
 #include <asm/kernel.h>
 
 typedef __be32 gic_interrupt_t[3];
-
-bool allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
-                          gfn_t sgfn, paddr_t tot_size);
+typedef int (*alloc_domheap_mem_cb)(struct domain *d, struct page_info *pg,
+                                    unsigned int order, void *extra);
+bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
+                             alloc_domheap_mem_cb cb, void *extra);
+bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
+                          paddr_t tot_size);
 int construct_domain(struct domain *d, struct kernel_info *kinfo);
 int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
 int make_chosen_node(const struct kernel_info *kinfo);
-- 
2.34.1

Re: [PATCH 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> The function allocate_bank_memory allocates pages from the heap and
> map them to the guest using guest_physmap_add_page.
s/map/maps

> 
> As a preparation work to support static shared memory bank when the
> host physical address is not provided, Xen needs to allocate memory
> from the heap, so rework allocate_bank_memory moving out the page
> allocation in a new function called allocate_domheap_memory.
> 
> The function allocate_domheap_memory takes a callback function and
> a pointer to some extra information passed to the callback and this
> function will be called for every page allocated, until a defined
for every region given that you pass sgfn and order

> size is reached.
> 
> In order to keep allocate_bank_memory functionality, the callback
> passed to allocate_domheap_memory is a wrapper for
> guest_physmap_add_page.
> 
> Let allocate_domheap_memory be externally visible, in order to use
> it in the future from the static shared memory module.
> 
> Take the opportunity to change the signature of allocate_bank_memory
> and remove the 'struct domain' parameter, which can be retrieved from
> 'struct kernel_info'.
> 
> No functional changes is intended.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/dom0less-build.c           |  4 +-
>  xen/arch/arm/domain_build.c             | 77 +++++++++++++++++--------
>  xen/arch/arm/include/asm/domain_build.h |  9 ++-
>  3 files changed, 62 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 74f053c242f4..20ddf6f8f250 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> 
>      mem->nr_banks = 0;
>      bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> -    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> +    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
>                                 bank_size) )
>          goto fail;
> 
>      bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> -    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> +    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
>                                 bank_size) )
>          goto fail;
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 0784e4c5e315..148db06b8ca3 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -417,26 +417,13 @@ static void __init allocate_memory_11(struct domain *d,
>  }
> 
>  #ifdef CONFIG_DOM0LESS_BOOT
> -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
> -                                 gfn_t sgfn, paddr_t tot_size)
> +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> +                                    alloc_domheap_mem_cb cb, void *extra)
>  {
> -    struct membanks *mem = kernel_info_get_mem(kinfo);
> -    int res;
> +    unsigned int max_order = UINT_MAX;
>      struct page_info *pg;
You can limit the visibility of these variables by moving them into loop

> -    struct membank *bank;
> -    unsigned int max_order = ~0;
> 
> -    /*
> -     * allocate_bank_memory can be called with a tot_size of zero for
> -     * the second memory bank. It is not an error and we can safely
> -     * avoid creating a zero-size memory bank.
> -     */
> -    if ( tot_size == 0 )
> -        return true;
> -
> -    bank = &mem->bank[mem->nr_banks];
> -    bank->start = gfn_to_gaddr(sgfn);
> -    bank->size = tot_size;
> +    BUG_ON(!cb);
No need for that

> 
>      while ( tot_size > 0 )
>      {
> @@ -463,17 +450,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
>              continue;
>          }
> 
> -        res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
> -        if ( res )
> -        {
> -            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +        if ( cb(d, pg, order, extra) )
>              return false;
> -        }
> 
> -        sgfn = gfn_add(sgfn, 1UL << order);
>          tot_size -= (1ULL << (PAGE_SHIFT + order));
>      }
> 
> +    return true;
> +}
> +
> +static int __init guest_map_pages(struct domain *d, struct page_info *pg,
Does it make sense to return int if it is not taken into account by the user?

> +                                  unsigned int order, void *extra)
> +{
> +    gfn_t *sgfn = (gfn_t *)extra;
> +    int res;
> +
> +    BUG_ON(!sgfn);
> +    res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order);
> +    if ( res )
> +    {
> +        dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +        return res;
> +    }
> +
> +    *sgfn = gfn_add(*sgfn, 1UL << order);
> +
> +    return 0;
> +}
> +
> +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> +                                 paddr_t tot_size)
> +{
> +    struct membanks *mem = kernel_info_get_mem(kinfo);
> +    struct domain *d = kinfo->d;
> +    struct membank *bank;
> +
> +    /*
> +     * allocate_bank_memory can be called with a tot_size of zero for
> +     * the second memory bank. It is not an error and we can safely
> +     * avoid creating a zero-size memory bank.
> +     */
> +    if ( tot_size == 0 )
> +        return true;
> +
> +    bank = &mem->bank[mem->nr_banks];
> +    bank->start = gfn_to_gaddr(sgfn);
> +    bank->size = tot_size;
> +
> +    /*
> +     * Allocate pages from the heap until tot_size and map them to the guest
until tot_size is 0

> +     * using guest_map_pages, passing the starting gfn as extra parameter for
> +     * the map operation.
> +     */
> +    if ( !allocate_domheap_memory(d, tot_size, guest_map_pages, &sgfn) )
> +        return false;
> +
>      mem->nr_banks++;
>      kinfo->unassigned_mem -= bank->size;
> 
> diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
> index 45936212ca21..9eeb5839f6ed 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -5,9 +5,12 @@
>  #include <asm/kernel.h>
> 
>  typedef __be32 gic_interrupt_t[3];
> -
> -bool allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
> -                          gfn_t sgfn, paddr_t tot_size);
> +typedef int (*alloc_domheap_mem_cb)(struct domain *d, struct page_info *pg,
> +                                    unsigned int order, void *extra);
> +bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> +                             alloc_domheap_mem_cb cb, void *extra);
> +bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> +                          paddr_t tot_size);
>  int construct_domain(struct domain *d, struct kernel_info *kinfo);
>  int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
>  int make_chosen_node(const struct kernel_info *kinfo);
> --
> 2.34.1
> 

~Michal

[PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap

[Luca Fancellu]

This commit implements the logic to have the static shared memory banks
from the Xen heap instead of having the host physical address passed from
the user.

When the host physical address is not supplied, the physical memory is
taken from the Xen heap using allocate_domheap_memory, the allocation
needs to occur at the first handled DT node and the allocated banks
need to be saved somewhere, so introduce the 'shm_heap_banks' static
global variable of type 'struct meminfo' that will hold the banks
allocated from the heap, its field .shmem_extra will be used to point
to the bootinfo shared memory banks .shmem_extra space, so that there
is not further allocation of memory and every bank in shm_heap_banks
can be safely identified by the shm_id to reconstruct its traceability
and if it was allocated or not.

A search into 'shm_heap_banks' will reveal if the banks were allocated
or not, in case the host address is not passed, and the callback given
to allocate_domheap_memory will store the banks in the structure and
map them to the current domain, to do that, some changes to
acquire_shared_memory_bank are made to let it differentiate if the bank
is from the heap and if it is, then assign_pages is called for every
bank.

When the bank is already allocated, for every bank allocated with the
corresponding shm_id, handle_shared_mem_bank is called and the mapping
are done.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/static-shmem.c | 193 +++++++++++++++++++++++++++++-------
 1 file changed, 157 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 1c03bb7f1882..10396ed52ff1 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -9,6 +9,18 @@
 #include <asm/static-memory.h>
 #include <asm/static-shmem.h>
 
+typedef struct {
+    struct domain *d;
+    paddr_t gbase;
+    bool owner_dom_io;
+    const char *role_str;
+    struct shmem_membank_extra *bank_extra_info;
+} alloc_heap_pages_cb_extra;
+
+static struct meminfo __initdata shm_heap_banks = {
+    .common.max_banks = NR_MEM_BANKS
+};
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
@@ -66,7 +78,8 @@ static bool __init is_shm_allocated_to_domio(paddr_t pbase)
 }
 
 static mfn_t __init acquire_shared_memory_bank(struct domain *d,
-                                               paddr_t pbase, paddr_t psize)
+                                               paddr_t pbase, paddr_t psize,
+                                               bool bank_from_heap)
 {
     mfn_t smfn;
     unsigned long nr_pfns;
@@ -86,19 +99,31 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
     d->max_pages += nr_pfns;
 
     smfn = maddr_to_mfn(pbase);
-    res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
+    if ( bank_from_heap )
+        /*
+         * When host address is not provided, static shared memory is
+         * allocated from heap and shall be assigned to owner domain.
+         */
+        res = assign_pages(maddr_to_page(pbase), nr_pfns, d, 0);
+    else
+        res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
+
     if ( res )
     {
-        printk(XENLOG_ERR
-               "%pd: failed to acquire static memory: %d.\n", d, res);
-        d->max_pages -= nr_pfns;
-        return INVALID_MFN;
+        printk(XENLOG_ERR "%pd: failed to %s static memory: %d.\n", d,
+               bank_from_heap ? "assign" : "acquire", res);
+        goto fail;
     }
 
     return smfn;
+
+ fail:
+    d->max_pages -= nr_pfns;
+    return INVALID_MFN;
 }
 
 static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
+                                       bool bank_from_heap,
                                        const struct membank *shm_bank)
 {
     mfn_t smfn;
@@ -113,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
     psize = shm_bank->size;
     nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
 
-    printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
-           d, pbase, pbase + psize);
-
-    smfn = acquire_shared_memory_bank(d, pbase, psize);
+    smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
     if ( mfn_eq(smfn, INVALID_MFN) )
         return -EINVAL;
 
@@ -188,6 +210,7 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start,
 static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
                                          bool owner_dom_io,
                                          const char *role_str,
+                                         bool bank_from_heap,
                                          const struct membank *shm_bank)
 {
     paddr_t pbase, psize;
@@ -197,6 +220,10 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
 
     pbase = shm_bank->start;
     psize = shm_bank->size;
+
+    printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
+           d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
+
     /*
      * DOMID_IO is a fake domain and is not described in the Device-Tree.
      * Therefore when the owner of the shared region is DOMID_IO, we will
@@ -209,7 +236,8 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
          * We found the first borrower of the region, the owner was not
          * specified, so they should be assigned to dom_io.
          */
-        ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
+        ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
+                                   bank_from_heap, shm_bank);
         if ( ret )
             return ret;
     }
@@ -226,6 +254,40 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
     return 0;
 }
 
+static int __init save_map_heap_pages(struct domain *d, struct page_info *pg,
+                                      unsigned int order, void *extra)
+{
+    alloc_heap_pages_cb_extra *b_extra = (alloc_heap_pages_cb_extra *)extra;
+    int idx = shm_heap_banks.common.nr_banks;
+    int ret = -ENOSPC;
+
+    BUG_ON(!b_extra);
+
+    if ( idx < shm_heap_banks.common.max_banks )
+    {
+        shm_heap_banks.bank[idx].start = page_to_maddr(pg);
+        shm_heap_banks.bank[idx].size = (1ULL << (PAGE_SHIFT + order));
+        shm_heap_banks.bank[idx].shmem_extra = b_extra->bank_extra_info;
+        shm_heap_banks.common.nr_banks++;
+
+        ret = handle_shared_mem_bank(b_extra->d, b_extra->gbase,
+                                     b_extra->owner_dom_io, b_extra->role_str,
+                                     true, &shm_heap_banks.bank[idx]);
+        if ( !ret )
+        {
+            /* Increment guest physical address for next mapping */
+            b_extra->gbase += shm_heap_banks.bank[idx].size;
+            ret = 0;
+        }
+    }
+
+    if ( ret )
+        printk("Failed to allocate static shared memory from Xen heap: (%d)\n",
+               ret);
+
+    return ret;
+}
+
 int __init process_shm(struct domain *d, struct kernel_info *kinfo,
                        const struct dt_device_node *node)
 {
@@ -264,42 +326,101 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         pbase = boot_shm_bank->start;
         psize = boot_shm_bank->size;
 
-        if ( INVALID_PADDR == pbase )
-        {
-            printk("%pd: host physical address must be chosen by users at the moment.", d);
-            return -EINVAL;
-        }
+        /*
+         * "role" property is optional and if it is defined explicitly,
+         * then the owner domain is not the default "dom_io" domain.
+         */
+        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
+            owner_dom_io = false;
 
         /*
-         * xen,shared-mem = <pbase, gbase, size>;
-         * TODO: pbase is optional.
+         * xen,shared-mem = <[pbase,] gbase, size>;
+         * pbase is optional.
          */
         addr_cells = dt_n_addr_cells(shm_node);
         size_cells = dt_n_size_cells(shm_node);
         prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
         BUG_ON(!prop);
         cells = (const __be32 *)prop->value;
-        gbase = dt_read_paddr(cells + addr_cells, addr_cells);
 
-        for ( i = 0; i < PFN_DOWN(psize); i++ )
-            if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
-            {
-                printk("%pd: invalid physical address 0x%"PRI_mfn"\n",
-                       d, mfn_x(mfn_add(maddr_to_mfn(pbase), i)));
-                return -EINVAL;
-            }
+        if ( pbase != INVALID_PADDR )
+        {
+            /* guest phys address is after host phys address */
+            gbase = dt_read_paddr(cells + addr_cells, addr_cells);
+
+            for ( i = 0; i < PFN_DOWN(psize); i++ )
+                if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
+                {
+                    printk("%pd: invalid physical address 0x%"PRI_mfn"\n",
+                        d, mfn_x(mfn_add(maddr_to_mfn(pbase), i)));
+                    return -EINVAL;
+                }
+
+            /* The host physical address is supplied by the user */
+            ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str,
+                                         false, boot_shm_bank);
+            if ( ret )
+                return ret;
+        }
+        else
+        {
+            /*
+             * The host physical address is not supplied by the user, so it
+             * means that the banks needs to be allocated from the Xen heap,
+             * look into the already allocated banks from the heap.
+             */
+            const struct membank *alloc_bank = find_shm(&shm_heap_banks.common,
+                                                        shm_id);
 
-        /*
-         * "role" property is optional and if it is defined explicitly,
-         * then the owner domain is not the default "dom_io" domain.
-         */
-        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
-            owner_dom_io = false;
+            /* guest phys address is right at the beginning */
+            gbase = dt_read_paddr(cells, addr_cells);
 
-        ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str,
-                                     boot_shm_bank);
-        if ( ret )
-            return ret;
+            if ( !alloc_bank )
+            {
+                alloc_heap_pages_cb_extra cb_arg = { d, gbase, owner_dom_io,
+                    role_str, boot_shm_bank->shmem_extra };
+
+                /* shm_id identified bank is not yet allocated */
+                if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages,
+                                              &cb_arg) )
+                {
+                    printk(XENLOG_ERR
+                           "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n",
+                           psize >> 20);
+                    return -EINVAL;
+                }
+            }
+            else
+            {
+                /* shm_id identified bank is already allocated */
+                const struct membank *end_bank =
+                        &shm_heap_banks.bank[shm_heap_banks.common.nr_banks];
+                paddr_t gbase_bank = gbase;
+
+                /*
+                 * Static shared memory banks that are taken from the Xen heap
+                 * are allocated sequentially in shm_heap_banks, so starting
+                 * from the first bank found identified by shm_id, the code can
+                 * just advance by one bank at the time until it reaches the end
+                 * of the array or it finds another bank NOT identified by
+                 * shm_id
+                 */
+                for ( ; alloc_bank < end_bank; alloc_bank++ )
+                {
+                    if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
+                                 MAX_SHM_ID_LENGTH) != 0 )
+                        break;
+
+                    ret = handle_shared_mem_bank(d, gbase_bank, owner_dom_io,
+                                                 role_str, true, alloc_bank);
+                    if ( ret )
+                        return ret;
+
+                    /* Increment guest physical address for next mapping */
+                    gbase_bank += alloc_bank->size;
+                }
+            }
+        }
 
         /*
          * Record static shared memory region info for later setting
-- 
2.34.1

Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> This commit implements the logic to have the static shared memory banks
> from the Xen heap instead of having the host physical address passed from
> the user.
> 
> When the host physical address is not supplied, the physical memory is
> taken from the Xen heap using allocate_domheap_memory, the allocation
> needs to occur at the first handled DT node and the allocated banks
> need to be saved somewhere, so introduce the 'shm_heap_banks' static
> global variable of type 'struct meminfo' that will hold the banks
> allocated from the heap, its field .shmem_extra will be used to point
> to the bootinfo shared memory banks .shmem_extra space, so that there
> is not further allocation of memory and every bank in shm_heap_banks
> can be safely identified by the shm_id to reconstruct its traceability
> and if it was allocated or not.
> 
> A search into 'shm_heap_banks' will reveal if the banks were allocated
> or not, in case the host address is not passed, and the callback given
> to allocate_domheap_memory will store the banks in the structure and
> map them to the current domain, to do that, some changes to
> acquire_shared_memory_bank are made to let it differentiate if the bank
> is from the heap and if it is, then assign_pages is called for every
> bank.
> 
> When the bank is already allocated, for every bank allocated with the
> corresponding shm_id, handle_shared_mem_bank is called and the mapping
> are done.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

I tested this patch and it resulted in assertion:
Assertion 's <= e' failed at common/rangeset.c:189

I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided),
start + size would overflow. Did you test this patch?

~Michal

Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap

[Luca Fancellu]

> On 10 May 2024, at 10:17, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Luca,
> 
> On 23/04/2024 10:25, Luca Fancellu wrote:
>> 
>> 
>> This commit implements the logic to have the static shared memory banks
>> from the Xen heap instead of having the host physical address passed from
>> the user.
>> 
>> When the host physical address is not supplied, the physical memory is
>> taken from the Xen heap using allocate_domheap_memory, the allocation
>> needs to occur at the first handled DT node and the allocated banks
>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>> global variable of type 'struct meminfo' that will hold the banks
>> allocated from the heap, its field .shmem_extra will be used to point
>> to the bootinfo shared memory banks .shmem_extra space, so that there
>> is not further allocation of memory and every bank in shm_heap_banks
>> can be safely identified by the shm_id to reconstruct its traceability
>> and if it was allocated or not.
>> 
>> A search into 'shm_heap_banks' will reveal if the banks were allocated
>> or not, in case the host address is not passed, and the callback given
>> to allocate_domheap_memory will store the banks in the structure and
>> map them to the current domain, to do that, some changes to
>> acquire_shared_memory_bank are made to let it differentiate if the bank
>> is from the heap and if it is, then assign_pages is called for every
>> bank.
>> 
>> When the bank is already allocated, for every bank allocated with the
>> corresponding shm_id, handle_shared_mem_bank is called and the mapping
>> are done.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> I tested this patch and it resulted in assertion:
> Assertion 's <= e' failed at common/rangeset.c:189
> 
> I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided),
> start + size would overflow. Did you test this patch?

Hi Michal,

Mmm I’m testing with a dom0less setup, without dom0, I think I missed that part because my guests doesn’t have
the hypervisor node (enhanced), sorry about that, I’ll test using your setup, can you confirm what are you using?


> 
> ~Michal

Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap

[Michal Orzel]

Hi Luca,

On 10/05/2024 11:25, Luca Fancellu wrote:
> 
> 
>> On 10 May 2024, at 10:17, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Hi Luca,
>>
>> On 23/04/2024 10:25, Luca Fancellu wrote:
>>>
>>>
>>> This commit implements the logic to have the static shared memory banks
>>> from the Xen heap instead of having the host physical address passed from
>>> the user.
>>>
>>> When the host physical address is not supplied, the physical memory is
>>> taken from the Xen heap using allocate_domheap_memory, the allocation
>>> needs to occur at the first handled DT node and the allocated banks
>>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>>> global variable of type 'struct meminfo' that will hold the banks
>>> allocated from the heap, its field .shmem_extra will be used to point
>>> to the bootinfo shared memory banks .shmem_extra space, so that there
>>> is not further allocation of memory and every bank in shm_heap_banks
>>> can be safely identified by the shm_id to reconstruct its traceability
>>> and if it was allocated or not.
>>>
>>> A search into 'shm_heap_banks' will reveal if the banks were allocated
>>> or not, in case the host address is not passed, and the callback given
>>> to allocate_domheap_memory will store the banks in the structure and
>>> map them to the current domain, to do that, some changes to
>>> acquire_shared_memory_bank are made to let it differentiate if the bank
>>> is from the heap and if it is, then assign_pages is called for every
>>> bank.
>>>
>>> When the bank is already allocated, for every bank allocated with the
>>> corresponding shm_id, handle_shared_mem_bank is called and the mapping
>>> are done.
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>
>> I tested this patch and it resulted in assertion:
>> Assertion 's <= e' failed at common/rangeset.c:189
>>
>> I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided),
>> start + size would overflow. Did you test this patch?
> 
> Hi Michal,
> 
> Mmm I’m testing with a dom0less setup, without dom0, I think I missed that part because my guests doesn’t have
> the hypervisor node (enhanced), sorry about that, I’ll test using your setup, can you confirm what are you using?
I have these Qemu tests (with and without SMMU):
 - shmem between domU1 and domU2 with/without host address provided (owner domIO or domU1)
 - shmem between dom0 and domU1 with/without host address provided (owner domIO or dom0)

BTW. What was the conclusion about the issue if shmem region clashes with RAM allocated for 1:1 domU e.g. dom0.
Accidentally, I end up with a configuration where Xen allocated for dom0 a region of RAM clashing with my shmem region.

~Michal

Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap

[Luca Fancellu]

> On 10 May 2024, at 10:32, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Luca,
> 
> On 10/05/2024 11:25, Luca Fancellu wrote:
>> 
>> 
>>> On 10 May 2024, at 10:17, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> Hi Luca,
>>> 
>>> On 23/04/2024 10:25, Luca Fancellu wrote:
>>>> 
>>>> 
>>>> This commit implements the logic to have the static shared memory banks
>>>> from the Xen heap instead of having the host physical address passed from
>>>> the user.
>>>> 
>>>> When the host physical address is not supplied, the physical memory is
>>>> taken from the Xen heap using allocate_domheap_memory, the allocation
>>>> needs to occur at the first handled DT node and the allocated banks
>>>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>>>> global variable of type 'struct meminfo' that will hold the banks
>>>> allocated from the heap, its field .shmem_extra will be used to point
>>>> to the bootinfo shared memory banks .shmem_extra space, so that there
>>>> is not further allocation of memory and every bank in shm_heap_banks
>>>> can be safely identified by the shm_id to reconstruct its traceability
>>>> and if it was allocated or not.
>>>> 
>>>> A search into 'shm_heap_banks' will reveal if the banks were allocated
>>>> or not, in case the host address is not passed, and the callback given
>>>> to allocate_domheap_memory will store the banks in the structure and
>>>> map them to the current domain, to do that, some changes to
>>>> acquire_shared_memory_bank are made to let it differentiate if the bank
>>>> is from the heap and if it is, then assign_pages is called for every
>>>> bank.
>>>> 
>>>> When the bank is already allocated, for every bank allocated with the
>>>> corresponding shm_id, handle_shared_mem_bank is called and the mapping
>>>> are done.
>>>> 
>>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> 
>>> I tested this patch and it resulted in assertion:
>>> Assertion 's <= e' failed at common/rangeset.c:189
>>> 
>>> I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided),
>>> start + size would overflow. Did you test this patch?
>> 
>> Hi Michal,
>> 
>> Mmm I’m testing with a dom0less setup, without dom0, I think I missed that part because my guests doesn’t have
>> the hypervisor node (enhanced), sorry about that, I’ll test using your setup, can you confirm what are you using?
> I have these Qemu tests (with and without SMMU):
> - shmem between domU1 and domU2 with/without host address provided (owner domIO or domU1)

Ok, I tested this one, but without enhanced domUs

> - shmem between dom0 and domU1 with/without host address provided (owner domIO or dom0)

I’m missing this one, I’ll check everywhere where bootinfo_get_shmem() is used, sorry for the oversight 

> 
> BTW. What was the conclusion about the issue if shmem region clashes with RAM allocated for 1:1 domU e.g. dom0.
> Accidentally, I end up with a configuration where Xen allocated for dom0 a region of RAM clashing with my shmem region.

So the conclusion is that we should not have this configuration, but at the moment there is a tech debt because to enforce the
check we should do some work around the p2m as Julien suggested, but it’s not trivial because seems that some hyper calls
are currently relaying on overwriting the mappings.


> 
> ~Michal

[PATCH 7/7] xen/docs: Describe static shared memory when host address is not provided

[Luca Fancellu]

From: Penny Zheng <Penny.Zheng@arm.com>

This commit describe the new scenario where host address is not provided
in "xen,shared-mem" property and a new example is added to the page to
explain in details.

Take the occasion to fix some typos in the page.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v1:
 - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-10-Penny.Zheng@arm.com/
   with some changes in the commit message.
---
 docs/misc/arm/device-tree/booting.txt | 52 ++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index bbd955e9c2f6..ac4bad6fe5e0 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -590,7 +590,7 @@ communication.
     An array takes a physical address, which is the base address of the
     shared memory region in host physical address space, a size, and a guest
     physical address, as the target address of the mapping.
-    e.g. xen,shared-mem = < [host physical address] [guest address] [size] >
+    e.g. xen,shared-mem = < [host physical address] [guest address] [size] >;
 
     It shall also meet the following criteria:
     1) If the SHM ID matches with an existing region, the address range of the
@@ -601,8 +601,8 @@ communication.
     The number of cells for the host address (and size) is the same as the
     guest pseudo-physical address and they are inherited from the parent node.
 
-    Host physical address is optional, when missing Xen decides the location
-    (currently unimplemented).
+    Host physical address is optional, when missing Xen decides the location.
+    e.g. xen,shared-mem = < [guest address] [size] >;
 
 - role (Optional)
 
@@ -629,7 +629,7 @@ chosen {
         role = "owner";
         xen,shm-id = "my-shared-mem-0";
         xen,shared-mem = <0x10000000 0x10000000 0x10000000>;
-    }
+    };
 
     domU1 {
         compatible = "xen,domain";
@@ -640,25 +640,36 @@ chosen {
         vpl011;
 
         /*
-         * shared memory region identified as 0x0(xen,shm-id = <0x0>)
-         * is shared between Dom0 and DomU1.
+         * shared memory region "my-shared-mem-0" is shared
+         * between Dom0 and DomU1.
          */
         domU1-shared-mem@10000000 {
             compatible = "xen,domain-shared-memory-v1";
             role = "borrower";
             xen,shm-id = "my-shared-mem-0";
             xen,shared-mem = <0x10000000 0x50000000 0x10000000>;
-        }
+        };
 
         /*
-         * shared memory region identified as 0x1(xen,shm-id = <0x1>)
-         * is shared between DomU1 and DomU2.
+         * shared memory region "my-shared-mem-1" is shared between
+         * DomU1 and DomU2.
          */
         domU1-shared-mem@50000000 {
             compatible = "xen,domain-shared-memory-v1";
             xen,shm-id = "my-shared-mem-1";
             xen,shared-mem = <0x50000000 0x60000000 0x20000000>;
-        }
+        };
+
+        /*
+         * shared memory region "my-shared-mem-2" is shared between
+         * DomU1 and DomU2.
+         */
+        domU1-shared-mem-2 {
+            compatible = "xen,domain-shared-memory-v1";
+            xen,shm-id = "my-shared-mem-2";
+            role = "owner";
+            xen,shared-mem = <0x80000000 0x20000000>;
+        };
 
         ......
 
@@ -672,14 +683,21 @@ chosen {
         cpus = <1>;
 
         /*
-         * shared memory region identified as 0x1(xen,shm-id = <0x1>)
-         * is shared between domU1 and domU2.
+         * shared memory region "my-shared-mem-1" is shared between
+         * domU1 and domU2.
          */
         domU2-shared-mem@50000000 {
             compatible = "xen,domain-shared-memory-v1";
             xen,shm-id = "my-shared-mem-1";
             xen,shared-mem = <0x50000000 0x70000000 0x20000000>;
-        }
+        };
+
+        domU2-shared-mem-2 {
+            compatible = "xen,domain-shared-memory-v1";
+            xen,shm-id = "my-shared-mem-2";
+            role = "borrower";
+            xen,shared-mem = <0x90000000 0x20000000>;
+        };
 
         ......
     };
@@ -699,3 +717,11 @@ shared between DomU1 and DomU2. It will get mapped at 0x60000000 in DomU1 guest
 physical address space, and at 0x70000000 in DomU2 guest physical address space.
 DomU1 and DomU2 are both the borrower domain, the owner domain is the default
 owner domain DOMID_IO.
+
+For the static shared memory region "my-shared-mem-2", since host physical
+address is not provided by user, Xen will automatically allocate 512MB
+from heap as static shared memory to be shared between DomU1 and DomU2.
+The automatically allocated static shared memory will get mapped at
+0x80000000 in DomU1 guest physical address space, and at 0x90000000 in DomU2
+guest physical address space. DomU1 is explicitly defined as the owner domain,
+and DomU2 is the borrower domain.
-- 
2.34.1

Re: [PATCH 7/7] xen/docs: Describe static shared memory when host address is not provided

[Michal Orzel]

Hi Luca,

On 23/04/2024 10:25, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> This commit describe the new scenario where host address is not provided
> in "xen,shared-mem" property and a new example is added to the page to
> explain in details.
> 
> Take the occasion to fix some typos in the page.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal