Re: [PATCH V3 (resend) 02/19] x86/pv: Domheap pages should be mapped while relocating initrd

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

On Mon, May 13, 2024 at 01:40:29PM +0000, Elias El Yandouzi wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> Xen shouldn't use domheap page as if they were xenheap pages. Map and
> unmap pages accordingly.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Wei Wang <wawei@xxxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Elias El Yandouzi <eliasely@xxxxxxxxxx>
> 
> ----
>     Changes in V3:
>         * Rename commit title
>         * Rework the for loop copying the pages
> 
>     Changes in V2:
>         * Get rid of mfn_to_virt
>         * Don't open code copy_domain_page()
> 
>     Changes since Hongyan's version:
>         * Add missing newline after the variable declaration
> 
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index d8043fa58a..807296c280 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -618,18 +618,24 @@ int __init dom0_construct_pv(struct domain *d,
>          if ( d->arch.physaddr_bitsize &&
>               ((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
>          {
> +            unsigned int nr_pages = 1UL << order;

Shouldn't you better initialize nr_pages to 'count' instead of 'order'
here?

Also note how 'order' at this point is not yet initialized to the
'count' based value, so I'm not sure from where that 'order' is coming
from.

In v2 you had:

+            unsigned long nr_pages;
+
             order = get_order_from_pages(count);
             page = alloc_domheap_pages(d, order, MEMF_no_scrub);
             if ( !page )
                 panic("Not enough RAM for domain 0 initrd\n");
+
+            nr_pages = 1UL << order;

nr_pages was derived from the 'order' value based on 'count'.  As said
above, I think you want to use just 'count' here, which is the rounded
up value of pages needed by initrd_len.

Thanks, Roger.