| Message ID | 20221216114853.8227-4-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove the directmap | expand |
Hi, On 16/12/2022 11:48, Julien Grall wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > Also, introduce a wrapper around vmap that maps a contiguous range for > boot allocations. Unfortunately, the new helper cannot be a static inline > because the dependences are a mess. We would need to re-include > asm/page.h (was removed in aa4b9d1ee653 "include: don't use asm/page.h > from common headers") and it doesn't look to be enough anymore > because bits from asm/cpufeature.h is used in the definition of PAGE_NX. > > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > > ---- Sorry I sent this patch (and the others) with 4 dashes rather than 3. This is my way to workaround an issue with the patchqueue tools I am using (it would strip the text after the --- otherwise). I will try to remember to remove the extra dash in the next version. Cheers,
On 16.12.2022 12:48, Julien Grall wrote: > --- a/xen/common/vmap.c > +++ b/xen/common/vmap.c > @@ -244,6 +244,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr) > return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); > } > > +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages) I don't think the _pages suffix buys us much here. I also think parameter names would better be consistent with other functions here, in particular with vmap() (i.e. s/nr_pages/nr/). > --- a/xen/drivers/acpi/osl.c > +++ b/xen/drivers/acpi/osl.c > @@ -221,7 +221,11 @@ void *__init acpi_os_alloc_memory(size_t sz) > void *ptr; > > if (system_state == SYS_STATE_early_boot) > - return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1))); > + { > + mfn_t mfn = alloc_boot_pages(PFN_UP(sz), 1); > + > + return vmap_contig_pages(mfn, PFN_UP(sz)); > + } Multiple pages may be allocated here, yet ... > @@ -246,5 +250,10 @@ void __init acpi_os_free_memory(void *ptr) > if (is_xmalloc_memory(ptr)) > xfree(ptr); > else if (ptr && system_state == SYS_STATE_early_boot) > - init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE); > + { > + paddr_t addr = mfn_to_maddr(vmap_to_mfn(ptr)); > + > + vunmap(ptr); > + init_boot_pages(addr, addr + PAGE_SIZE); > + } ... (as before) only one page would be freed here. With the move to vmap() it ought to be possible to do better now. (If you want to defer this to a later patch, please at least mention the aspect in the description.) Jan
Hi Jan, On 20/12/2022 15:15, Jan Beulich wrote: > On 16.12.2022 12:48, Julien Grall wrote: >> --- a/xen/common/vmap.c >> +++ b/xen/common/vmap.c >> @@ -244,6 +244,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr) >> return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); >> } >> >> +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages) > > I don't think the _pages suffix buys us much here. I also think parameter > names would better be consistent with other functions here, in particular > with vmap() (i.e. s/nr_pages/nr/). I will do the renaming. > >> --- a/xen/drivers/acpi/osl.c >> +++ b/xen/drivers/acpi/osl.c >> @@ -221,7 +221,11 @@ void *__init acpi_os_alloc_memory(size_t sz) >> void *ptr; >> >> if (system_state == SYS_STATE_early_boot) >> - return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1))); >> + { >> + mfn_t mfn = alloc_boot_pages(PFN_UP(sz), 1); >> + >> + return vmap_contig_pages(mfn, PFN_UP(sz)); >> + } > > Multiple pages may be allocated here, yet ... > >> @@ -246,5 +250,10 @@ void __init acpi_os_free_memory(void *ptr) >> if (is_xmalloc_memory(ptr)) >> xfree(ptr); >> else if (ptr && system_state == SYS_STATE_early_boot) >> - init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE); >> + { >> + paddr_t addr = mfn_to_maddr(vmap_to_mfn(ptr)); >> + >> + vunmap(ptr); >> + init_boot_pages(addr, addr + PAGE_SIZE); >> + } > > ... (as before) only one page would be freed here. With the move to > vmap() it ought to be possible to do better now. (If you want to > defer this to a later patch, please at least mention the aspect in > the description.) Good point, I will have a look to solve it in this patch. Cheers,
On Fri, 16 Dec 2022, Julien Grall wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > Also, introduce a wrapper around vmap that maps a contiguous range for > boot allocations. Unfortunately, the new helper cannot be a static inline > because the dependences are a mess. We would need to re-include > asm/page.h (was removed in aa4b9d1ee653 "include: don't use asm/page.h > from common headers") and it doesn't look to be enough anymore > because bits from asm/cpufeature.h is used in the definition of PAGE_NX. > > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> I saw Jan's comments and I agree with them but I also wanted to track that I reviewed this patch and looks OK: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > ---- > > Changes since Hongyan's version: > * Rename vmap_boot_pages() to vmap_contig_pages() > * Move the new helper in vmap.c to avoid compilation issue > * Don't use __pa() to translate the virtual address > --- > xen/common/vmap.c | 5 +++++ > xen/drivers/acpi/osl.c | 13 +++++++++++-- > xen/include/xen/vmap.h | 2 ++ > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/xen/common/vmap.c b/xen/common/vmap.c > index 1340c7c6faf6..78f051a67682 100644 > --- a/xen/common/vmap.c > +++ b/xen/common/vmap.c > @@ -244,6 +244,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr) > return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); > } > > +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages) > +{ > + return __vmap(&mfn, nr_pages, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); > +} > + > void vunmap(const void *va) > { > unsigned long addr = (unsigned long)va; > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c > index 389505f78666..44a9719b0dcf 100644 > --- a/xen/drivers/acpi/osl.c > +++ b/xen/drivers/acpi/osl.c > @@ -221,7 +221,11 @@ void *__init acpi_os_alloc_memory(size_t sz) > void *ptr; > > if (system_state == SYS_STATE_early_boot) > - return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1))); > + { > + mfn_t mfn = alloc_boot_pages(PFN_UP(sz), 1); > + > + return vmap_contig_pages(mfn, PFN_UP(sz)); > + } > > ptr = xmalloc_bytes(sz); > ASSERT(!ptr || is_xmalloc_memory(ptr)); > @@ -246,5 +250,10 @@ void __init acpi_os_free_memory(void *ptr) > if (is_xmalloc_memory(ptr)) > xfree(ptr); > else if (ptr && system_state == SYS_STATE_early_boot) > - init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE); > + { > + paddr_t addr = mfn_to_maddr(vmap_to_mfn(ptr)); > + > + vunmap(ptr); > + init_boot_pages(addr, addr + PAGE_SIZE); > + } > } > diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h > index b0f7632e8985..3c06c7c3ba30 100644 > --- a/xen/include/xen/vmap.h > +++ b/xen/include/xen/vmap.h > @@ -23,6 +23,8 @@ void *vmalloc_xen(size_t size); > void *vzalloc(size_t size); > void vfree(void *va); > > +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages); > + > void __iomem *ioremap(paddr_t, size_t); > > static inline void iounmap(void __iomem *va) > -- > 2.38.1 >
diff --git a/xen/common/vmap.c b/xen/common/vmap.c index 1340c7c6faf6..78f051a67682 100644 --- a/xen/common/vmap.c +++ b/xen/common/vmap.c @@ -244,6 +244,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr) return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); } +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages) +{ + return __vmap(&mfn, nr_pages, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); +} + void vunmap(const void *va) { unsigned long addr = (unsigned long)va; diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index 389505f78666..44a9719b0dcf 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -221,7 +221,11 @@ void *__init acpi_os_alloc_memory(size_t sz) void *ptr; if (system_state == SYS_STATE_early_boot) - return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1))); + { + mfn_t mfn = alloc_boot_pages(PFN_UP(sz), 1); + + return vmap_contig_pages(mfn, PFN_UP(sz)); + } ptr = xmalloc_bytes(sz); ASSERT(!ptr || is_xmalloc_memory(ptr)); @@ -246,5 +250,10 @@ void __init acpi_os_free_memory(void *ptr) if (is_xmalloc_memory(ptr)) xfree(ptr); else if (ptr && system_state == SYS_STATE_early_boot) - init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE); + { + paddr_t addr = mfn_to_maddr(vmap_to_mfn(ptr)); + + vunmap(ptr); + init_boot_pages(addr, addr + PAGE_SIZE); + } } diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h index b0f7632e8985..3c06c7c3ba30 100644 --- a/xen/include/xen/vmap.h +++ b/xen/include/xen/vmap.h @@ -23,6 +23,8 @@ void *vmalloc_xen(size_t size); void *vzalloc(size_t size); void vfree(void *va); +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages); + void __iomem *ioremap(paddr_t, size_t); static inline void iounmap(void __iomem *va)