[PATCH v1 0/1] Remove the need to alloc memory in uv.c

[PATCH v1 0/1] Remove the need to alloc memory in uv.c

[Harald Freudenberger]

The pkey handler is calling the uv in some circumstances
where no memory allocation is acceptable. As of now only
the uv_get_secret_metadata() function allocates memory.
As this is exactly one page, lets introduce a pre-allocated
work page and protect the concurrent use with a mutex to
remove dynamic memory allocation and free. This page may be
also used with future extension to the uv code.

Harald Freudenberger (1):
  s390/uv: Prealloc and use one work page

 arch/s390/kernel/uv.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v1 1/1] s390/uv: Prealloc and use one work page

[Harald Freudenberger]

The pkey handler is calling the uv in some circumstances
where no memory allocation is acceptable. As of now only
the uv_get_secret_metadata() function allocates memory.
As this is exactly one page, lets introduce a pre-allocated
work page and protect the concurrent use with a mutex to
remove dynamic memory allocation and free. This page may be
also used with future extension to the uv code.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
---
 arch/s390/kernel/uv.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 9f05df2da2f7..71900ec68a23 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -37,6 +37,13 @@ EXPORT_SYMBOL(uv_info);
 int __bootdata_preserved(prot_virt_host);
 EXPORT_SYMBOL(prot_virt_host);
 
+/*
+ * Need one work page for ephemeral use by uv_get_secret_metadata()
+ * The concurrent use of this page is protected by a mutex.
+ */
+static u8 *work_page;
+static DEFINE_MUTEX(work_page_lock);
+
 static int __init uv_init(phys_addr_t stor_base, unsigned long stor_len)
 {
 	struct uv_cb_init uvcb = {
@@ -61,6 +68,12 @@ void __init setup_uv(void)
 	if (!is_prot_virt_host())
 		return;
 
+	work_page = (u8 *)__get_free_page(GFP_KERNEL);
+	if (!work_page) {
+		pr_warn("Failed to alloc a working memory page\n");
+		return;
+	}
+
 	uv_stor_base = memblock_alloc_try_nid(
 		uv_info.uv_base_stor_len, SZ_1M, SZ_2G,
 		MEMBLOCK_ALLOC_ACCESSIBLE, NUMA_NO_NODE);
@@ -80,6 +93,7 @@ void __init setup_uv(void)
 	return;
 fail:
 	pr_info("Disabling support for protected virtualization");
+	free_page((unsigned long)work_page);
 	prot_virt_host = 0;
 }
 
@@ -730,11 +744,11 @@ int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
 	struct uv_secret_list *buf;
 	int rc;
 
-	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
+	mutex_lock(&work_page_lock);
+	buf = (struct uv_secret_list *)work_page;
 	rc = find_secret(secret_id, buf, secret);
-	kfree(buf);
+	mutex_unlock(&work_page_lock);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(uv_get_secret_metadata);
-- 
2.43.0

Re: [PATCH v1 1/1] s390/uv: Prealloc and use one work page

[Heiko Carstens]

On Thu, Mar 27, 2025 at 04:38:24PM +0100, Harald Freudenberger wrote:
> The pkey handler is calling the uv in some circumstances
> where no memory allocation is acceptable. As of now only
> the uv_get_secret_metadata() function allocates memory.
> As this is exactly one page, lets introduce a pre-allocated
> work page and protect the concurrent use with a mutex to
> remove dynamic memory allocation and free. This page may be
> also used with future extension to the uv code.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> ---
>  arch/s390/kernel/uv.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)

