[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Luca Fancellu <luca.fancellu@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Michal Orzel <michal.orzel@xxxxxxx>
  • Date: Wed, 8 May 2024 14:09:16 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=arm.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=JQ97iZJtt9Thg0nMoKUUPhZ0X3mcr4cpwmkH/PCWogk=; b=GEn+gd+SeLZdO3m87bo8XUlFM+1/C3Mqt4eo3T3DB9/XSTO5O4X05mHriQDyAb7MDqfL4VsvN1cjrHKi1M7GQzmeho+HxkS9vBC32K3gw5gpRlWIMNdLeL75L/cgMIpVWEal0E7qjiRryr1WZZYKitpRAZNzxsHYqLB+pKVWgD/1KU8NRu7hWeF1XxB7NfM7hLv0Sa4al39zSY1lVP4TTm2Y/K+vdR9h529U27pV/Z4YgbmRExR12JQTOcpUnHb9NpebTCChQ3J4KJkOH8kIsB9v3Xm8Dr+SsNQ7+MwuUGw7V1n14SEEzmOnmzYoC6+QC6m7idYN1QULuXVTYLfxgw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BrsNPWbMss67TDAn1mJfgMgD34hAhSZGfWAmHTakd6xxFlLyTggAVzPjJ+l+INXgYM6ut8ZbDBY5R+m1thon8ShZZQhQ2BOnFS9ofRrhxhTad7eb3+Af0xYYa/qXi6knyr+ByqySSA1ZDlDIL9Wmc7qAVrYYl9uWC47oFHKu1wGg7lVZARx8dDudmZ3GFfvlij2jVoE6dtD7b7mGNtE616/gglWAaiidyR+/3eDBBGp6NWiLN+S+olMbf7Bv36MsXh/0gnAYs0dz9qj/x03DHX91NXFOdyeZ1Do5FF/GLZOtXDYn/yKEVn+vR1hGF9E9u5vlgZzlVGFZ+Z9fnqyI6A==
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 08 May 2024 12:09:51 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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@xxxxxxx>
> ---
>  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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.