On 16.12.2022 12:48, Julien Grall wrote:
> From: Wei Liu <wei.l...@citrix.com>
>
> Xen shouldn't use domheap page as if they were xenheap pages. Map and
> unmap pages accordingly.
>
> Signed-off-by: Wei Liu <wei.l...@citrix.com>
> Signed-off-by: Wei Wang <wa...@amazon.de>
> Signed-off-by: Julien Grall <jgr...@amazon.com>
>
> ----
>
> Changes since Hongyan's version:
> * Add missing newline after the variable declaration
> ---
> xen/arch/x86/pv/dom0_build.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index a62f0fa2ef29..c837b2d96f89 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -611,18 +611,32 @@ 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;
> + unsigned long len = initrd_len;
> +
> 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;
I don't think this needs establishing here and ...
> for ( count = -count; order--; )
> if ( count & (1UL << order) )
> {
> free_domheap_pages(page, order);
> page += 1UL << order;
> + nr_pages -= 1UL << order;
... updating here. Doing so just once ...
> }
> - memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
> - initrd_len);
> +
... here ought to suffice, assuming this 2nd variable is needed at all
(alongside "len").
> + for ( i = 0; i < nr_pages; i++, len -= PAGE_SIZE )
> + {
> + void *p = __map_domain_page(page + i);
> +
> + memcpy(p, mfn_to_virt(initrd_mfn + i),
> + min(len, (unsigned long)PAGE_SIZE));
> + unmap_domain_page(p);
> + }
You're half open-coding copy_domain_page() without saying anywhere why
the remaining mfn_to_virt() is okay to keep. If you used
copy_domain_page(), no such remark would need adding in the description.
Jan