[adding maintainers, according to get_maintainer.pl]

 
>  static int __init uv_init(phys_addr_t stor_base, unsigned long stor_len)
>  {
>  	struct uv_cb_init uvcb = {
> @@ -61,6 +68,12 @@ void __init setup_uv(void)
>  	if (!is_prot_virt_host())
>  		return;
>  
> +	work_page = (u8 *)__get_free_page(GFP_KERNEL);
> +	if (!work_page) {
> +		pr_warn("Failed to alloc a working memory page\n");
> +		return;
> +	}
> +
>  	uv_stor_base = memblock_alloc_try_nid(

Did you test this? I think this cannot work. When setup_uv() is called
the buddy allocator is not yet initialized.
Please use memblock_alloc_or_panic() instead.

> -	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> +	mutex_lock(&work_page_lock);
> +	buf = (struct uv_secret_list *)work_page;
>  	rc = find_secret(secret_id, buf, secret);
> -	kfree(buf);
> +	mutex_unlock(&work_page_lock);

The commit message does not explain why memory allocation is not
acceptable. Usually this translates to non-sleepable context. If that
is the case, then using a mutex would be wrong. This needs to be
clarified.

> +
>  	return rc;

Unrelated whitespace change.

It is up to the kvm folks to decide if the whole approach is ok.

Re: [PATCH v1 1/1] s390/uv: Prealloc and use one work page

[Harald Freudenberger]

On 2025-03-28 11:34, Heiko Carstens wrote:
> On Thu, Mar 27, 2025 at 04:38:24PM +0100, Harald Freudenberger wrote:
>> The pkey handler is calling the uv in some circumstances
>> where no memory allocation is acceptable. As of now only
>> the uv_get_secret_metadata() function allocates memory.
>> As this is exactly one page, lets introduce a pre-allocated
>> work page and protect the concurrent use with a mutex to
>> remove dynamic memory allocation and free. This page may be
>> also used with future extension to the uv code.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> ---
>>  arch/s390/kernel/uv.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> [adding maintainers, according to get_maintainer.pl]
> 
> 
>>  static int __init uv_init(phys_addr_t stor_base, unsigned long 
>> stor_len)
>>  {
>>  	struct uv_cb_init uvcb = {
>> @@ -61,6 +68,12 @@ void __init setup_uv(void)
>>  	if (!is_prot_virt_host())
>>  		return;
>> 
>> +	work_page = (u8 *)__get_free_page(GFP_KERNEL);
>> +	if (!work_page) {
>> +		pr_warn("Failed to alloc a working memory page\n");
>> +		return;
>> +	}
>> +
>>  	uv_stor_base = memblock_alloc_try_nid(
> 
> Did you test this? I think this cannot work. When setup_uv() is called
> the buddy allocator is not yet initialized.
> Please use memblock_alloc_or_panic() instead.
> 

I only compiled this and I wanted to test this today in my
SEL environment. The patch is a suggestion and should trigger
maybe some feedback.

>> -	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>> -	if (!buf)
>> -		return -ENOMEM;
>> +	mutex_lock(&work_page_lock);
>> +	buf = (struct uv_secret_list *)work_page;
>>  	rc = find_secret(secret_id, buf, secret);
>> -	kfree(buf);
>> +	mutex_unlock(&work_page_lock);
> 
> The commit message does not explain why memory allocation is not
> acceptable. Usually this translates to non-sleepable context. If that
> is the case, then using a mutex would be wrong. This needs to be
> clarified.
> 
>> +
>>  	return rc;
> 
> Unrelated whitespace change.
> 
> It is up to the kvm folks to decide if the whole approach is ok.

Re: [PATCH v1 1/1] s390/uv: Prealloc and use one work page

[Heiko Carstens]

On Fri, Mar 28, 2025 at 01:51:06PM +0100, Harald Freudenberger wrote:
> > > +	work_page = (u8 *)__get_free_page(GFP_KERNEL);
> > > +	if (!work_page) {
> > > +		pr_warn("Failed to alloc a working memory page\n");
> > > +		return;
> > > +	}
> > > +
> > >  	uv_stor_base = memblock_alloc_try_nid(
> > 
> > Did you test this? I think this cannot work. When setup_uv() is called
> > the buddy allocator is not yet initialized.
> > Please use memblock_alloc_or_panic() instead.
> > 
> 
> I only compiled this and I wanted to test this today in my
> SEL environment. The patch is a suggestion and should trigger
> maybe some feedback.

Well, you got some feedback :)

> > > -	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> > > -	if (!buf)
> > > -		return -ENOMEM;
> > > +	mutex_lock(&work_page_lock);
> > > +	buf = (struct uv_secret_list *)work_page;
> > >  	rc = find_secret(secret_id, buf, secret);
> > > -	kfree(buf);
> > > +	mutex_unlock(&work_page_lock);
> > 
> > The commit message does not explain why memory allocation is not
> > acceptable. Usually this translates to non-sleepable context. If that
> > is the case, then using a mutex would be wrong. This needs to be
> > clarified.

But I'm really wondering about this part. Please clarify.