diff mbox series

[03/22] acpi: vmap pages in acpi_os_alloc_memory

Message ID 20221216114853.8227-4-julien@xen.org (mailing list archive)
State New
Headers show
Series Remove the directmap | expand

Commit Message

Julien Grall Dec. 16, 2022, 11:48 a.m. UTC
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>

----

    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(-)

Comments

Julien Grall Dec. 16, 2022, 12:07 p.m. UTC | #1
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,
Jan Beulich Dec. 20, 2022, 3:15 p.m. UTC | #2
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
Julien Grall Dec. 21, 2022, 10:23 a.m. UTC | #3
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,
Stefano Stabellini Jan. 23, 2023, 9:39 p.m. UTC | #4
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 mbox series

Patch

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)