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

[Michal Orzel <michal.orzel@amd.com>]

Hi Luca,

On 22/05/2024 09:51, 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 not 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, 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.
> Same change is done inside consider_modules, find_unallocated_memory and
> dt_unreserved_regions functions, in order to skip banks that starts with
> INVALID_PADDR from any computation.
> The changes above holds because of this consideration.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> ---
> v3 changes:
>  - fix typo in commit msg, add R-by Michal
> v2 changes:
>  - fix comments, add parenthesis to some conditions, remove unneeded
>    variables, remove else branch, increment counter in the for loop,
>    skip INVALID_PADDR start banks from also consider_modules,
>    find_unallocated_memory and dt_unreserved_regions. (Michal)
> ---
>  xen/arch/arm/arm32/mmu/mm.c |  11 +++-
>  xen/arch/arm/domain_build.c |   5 ++
>  xen/arch/arm/setup.c        |  14 +++-
>  xen/arch/arm/static-shmem.c | 125 +++++++++++++++++++++++++-----------
>  4 files changed, 111 insertions(+), 44 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
> index be480c31ea05..30a7aa1e8e51 100644
> --- a/xen/arch/arm/arm32/mmu/mm.c
> +++ b/xen/arch/arm/arm32/mmu/mm.c
> @@ -101,8 +101,15 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>      nr += reserved_mem->nr_banks;
>      for ( ; i - nr < shmem->nr_banks; i++ )
>      {
> -        paddr_t r_s = shmem->bank[i - nr].start;
> -        paddr_t r_e = r_s + shmem->bank[i - nr].size;
> +        paddr_t r_s, r_e;
> +
> +        r_s = shmem->bank[i - nr].start;
> +
> +        /* Shared memory banks can contain INVALID_PADDR as start */
> +        if ( INVALID_PADDR == r_s )
> +            continue;
> +
> +        r_e = r_s + shmem->bank[i - nr].size;
> 
>          if ( s < r_e && r_s < e )
>          {
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 968c497efc78..02e741685102 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -927,6 +927,11 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>          for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
>          {
>              start = mem_banks[i]->bank[j].start;
> +
> +            /* Shared memory banks can contain INVALID_PADDR as start */
> +            if ( INVALID_PADDR == start )
> +                continue;
> +
>              end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
>              res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
>                                          PFN_DOWN(end - 1));
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index c4e5c19b11d6..0c2fdaceaf21 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -240,8 +240,15 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>      offset = reserved_mem->nr_banks;
>      for ( ; i - offset < shmem->nr_banks; i++ )
>      {
> -        paddr_t r_s = shmem->bank[i - offset].start;
> -        paddr_t r_e = r_s + shmem->bank[i - offset].size;
> +        paddr_t r_s, r_e;
> +
> +        r_s = shmem->bank[i - offset].start;
> +
> +        /* Shared memory banks can contain INVALID_PADDR as start */
> +        if ( INVALID_PADDR == r_s )
> +            continue;
> +
> +        r_e = r_s + shmem->bank[i - offset].size;
> 
>          if ( s < r_e && r_s < e )
>          {
> @@ -272,7 +279,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 c15a65130659..74c81904b8a4 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.
> @@ -377,7 +383,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;
> @@ -432,24 +439,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) )
> @@ -471,39 +491,64 @@ 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:
> -         * 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 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);
Shouldn't it be INVALID_PADDR != paddr to indicate that paddr was assigned? Otherwise, looking at the
code belowe you would allow a configuration where the shm_id matches but the phys addresses don't.

~Michal