Re: [PATCH v4 1/6] x86/PVH: improve Dom0 memory size calculation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 29, 2021 at 03:13:24PM +0200, Jan Beulich wrote:
> Assuming that the accounting for IOMMU page tables will also take care
> of the P2M needs was wrong: dom0_paging_pages() can determine a far
> higher value, high enough for the system to run out of memory while
> setting up Dom0. Hence in the case of shared page tables the larger of
> the two values needs to be used (without shared page tables the sum of
> both continues to be applicable).
> 
> To not further complicate the logic, eliminate the up-to-2-iteration
> loop in favor of doing a few calculations twice (before and after
> calling dom0_paging_pages()). While this will lead to slightly too high
> a value in "cpu_pages", it is deemed better to account a few too many
> than a few too little.
> 
> Also uniformly use paging_mode_enabled(), not is_hvm_domain().
> 
> While there also account for two further aspects in the PV case: With
> "iommu=dom0-passthrough" no IOMMU page tables would get allocated, so
> none need accounting for. And if shadow mode is to be enabled, setting
> aside a suitable amount for the P2M pool to get populated is also
> necessary (i.e. similar to the non-shared-page-tables case of PVH).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I wonder whether this isn't enough to drop the "PVH dom0 without
> dom0_mem" warning.
> 
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -318,8 +318,7 @@ unsigned long __init dom0_compute_nr_pag
>      struct domain *d, struct elf_dom_parms *parms, unsigned long initrd_len)
>  {
>      nodeid_t node;
> -    unsigned long avail = 0, nr_pages, min_pages, max_pages;
> -    bool need_paging;
> +    unsigned long avail = 0, nr_pages, min_pages, max_pages, iommu_pages = 0;
>  
>      /* The ordering of operands is to work around a clang5 issue. */
>      if ( CONFIG_DOM0_MEM[0] && !dom0_mem_set )
> @@ -337,53 +336,65 @@ unsigned long __init dom0_compute_nr_pag
>          avail -= d->max_vcpus - 1;
>  
>      /* Reserve memory for iommu_dom0_init() (rough estimate). */
> -    if ( is_iommu_enabled(d) )
> +    if ( is_iommu_enabled(d) && !iommu_hwdom_passthrough )
>      {
>          unsigned int s;
>  
>          for ( s = 9; s < BITS_PER_LONG; s += 9 )
> -            avail -= max_pdx >> s;
> +            iommu_pages += max_pdx >> s;
> +
> +        avail -= iommu_pages;
> +    }
> +
> +    nr_pages = get_memsize(&dom0_size, avail);
> +
> +    /*
> +     * If allocation isn't specified, reserve 1/16th of available memory for
> +     * things like DMA buffers. This reservation is clamped to a maximum of
> +     * 128MB.
> +     */
> +    if ( !nr_pages )
> +    {
> +        nr_pages = avail - (pv_shim ? pv_shim_mem(avail)
> +                            : min(avail / 16, 128UL << (20 - PAGE_SHIFT)));
> +        if ( paging_mode_enabled(d) )
> +            /*
> +             * Temporary workaround message until internal (paging) memory
> +             * accounting required to build a pvh dom0 is improved.
> +             */
> +            printk("WARNING: PVH dom0 without dom0_mem set is still 
> unstable. "
> +                   "If you get crashes during boot, try adding a dom0_mem 
> parameter\n");
>      }
>  
> -    need_paging = is_hvm_domain(d) &&
> -        (!iommu_use_hap_pt(d) || !paging_mode_hap(d));
> -    for ( ; ; need_paging = false )
> +    if ( paging_mode_enabled(d) || opt_dom0_shadow )

Do we also need to account for opt_pv_l1tf_hwdom in case dom0 gets
shadowing enabled during runtime?

The rest LGTM, so:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I'm also fine if you want to remove the warning message at this time.

Thanks, Roger.