RE: [PATCH v5 1/8] xen/arm: introduce static shared memory

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, July 16, 2022 2:10 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
> 
> Hi Penny,
> 
> On 29/06/2022 09:39, Penny Zheng wrote:
> >>> +    for ( i = 0; i < mem->nr_banks; i++ )
> >>> +    {
> >>> +        /*
> >>> +         * A static shared memory region could be shared between multiple
> >>> +         * domains.
> >>> +         */
> >>> +        if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> >>> +            break;
> >
> > Maybe I need to add a check on shm-id:
> > "
> >          /*
> >           * A static shared memory region could be shared between multiple
> >           * domains.
> >           */
> >          if ( strcmp(shm_id, mem->bank[i].shm_id) == 0 )
> >          {
> >              if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> >                  break;
> >              else
> >              {
> >                  printk("Warning: xen,shm-id %s does not match for all the 
> > nodes
> using the same region.\n",
> >                         shm_id);
> >                  return -EINVAL;
> >              }
> >          }
> > "
> > Wdyt？
> 
> AFAICT, this would allow to region to overlap if they have different shm ID. I
> am not entirely sure the rest of your code would work properly in this case
> (what if the owner is different).
> 
> So I think we need the following checks:
>    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
> 

Understood, true, the overlap shall also be checked.
"
@@ -451,6 +453,31 @@ static int __init process_shm_node(const void *fdt, int 
node,
                 return -EINVAL;
             }
         }
+        else
+        {
+            paddr_t end = paddr + size;
+            paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
+
+            if ( (paddr < mem->bank[i].start && end <= mem->bank[i].start) ||
+                 (paddr >= bank_end && end > bank_end) )
+            {
+                if ( strncmp(shm_id, mem->bank[i].shm_id,
+                             MAX_SHM_ID_LENGTH) != 0 )
+                    break;
+                else
+                {
+                    printk("fdt: different shared memory region could not 
share the same shm ID %s\n",
+                           shm_id);
+                    return -EINVAL;
+                }
+            }
+            else
+            {
+                printk("fdt: shared memory region overlap with an existing 
entry %#"PRIpaddr" - %#"PRIpaddr"\n",
+                        mem->bank[i].start, bank_end);
+                return -EINVAL;
+            }
+        }
     }

     if ( i == mem->nr_banks )
"

> Cheers,
> 
> --
> Julien Grall