[02/11] xen/arm: avoid repetitive checking in process_shm_node

Commit Message

Luca Fancellu March 12, 2024, 1:03 p.m. UTC

From: Penny Zheng <[e-mail]>

Putting overlap and overflow checking in the loop is causing repetitive
operation, so this commit extracts both checking outside the loop.

Signed-off-by: Penny Zheng <[e-mail]>
Signed-off-by: Luca Fancellu <[e-mail]>
---
v1:
 - Rework of https://patchwork.kernel.org/project/xen-devel/patch/[e-mail]/
 - use strncmp to match the branch above
 - drop Michal R-by given the change
---
 xen/arch/arm/static-shmem.c | 39 +++++++++++++++----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

Comments

Michal Orzel March 19, 2024, 10:57 a.m. UTC | #1

Hi Luca,

On 12/03/2024 14:03, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <[e-mail]>
> 
> Putting overlap and overflow checking in the loop is causing repetitive
> operation, so this commit extracts both checking outside the loop.
> 
> Signed-off-by: Penny Zheng <[e-mail]>
> Signed-off-by: Luca Fancellu <[e-mail]>
> ---
> v1:
>  - Rework of https://patchwork.kernel.org/project/xen-devel/patch/[e-mail]/
>  - use strncmp to match the branch above
I don't understand why we need strncmp if we already validated that shm_id is NULL terminated within MAX_SHM_ID_LENGTH
at the beginning of the function with strnlen.

Other than that:
Reviewed-by: Michal Orzel <[e-mail]>

~Michal

Patch

diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index cb268cd2edf1..40a0e860c79d 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -349,7 +349,7 @@  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;
+    paddr_t paddr, gaddr, size, end;
     struct meminfo *mem = &bootinfo.reserved_mem;
     unsigned int i;
     int len;
@@ -422,6 +422,13 @@  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++ )
     {
         /*
@@ -441,30 +448,13 @@  int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
                 return -EINVAL;
             }
         }
+        else if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) != 0 )
+            continue;
         else
         {
-            paddr_t end = paddr + size;
-            paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
-
-            if ( (end <= paddr) || (bank_end <= mem->bank[i].start) )
-            {
-                printk("fdt: static shared memory region %s overflow\n", shm_id);
-                return -EINVAL;
-            }
-
-            if ( check_reserved_regions_overlap(paddr, size) )
-                return -EINVAL;
-            else
-            {
-                if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
-                    continue;
-                else
-                {
-                    printk("fdt: different shared memory region could not share the same shm ID %s\n",
-                           shm_id);
-                    return -EINVAL;
-                }
-            }
+            printk("fdt: different shared memory region could not share the same shm ID %s\n",
+                   shm_id);
+            return -EINVAL;
         }
     }
 
@@ -472,6 +462,9 @@  int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
     {
         if ( i < NR_MEM_BANKS )
         {
+            if ( check_reserved_regions_overlap(paddr, size) )
+                return -EINVAL;
+
             /* Static shared memory shall be reserved from any other use. */
             safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
             mem->bank[mem->nr_banks].start = paddr;