Re: [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map

[Stefano Stabellini <sstabellini@kernel.org>]

On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
> 
> In a few scenarios where owner domain, is defined after borrower domain in
> device tree configuration, then statically shared pages haven't been properly
> allocated if borrower domain tries to do foreign memory map during
> domain construction.
> 
> In order to cover such scenario, we defer all borrower domains' foreign
> memory map after all domain construction finished, then only need to store
> shm-info during domain construction.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>  xen/arch/arm/domain.c             |  3 +++
>  xen/arch/arm/domain_build.c       | 34 ++++++++++++++++++++++++++++++-
>  xen/arch/arm/include/asm/domain.h | 25 +++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index f0bfd67fe5..73ffbfb918 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -47,6 +47,9 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>  
>  #ifdef CONFIG_STATIC_SHM
>  struct domain *__read_mostly dom_shared;
> +
> +shm_info_t shm_list[NR_MEM_BANKS];

Instead of adding shm_list, maybe we can we re-use mem->bank
(bootinfo.reserved_mem)?

It is already storing the physical address and size (added in patch #1
with process_shm_node). We should be able to find the other info from
the mfn: mfn_to_page, page_get_owner, mfn_to_gfn. At most, we need to
mark the memory bank as shared and we could do that with another field
in struct membank. 


> +DECLARE_BITMAP(shm_list_mask, NR_MEM_BANKS);

This is the third bitmask we introduce :-)

Can we narrow it down to a single bitmask? Maybe we don't need it at all
if we switch to using bootinfo.mem.bank.


>  #endif
>  
>  static void do_idle(void)
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7ee4d33e0b..4b19160674 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -771,7 +771,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>  
>  }
>  
> -static int __init allocate_shared_memory(struct domain *d,
> +static int __init allocate_shared_memory(struct domain *d, u32 shm_id,

No need for it to be u32?


>                                           u32 addr_cells, u32 size_cells,
>                                           paddr_t pbase, paddr_t psize,
>                                           paddr_t gbase)
> @@ -795,6 +795,18 @@ static int __init allocate_shared_memory(struct domain *d,
>          return ret;
>      }
>  
> +    /*
> +     * If owner domain is not default dom_shared, shm-info of owner domain
> +     * shall also be recorded for later deferred foreign memory map.
> +     */
> +    if ( d != dom_shared )
> +    {
> +        shm_list[shm_id].owner_dom = d->domain_id;
> +        shm_list[shm_id].owner_gbase = gbase;
> +        shm_list[shm_id].size = psize;
> +        set_bit(shm_id, shm_list_mask);
> +    }
>      return ret;
>  }
>  
> @@ -962,6 +974,26 @@ static int __init process_shm(struct domain *d,
>              if ( ret )
>                  return ret;
>          }
> +        else
> +        {
> +            if ( strcmp(role_str, "borrower") == 0 )
> +            {
> +                /*
> +                 * In a few scenarios where owner domain, is defined after
> +                 * borrower domain in device tree configuration, statically
> +                 * shared pages haven't been properly allocated if borrower
> +                 * domain here tries to do foreign memory map.
> +                 * In order to cover such scenario, we defer all borrower
> +                 * domains'foreign memory map after all domain construction
> +                 * finished, and only store shm-info here for later use.
> +                 */
> +                shm_list[shm_id].borrower_dom[shm_list[shm_id].nr_borrower] =
> +                                                                d->domain_id;
> +                shm_list[shm_id].borrower_gbase[shm_list[shm_id].nr_borrower] =
> +                                                                gbase;
> +                shm_list[shm_id].nr_borrower++;
> +            }
> +        }

Maybe we don't need to defer this at all. guest_physmap_add_shm does
only two things:

1) get a page ref using the owner domain
2) add page to borrower p2m


We can do 2) straight away even if the owner is not yet allocated.

For 1), we need to get the right amount of references when the owner is
allocated (which could be after the borrowers).

Keeping in mind that we have already parsed all the info in
xen/arch/arm/bootfdt.c:process_shm_node, I wonder if we can add a field
to mem->bank[mem->nr_banks] to keep a count of the number of borrowers.

Then when we allocate the page to the owner here, we just get as many
additional reference as the number of borrowers.

This would:
- add a field to bootinfo.reserved_mem
- remove the need for shm_list
- remove the need for shm_list_mask
- remove the need for the deferral

Just trying to make things simpler :-)


>          /*
>           * Record static shared memory region info for later setting
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index 6df37d2c46..1c0f2e22ca 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -10,6 +10,7 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  #include <asm/vpl011.h>
> +#include <asm/setup.h>
>  #include <public/hvm/params.h>
>  
>  struct hvm_domain
> @@ -33,6 +34,30 @@ enum domain_type {
>  
>  #ifdef CONFIG_STATIC_SHM
>  extern struct domain *dom_shared;
> +
> +/* Maximum number of borrower domains. */
> +#define NR_SHM_DOMAIN 32
> +/*
> + * shm_list is indexed by unique identifier "xen,shm-id", but it only stores
> + * a subset of static shared memory regions, of which owner domain is not the
> + * default dom_shared.
> + * shm_list_mask bitmask is to record the position of these static shared
> + * memory regions.
> + * Per bit represents a entry in shm_list, and setting it 1 means the
> + * static shared memory region here is owned by a specific domain, then bit 0
> + * means the static shared memory region here is either owned by the default
> + * dom_shared or no static shared memory region here at all.
> + */
> +typedef struct {
> +    domid_t owner_dom;
> +    paddr_t owner_gbase;
> +    paddr_t size;
> +    domid_t borrower_dom[NR_SHM_DOMAIN];
> +    paddr_t borrower_gbase[NR_SHM_DOMAIN];
> +    unsigned long nr_borrower;
> +} shm_info_t;
> +extern shm_info_t shm_list[NR_MEM_BANKS];
> +extern unsigned long shm_list_mask[BITS_TO_LONGS(NR_MEM_BANKS)];
>  #else
>  #define dom_shared NULL
>  #endif
> -- 
> 2.25.1
>