diff mbox series

[v2,(resend),04/27] acpi: vmap pages in acpi_os_alloc_memory

Message ID 20240116192611.41112-5-eliasely@amazon.com (mailing list archive)
State New, archived
Headers show
Series Remove the directmap | expand

Commit Message

Elias El Yandouzi Jan. 16, 2024, 7:25 p.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 dependencies 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.

Lastly, with the move to vmap(), it is now easier to find the size
of the mapping. So pass the whole area to init_boot_pages() rather than
just the first page.

Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Elias El Yandouzi <eliasely@amazon.com>

----

    Changes in v2:
        * Rename vmap_contig_pages() to vmap_contig()
        * Rename nr_pages to nr to be consistent with vmap() parameters
        * Pass the whole region to init_boot_pages()

    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

Comments

Jan Beulich Jan. 25, 2024, 4:28 p.m. UTC | #1 
On 16.01.2024 20:25, Elias El Yandouzi 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 dependencies 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.
> 
> Lastly, with the move to vmap(), it is now easier to find the size
> of the mapping. So pass the whole area to init_boot_pages() rather than
> just the first page.
> 
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Elias El Yandouzi <eliasely@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
Alejandro Vallejo June 26, 2024, 1:54 p.m. UTC | #2 
I'm late to the party but there's something bothering me a little.

