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 <jbeul...@suse.com>
> ---
> 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....@citrix.com>
I'm also fine if you want to remove the warning message at this time.
Thanks, Roger.
