Re: [PATCH v2 (resend) 08/27] x86/pv: Domheap pages should be mapped while relocating initrd

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2024 20:25, 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.

The title could to with mentioning Dom0. Since it's already a little long,
the mentioning of domheap pages could be left to the description. E.g.
"map pages while relocating Dom0 initrd"?

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -615,18 +615,25 @@ int __init dom0_construct_pv(struct domain *d,
>          if ( d->arch.physaddr_bitsize &&
>               ((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
>          {
> +            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;

Nit: Is there anything wrong with making this the initializer of the
variable?

>              for ( count = -count; order--; )
>                  if ( count & (1UL << order) )
>                  {
>                      free_domheap_pages(page, order);
>                      page += 1UL << order;
> +                    nr_pages -= 1UL << order;
>                  }
> -            memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
> -                   initrd_len);
> +
> +            for ( i = 0; i < nr_pages; i++ )
> +                copy_domain_page(page_to_mfn(page + i), _mfn(initrd_mfn + 
> i));

There's a type discrepancy between "i" and "nr_pages". If we truly expect
initrd-s of more than 16Tb size, "nr_pages" indeed needs to be unsigned
long, but then "i" can't remain as int. I for one think that having
"nr_pages" be unsigned int is more than enough. It might then still be
good to switch "i" from plain int to unsigned int, albeit maybe in a
(tiny) separate patch.

I further wonder whether it wouldn't be more consistent with the "else"
branch of the containing "if()" if instead of "initrd_mfn" "mfn" would
be used here (and incremented as we go). At which point I think the use
of "i" could be avoided here altogether:

            for ( ; nr_pages--; page++, mfn++ )
                copy_domain_page(page_to_mfn(page), _mfn(mfn));

or something substantially similar (e.g. re-written as while() loop).

Jan