Re: [PATCH v4 05/11] xen/arm: introduce direct-map for domUs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 20 Dec 2021, Penny Zheng wrote:
> Cases where domU needs direct-map memory map:
>   * IOMMU not present in the system.
>   * IOMMU disabled if it doesn't cover a specific device and all the guests
> are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
> a few without, then guest DMA security still could not be totally guaranteed.
> So users may want to disable the IOMMU, to at least gain some performance
> improvement from IOMMU disabled.
>   * IOMMU disabled as a workaround when it doesn't have enough bandwidth.
> To be specific, in a few extreme situation, when multiple devices do DMA
> concurrently, these requests may exceed IOMMU's transmission capacity.
>   * IOMMU disabled when it adds too much latency on DMA. For example,
> TLB may be missing in some IOMMU hardware, which may bring latency in DMA
> progress, so users may want to disable it in some realtime scenario.
>   * Guest OS relies on the host memory layout
> 
> This commit introduces a new helper assign_static_memory_11 to allocate
> static memory as guest RAM for direct-map domain.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>

The patch looks good. There are a couple of minor code style issus below
that it would be good to fix, at least the function parameters
alignment.  With that:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> v2 changes:
> - split the common codes into two helpers: parse_static_mem_prop and
> acquire_static_memory_bank to deduce complexity.
> - introduce a new helper allocate_static_memory_11 for allocating static
> memory for direct-map guests
> - remove redundant use "bool direct_map", to be replaced by
> d_cfg.flags & XEN_DOMCTL_CDF_directmap
> - remove panic action since it is fine to assign a non-DMA capable device when
> IOMMU and direct-map both off
> ---
> v3 changes:
> - doc refinement
> - drop the pointless gbank
> - add check of the size of nr_banks shall not exceed NR_MEM_BANKS
> - add ASSERT_UNREACHABLE to catch any misuse
> - add another check of validating flag XEN_DOMCTL_CDF_INTERNAL_directmap only
> when CONFIG_STATIC_MEMORY is set.
> ---
> v4 changes:
> - comment refinement
> - rename function allocate_static_memory_11() to assign_static_memory_11()
> to make clear there is actually no allocation done. Instead we are only
> mapping pre-defined host regions to pre-defined guest regions.
> - remove tot_size to directly substract psize from kinfo->unassigned_mem
> - check kinfo->unassigned_mem doesn't underflow or overflow
> - remove nested if/else
> - refine "panic" info
> ---
>  xen/arch/arm/domain_build.c | 97 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 94 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9206ec908d..d74a3eb908 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -494,8 +494,17 @@ static bool __init append_static_memory_to_bank(struct 
> domain *d,
>  {
>      int res;
>      unsigned int nr_pages = PFN_DOWN(size);
> -    /* Infer next GFN. */
> -    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> +    gfn_t sgfn;
> +
> +    /*
> +     * For direct-mapped domain, the GFN match the MFN.
> +     * Otherwise, this is inferred on what has already been allocated
> +     * in the bank.
> +     */
> +    if ( !is_domain_direct_mapped(d) )
> +        sgfn = gaddr_to_gfn(bank->start + bank->size);
> +    else
> +        sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
>  
>      res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
>      if ( res )
> @@ -668,12 +677,92 @@ static void __init allocate_static_memory(struct domain 
> *d,
>   fail:
>      panic("Failed to allocate requested static memory for domain %pd.", d);
>  }
> +
> +/*
> + * Allocate static memory as RAM for one specific domain d.
> + * The static memory will be directly mapped in the guest(Guest Physical
> + * Address == Physical Address).
> + */
> +static void __init assign_static_memory_11(struct domain *d,
> +                                             struct kernel_info *kinfo,
> +                                             const struct dt_device_node 
> *node)

Please align the parameters


> +{
> +    u32 addr_cells, size_cells, reg_cells;
> +    unsigned int nr_banks, bank = 0;
> +    const __be32 *cell;
> +    paddr_t pbase, psize;
> +    mfn_t smfn;
> +    int length;
> +
> +    if ( parse_static_mem_prop(node, &addr_cells, &size_cells, &length, 
> &cell) )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: failed to parse \"xen,static-mem\" property.\n", d);
> +        goto fail;
> +    }
> +    reg_cells = addr_cells + size_cells;
> +    nr_banks = length / (reg_cells * sizeof (u32));

no space after sizeof


> +
> +    if ( nr_banks > NR_MEM_BANKS )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: exceed max number of supported guest memory banks.\n", 
> d);
> +        goto fail;
> +    }
> +
> +    for ( ; bank < nr_banks; bank++ )
> +    {
> +        smfn = acquire_static_memory_bank(d, &cell, addr_cells, size_cells,
> +                                          &pbase, &psize);
> +        if ( mfn_eq(smfn, INVALID_MFN) )
> +            goto fail;
> +
> +        printk(XENLOG_INFO "%pd: STATIC BANK[%u] 
> %#"PRIpaddr"-%#"PRIpaddr"\n",
> +               d, bank, pbase, pbase + psize);
> +
> +        /* One guest memory bank is matched with one physical memory bank. */
> +        kinfo->mem.bank[bank].start = pbase;
> +        if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[bank],
> +                                           smfn, psize) )
> +            goto fail;
> +
> +        kinfo->unassigned_mem -= psize;
> +    }
> +
> +    kinfo->mem.nr_banks = nr_banks;
> +
> +    /*
> +     * The property 'memory' should match the amount of memory given to
> +     * the guest.
> +     * Currently, it is only possible to either acquire static memory or
> +     * let Xen allocate. *Mixing* is not supported.
> +     */
> +    if ( kinfo->unassigned_mem != 0 )
> +    {
> +        printk(XENLOG_ERR
> +               "Size of \"memory\" property doesn't match up with the sum-up 
> of \"xen,static-mem\". Unsupported configuration.\n");

This line would benefit from being broken down, but I am also OK if we
leave it as is


> +        goto fail;
> +    }
> +
> +    return;
> +
> + fail:
> +    panic("Failed to assign requested static memory for direct-map domain 
> %pd.",
> +          d);
> +}
>  #else
>  static void __init allocate_static_memory(struct domain *d,
>                                            struct kernel_info *kinfo,
>                                            const struct dt_device_node *node)
>  {
>  }
> +
> +static void __init assign_static_memory_11(struct domain *d,
> +                                             struct kernel_info *kinfo,
> +                                             const struct dt_device_node 
> *node)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>  #endif
>  
>  /*
> @@ -3023,8 +3112,10 @@ static int __init construct_domU(struct domain *d,
>  #endif
>      if ( !dt_find_property(node, "xen,static-mem", NULL) )
>          allocate_memory(d, &kinfo);
> -    else
> +    else if ( !is_domain_direct_mapped(d) )
>          allocate_static_memory(d, &kinfo, node);
> +    else
> +        assign_static_memory_11(d, &kinfo, node);
>  
>      rc = prepare_dtb_domU(d, &kinfo);
>      if ( rc < 0 )
> -- 
> 2.25.1
>