| Message ID | 20240116192611.41112-9-eliasely@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove the directmap | expand |
On 16.01.2024 20:25, Elias El Yandouzi wrote: > From: Wei Liu <wei.liu2@citrix.com> > > 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
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index 5bbed3a36a..5659814e0c 100644 --- 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; 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)); + mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT; init_domheap_pages(mpt_alloc, mpt_alloc + PAGE_ALIGN(initrd_len));