Re: [PATCH 09/11] xen/arm: remove shm holes for extended regions

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

NIT: title s/for/from?

On 12/03/2024 14:03, Luca Fancellu wrote:
> 
> 
> From: Penny Zheng <Penny.Zheng@xxxxxxx>
> 
> Static shared memory acts as reserved memory in guest, so it shall be
> excluded from extended regions.
> 
> Extended regions are taken care of under three different scenarios:
> normal DomU, direct-map domain with iommu on, and direct-map domain
> with iommu off.
> 
> For normal DomU, we create a new function "remove_shm_holes_for_domU",
> to firstly transfer original outputs into the format of
> "struct rangeset", then use "remove_shm_from_rangeset" to remove static
> shm from them.
> 
> For direct-map domain with iommu on, after we get guest shm info from "kinfo",
> we use "remove_shm_from_rangeset" to remove static shm.
> 
> For direct-map domain with iommu off, as static shm has already been taken
> care of through reserved memory banks, we do nothing.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v1:
>  - Rework of 
> https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-Penny.Zheng@xxxxxxx/
> ---
>  xen/arch/arm/domain_build.c             | 16 ++++-
>  xen/arch/arm/include/asm/domain_build.h |  2 +
>  xen/arch/arm/include/asm/static-shmem.h | 18 ++++++
>  xen/arch/arm/static-shmem.c             | 86 +++++++++++++++++++++++++
>  4 files changed, 119 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9fad9e8b2c40..740c483ea2db 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -817,8 +817,8 @@ int __init make_memory_node(const struct domain *d,
>      return res;
>  }
> 
> -static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> -                                  void *data)
> +int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> +                           void *data)
>  {
>      struct membanks *ext_regions = data;
>      paddr_t start, size;
> @@ -969,6 +969,8 @@ static int __init handle_pci_range(const struct 
> dt_device_node *dev,
>   * - MMIO
>   * - Host RAM
>   * - PCI aperture
> + * - Static shared memory regions, which are described by special property
> + *   "xen,domain-shared-memory-v1"
I'm not sure if providing a compatible here makes sense. If at all, I would put 
"xen,shared-mem" which holds the addresses.

>   */
>  static int __init find_memory_holes(const struct kernel_info *kinfo,
>                                      struct membanks *ext_regions)
> @@ -985,6 +987,11 @@ static int __init find_memory_holes(const struct 
> kernel_info *kinfo,
>      if ( !mem_holes )
>          return -ENOMEM;
> 
> +    /* Remove static shared memory regions */
> +    res = remove_shm_from_rangeset(kinfo, mem_holes);
> +    if ( res )
> +        goto out;
How can you remove from a rangeset without first adding to it?
This should be moved after rangeset_add_range().
Also, usually (and this is the case in this function) we pass frames
to rangeset and not addresses (argument is of type ul). However...

> +
>      /* Start with maximum possible addressable physical memory range */
>      start = 0;
>      end = (1ULL << p2m_ipa_bits) - 1;
> @@ -1089,7 +1096,10 @@ static int __init find_domU_holes(const struct 
> kernel_info *kinfo,
>          res = 0;
>      }
> 
> -    return res;
> +    if ( res )
> +        return res;
> +
> +    return remove_shm_holes_for_domU(kinfo, ext_regions);
>  }
> 
>  int __init make_hypervisor_node(struct domain *d,
> diff --git a/xen/arch/arm/include/asm/domain_build.h 
> b/xen/arch/arm/include/asm/domain_build.h
> index a6f276cc4263..026d975da28e 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -51,6 +51,8 @@ static inline int prepare_acpi(struct domain *d, struct 
> kernel_info *kinfo)
>  int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
>  #endif
> 
> +int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
> +
>  #endif
> 
>  /*
> diff --git a/xen/arch/arm/include/asm/static-shmem.h 
> b/xen/arch/arm/include/asm/static-shmem.h
> index c6fac9906656..2f70aed53ac7 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -29,6 +29,12 @@ void early_print_info_shmem(void);
> 
>  void init_sharedmem_pages(void);
> 
> +int remove_shm_from_rangeset(const struct kernel_info *kinfo,
> +                             struct rangeset *rangeset);
> +
> +int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
> +                              struct membanks *ext_regions);
> +
>  #else /* !CONFIG_STATIC_SHM */
> 
>  static inline int make_resv_memory_node(const struct domain *d,
> @@ -61,6 +67,18 @@ static inline void early_print_info_shmem(void) {};
> 
>  static inline void init_sharedmem_pages(void) {};
> 
> +static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo,
> +                                           struct rangeset *rangeset)
> +{
> +    return 0;
> +}
> +
> +static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
> +                                            struct membanks *ext_regions)
> +{
> +    return 0;
> +}
> +
>  #endif /* CONFIG_STATIC_SHM */
> 
>  #endif /* __ASM_STATIC_SHMEM_H_ */
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 6143f52cb991..b3e2105dd3f2 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
> 
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/rangeset.h>
>  #include <xen/sched.h>
> 
>  #include <asm/domain_build.h>
> @@ -564,6 +565,91 @@ void __init init_sharedmem_pages(void)
>          init_staticmem_bank(&shmem->bank[bank]);
>  }
> 
> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
> +                                    struct rangeset *rangeset)
> +{
> +    const struct membanks *shm_mem = &kinfo->shm_mem.common;
> +    unsigned int i;
> +
> +    /* Remove static shared memory regions */
> +    for ( i = 0; i < shm_mem->nr_banks; i++ )
> +    {
> +        paddr_t start, end;
... here, these hold physical addresses and...

> +        int res;
> +
> +        start = shm_mem->bank[i].start;
> +        end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
> +        res = rangeset_remove_range(rangeset, start, end);
you will end up in a mix which won't work. Switch to PFN_DOWN()

> +        if ( res )
What's the point of res variable if it is not printed below?

> +        {
> +            printk(XENLOG_ERR "Failed to remove: 
> %#"PRIpaddr"->%#"PRIpaddr"\n",
> +                   start, end);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
> +                                     struct membanks *ext_regions)
> +{
> +    const struct membanks *shm_mem = &kinfo->shm_mem.common;
> +    struct rangeset *guest_holes;
> +    unsigned int i;
> +    paddr_t start;
> +    paddr_t end;
> +    int res;
> +
> +    /* No static shared memory region. */
> +    if ( shm_mem->nr_banks == 0 )
> +        return 0;
> +
> +    dt_dprintk("Remove static shared memory holes for extended regions of 
> DomU\n");
> +
> +    guest_holes = rangeset_new(NULL, NULL, 0);
> +    if ( !guest_holes )
> +        return -ENOMEM;
> +
> +    /* Copy extended regions sets into the rangeset */
> +    for ( i = 0; i < ext_regions->nr_banks; i++ )
> +    {
> +        start = ext_regions->bank[i].start;
> +        end = start + ext_regions->bank[i].size - 1;
> +
> +        res = rangeset_add_range(guest_holes, start, end);
Ditto, PFN_DOWN().

~Michal