diff mbox series

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

Message ID 20240423082532.776623-5-luca.fancellu@arm.com (mailing list archive)
State Superseded
Headers show
Series Static shared memory followup v2 - pt2 | expand

Commit Message

Luca Fancellu April 23, 2024, 8:25 a.m. UTC
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(-)

Comments

Michal Orzel May 8, 2024, 12:09 p.m. UTC | #1
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
Luca Fancellu May 8, 2024, 1:28 p.m. UTC | #2
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
Luca Fancellu May 9, 2024, 8:58 a.m. UTC | #3
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
diff mbox series

Patch

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,