On Tue Jan 16, 2024 at 7:25 PM GMT, Elias El Yandouzi wrote:
> diff --git a/xen/common/vmap.c b/xen/common/vmap.c
> index 171271fae3..966a7e763f 100644
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -245,6 +245,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr)
>      return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
>  }
>  
> +void *vmap_contig(mfn_t mfn, unsigned int nr)
> +{
> +    return __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> +}
> +
>  unsigned int vmap_size(const void *va)
>  {
>      unsigned int pages = vm_size(va, VMAP_DEFAULT);

How is vmap_contig() different from regular vmap()?

vmap() calls map_pages_to_xen() `nr` times, while vmap_contig() calls it just
once. I'd expect both cases to work fine as they are. What am I missing? What
would make...

> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 389505f786..ab80d6b2a9 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(mfn, PFN_UP(sz));
... this statement not operate identically with regular vmap()? Or
probably more interestingly, what would preclude existing calls to vmap() not
operate under vmap_contig() instead?

I'm guessing it has to do with ARM having granules, but the looping logic seems
wonky in the non 4K case anyway seeing how the va jumps are based on PAGE_SIZE.

> +	}
>  
>  	ptr = xmalloc_bytes(sz);
>  	ASSERT(!ptr || is_xmalloc_memory(ptr));
> @@ -246,5 +250,11 @@ 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));
> +		unsigned int nr = vmap_size(ptr);
> +
> +		vunmap(ptr);
> +		init_boot_pages(addr, addr + nr * PAGE_SIZE);
> +	}
>  }
> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> index 24c85de490..0c16baa85f 100644
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -15,6 +15,7 @@ void vm_init_type(enum vmap_region type, void *start, void *end);
>  void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
>               unsigned int align, unsigned int flags, enum vmap_region type);
>  void *vmap(const mfn_t *mfn, unsigned int nr);
> +void *vmap_contig(mfn_t mfn, unsigned int nr);
>  void vunmap(const void *va);
>  
>  void *vmalloc(size_t size);
Jan Beulich June 26, 2024, 3:17 p.m. UTC | #3 
On 26.06.2024 15:54, Alejandro Vallejo wrote:
> I'm late to the party but there's something bothering me a little.
> 
> On Tue Jan 16, 2024 at 7:25 PM GMT, Elias El Yandouzi wrote:
>> diff --git a/xen/common/vmap.c b/xen/common/vmap.c
>> index 171271fae3..966a7e763f 100644
>> --- a/xen/common/vmap.c
>> +++ b/xen/common/vmap.c
>> @@ -245,6 +245,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr)
>>      return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
>>  }
>>  
>> +void *vmap_contig(mfn_t mfn, unsigned int nr)
>> +{
>> +    return __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
>> +}
>> +
>>  unsigned int vmap_size(const void *va)
>>  {
>>      unsigned int pages = vm_size(va, VMAP_DEFAULT);
> 
> How is vmap_contig() different from regular vmap()?
> 
> vmap() calls map_pages_to_xen() `nr` times, while vmap_contig() calls it just
> once. I'd expect both cases to work fine as they are. What am I missing? What
> would make...
> 
>> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
>> index 389505f786..ab80d6b2a9 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(mfn, PFN_UP(sz));
> ... this statement not operate identically with regular vmap()? Or
> probably more interestingly, what would preclude existing calls to vmap() not
> operate under vmap_contig() instead?

Note how vmap()'s first parameter is "const mfn_t *mfn". This needs to point
to an array of "nr" MFNs. In order to use plain vmap() here, you'd first need
to set up a suitably large array, populate if with increasing MFN values, and
then make the call. Possible, but more complicated.

Jan
Alejandro Vallejo June 26, 2024, 4:33 p.m. UTC | #4 
On Wed Jun 26, 2024 at 4:17 PM BST, Jan Beulich wrote:
> On 26.06.2024 15:54, Alejandro Vallejo wrote:
> > I'm late to the party but there's something bothering me a little.
> > 
> > On Tue Jan 16, 2024 at 7:25 PM GMT, Elias El Yandouzi wrote:
> >> diff --git a/xen/common/vmap.c b/xen/common/vmap.c
> >> index 171271fae3..966a7e763f 100644
> >> --- a/xen/common/vmap.c
> >> +++ b/xen/common/vmap.c
> >> @@ -245,6 +245,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr)
> >>      return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> >>  }
> >>  
> >> +void *vmap_contig(mfn_t mfn, unsigned int nr)
> >> +{
> >> +    return __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> >> +}
> >> +
> >>  unsigned int vmap_size(const void *va)
> >>  {
> >>      unsigned int pages = vm_size(va, VMAP_DEFAULT);
> > 
> > How is vmap_contig() different from regular vmap()?
> > 
> > vmap() calls map_pages_to_xen() `nr` times, while vmap_contig() calls it just
> > once. I'd expect both cases to work fine as they are. What am I missing? What
> > would make...
> > 
> >> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> >> index 389505f786..ab80d6b2a9 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(mfn, PFN_UP(sz));
> > ... this statement not operate identically with regular vmap()? Or
> > probably more interestingly, what would preclude existing calls to vmap() not
> > operate under vmap_contig() instead?
>
> Note how vmap()'s first parameter is "const mfn_t *mfn". This needs to point
> to an array of "nr" MFNs. In order to use plain vmap() here, you'd first need
> to set up a suitably large array, populate if with increasing MFN values, and
> then make the call. Possible, but more complicated.
>
> Jan

I knew I must've been missing something. That pesky pointer... No wonder the
loop looked wonky. It was doing something completely different from what I
expected it to.

That clarifies it. Thanks a bunch, Jan.

Cheers,
Alejandro
diff mbox series

Patch

diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 171271fae3..966a7e763f 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -245,6 +245,11 @@  void *vmap(const mfn_t *mfn, unsigned int nr)
     return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
 }
 
+void *vmap_contig(mfn_t mfn, unsigned int nr)
+{
+    return __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+}
+
 unsigned int vmap_size(const void *va)
 {
     unsigned int pages = vm_size(va, VMAP_DEFAULT);
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 389505f786..ab80d6b2a9 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(mfn, PFN_UP(sz));
+	}
 
 	ptr = xmalloc_bytes(sz);
 	ASSERT(!ptr || is_xmalloc_memory(ptr));
@@ -246,5 +250,11 @@  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));
+		unsigned int nr = vmap_size(ptr);
+
+		vunmap(ptr);
+		init_boot_pages(addr, addr + nr * PAGE_SIZE);
+	}
 }
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index 24c85de490..0c16baa85f 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -15,6 +15,7 @@  void vm_init_type(enum vmap_region type, void *start, void *end);
 void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
              unsigned int align, unsigned int flags, enum vmap_region type);
 void *vmap(const mfn_t *mfn, unsigned int nr);
+void *vmap_contig(mfn_t mfn, unsigned int nr);
 void vunmap(const void *va);
 
 void *vmalloc(size_t size);