[PATCH 0/2] s390x: pv: Fixes and improvements

[PATCH 0/2] s390x: pv: Fixes and improvements

[Janosch Frank]

Using the destroy call instead of the export on a VM shutdown, we can
clear out a protected guest much faster.

The 3f exception can in fact be triggered by userspace and therefore
should not panic the whole system, but send a SIGSEGV to the culprit
process.

Janosch Frank (2):
  s390x: uv: Add destroy page call
  s390x: Add 3f program exception handler

 arch/s390/include/asm/uv.h   |  7 +++++++
 arch/s390/kernel/pgm_check.S |  2 +-
 arch/s390/kernel/uv.c        | 21 +++++++++++++++++++++
 arch/s390/mm/fault.c         | 23 +++++++++++++++++++++++
 arch/s390/mm/gmap.c          |  2 +-
 5 files changed, 53 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/2] s390x: uv: Add destroy page call

[Janosch Frank]

We don't need to export pages if we destroy the VM configuration
afterwards anyway. Instead we can destroy the page which will zero it
and then make it accessible to the host.

Destroying is about twice as fast as the export.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/uv.h |  7 +++++++
 arch/s390/kernel/uv.c      | 21 +++++++++++++++++++++
 arch/s390/mm/gmap.c        |  2 +-
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index cff4b4c99b75..0325fc0469b7 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -33,6 +33,7 @@
 #define UVC_CMD_DESTROY_SEC_CPU		0x0121
 #define UVC_CMD_CONV_TO_SEC_STOR	0x0200
 #define UVC_CMD_CONV_FROM_SEC_STOR	0x0201
+#define UVC_CMD_DESTR_SEC_STOR		0x0202
 #define UVC_CMD_SET_SEC_CONF_PARAMS	0x0300
 #define UVC_CMD_UNPACK_IMG		0x0301
 #define UVC_CMD_VERIFY_IMG		0x0302
@@ -344,6 +345,7 @@ static inline int is_prot_virt_host(void)
 }
 
 int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb);
+int uv_destroy_page(unsigned long paddr);
 int uv_convert_from_secure(unsigned long paddr);
 int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr);
 
@@ -354,6 +356,11 @@ void adjust_to_uv_max(unsigned long *vmax);
 static inline void setup_uv(void) {}
 static inline void adjust_to_uv_max(unsigned long *vmax) {}
 
+static inline int uv_destroy_page(unsigned long paddr)
+{
+	return 0;
+}
+
 static inline int uv_convert_from_secure(unsigned long paddr)
 {
 	return 0;
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index c296e5c8dbf9..e07a5859f1ea 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -118,6 +118,27 @@ static int uv_pin_shared(unsigned long paddr)
 	return 0;
 }
 
+/*
+ * Requests the Ultravisor to destroy a guest page and make it
+ * accessible to the host. The destroy clears the page instead of
+ * exporting.
+ *
+ * @paddr: Absolute host address of page to be destroyed
+ */
+int uv_destroy_page(unsigned long paddr)
+{
+	struct uv_cb_cfs uvcb = {
+		.header.cmd = UVC_CMD_DESTR_SEC_STOR,
+		.header.len = sizeof(uvcb),
+		.paddr = paddr
+	};
+
+	if (uv_call(0, (u64)&uvcb))
+		return -EINVAL;
+	return 0;
+}
+
+
 /*
  * Requests the Ultravisor to encrypt a guest page and make it
  * accessible to the host for paging (export).
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 373542ca1113..cfb0017f33a7 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr,
 	pte_t pte = READ_ONCE(*ptep);
 
 	if (pte_present(pte))
-		WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK));
+		WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK));
 	return 0;
 }
 
-- 
2.25.1

[PATCH 2/2] s390x: Add 3f program exception handler

[Janosch Frank]

Program exception 3f (secure storage violation) can only be detected
when the CPU is running in SIE with a format 4 state description,
e.g. running a protected guest. Because of this and because user
space partly controls the guest memory mapping and can trigger this
exception, we want to send a SIGSEGV to the process running the guest
and not panic the kernel.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
CC: <stable@vger.kernel.org> # 5.7+
Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kernel/pgm_check.S |  2 +-
 arch/s390/mm/fault.c         | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S
index 2c27907a5ffc..9a92638360ee 100644
--- a/arch/s390/kernel/pgm_check.S
+++ b/arch/s390/kernel/pgm_check.S
@@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception)		/* 3b */
 PGM_CHECK_DEFAULT			/* 3c */
 PGM_CHECK(do_secure_storage_access)	/* 3d */
 PGM_CHECK(do_non_secure_storage_access)	/* 3e */
-PGM_CHECK_DEFAULT			/* 3f */
+PGM_CHECK(do_secure_storage_violation)	/* 3f */
 PGM_CHECK(monitor_event_exception)	/* 40 */
 PGM_CHECK_DEFAULT			/* 41 */
 PGM_CHECK_DEFAULT			/* 42 */
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 4c8c063bce5b..20abb7c5c540 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(do_non_secure_storage_access);
 
+void do_secure_storage_violation(struct pt_regs *regs)
+{
+	char buf[TASK_COMM_LEN];
+
+	/*
+	 * Either KVM messed up the secure guest mapping or the same
+	 * page is mapped into multiple secure guests.
+	 *
+	 * This exception is only triggered when a guest 2 is running
+	 * and can therefore never occur in kernel context.
+	 */
+	printk_ratelimited(KERN_WARNING
+			   "Secure storage violation in task: %s, pid %d\n",
+			   get_task_comm(buf, current), task_pid_nr(current));
+	send_sig(SIGSEGV, current, 0);
+}
+NOKPROBE_SYMBOL(do_secure_storage_violation);
+
 #else
 void do_secure_storage_access(struct pt_regs *regs)
 {
@@ -869,4 +887,9 @@ void do_non_secure_storage_access(struct pt_regs *regs)
 {
 	default_trap_handler(regs);
 }
+
+void do_secure_storage_violation(struct pt_regs *regs)
+{
+	default_trap_handler(regs);
+}
 #endif
-- 
2.25.1

Re: [PATCH 2/2] s390x: Add 3f program exception handler

[Christian Borntraeger]

On 03.09.20 15:14, Janosch Frank wrote:
> Program exception 3f (secure storage violation) can only be detected
> when the CPU is running in SIE with a format 4 state description,
> e.g. running a protected guest. Because of this and because user
> space partly controls the guest memory mapping and can trigger this
> exception, we want to send a SIGSEGV to the process running the guest
> and not panic the kernel.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> CC: <stable@vger.kernel.org> # 5.7+
> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

I guess we will pick this up via the s390 tree?

 
> ---
>  arch/s390/kernel/pgm_check.S |  2 +-
>  arch/s390/mm/fault.c         | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S
> index 2c27907a5ffc..9a92638360ee 100644
> --- a/arch/s390/kernel/pgm_check.S
> +++ b/arch/s390/kernel/pgm_check.S
> @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception)		/* 3b */
>  PGM_CHECK_DEFAULT			/* 3c */
>  PGM_CHECK(do_secure_storage_access)	/* 3d */
>  PGM_CHECK(do_non_secure_storage_access)	/* 3e */
> -PGM_CHECK_DEFAULT			/* 3f */
> +PGM_CHECK(do_secure_storage_violation)	/* 3f */
>  PGM_CHECK(monitor_event_exception)	/* 40 */
>  PGM_CHECK_DEFAULT			/* 41 */
>  PGM_CHECK_DEFAULT			/* 42 */
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 4c8c063bce5b..20abb7c5c540 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(do_non_secure_storage_access);
>  
> +void do_secure_storage_violation(struct pt_regs *regs)
> +{
> +	char buf[TASK_COMM_LEN];
> +
> +	/*
> +	 * Either KVM messed up the secure guest mapping or the same
> +	 * page is mapped into multiple secure guests.
> +	 *
> +	 * This exception is only triggered when a guest 2 is running
> +	 * and can therefore never occur in kernel context.
> +	 */
> +	printk_ratelimited(KERN_WARNING
> +			   "Secure storage violation in task: %s, pid %d\n",
> +			   get_task_comm(buf, current), task_pid_nr(current));
> +	send_sig(SIGSEGV, current, 0);
> +}
> +NOKPROBE_SYMBOL(do_secure_storage_violation);
> +
>  #else
>  void do_secure_storage_access(struct pt_regs *regs)
>  {
> @@ -869,4 +887,9 @@ void do_non_secure_storage_access(struct pt_regs *regs)
>  {
>  	default_trap_handler(regs);
>  }
> +
> +void do_secure_storage_violation(struct pt_regs *regs)
> +{
> +	default_trap_handler(regs);
> +}
>  #endif
> 

Re: [PATCH 2/2] s390x: Add 3f program exception handler

[Heiko Carstens]

On Thu, Sep 03, 2020 at 09:14:35AM -0400, Janosch Frank wrote:
> Program exception 3f (secure storage violation) can only be detected
> when the CPU is running in SIE with a format 4 state description,
> e.g. running a protected guest. Because of this and because user
> space partly controls the guest memory mapping and can trigger this
> exception, we want to send a SIGSEGV to the process running the guest
> and not panic the kernel.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> CC: <stable@vger.kernel.org> # 5.7+
> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kernel/pgm_check.S |  2 +-
>  arch/s390/mm/fault.c         | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S
> index 2c27907a5ffc..9a92638360ee 100644
> --- a/arch/s390/kernel/pgm_check.S
> +++ b/arch/s390/kernel/pgm_check.S
> @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception)		/* 3b */
>  PGM_CHECK_DEFAULT			/* 3c */
>  PGM_CHECK(do_secure_storage_access)	/* 3d */
>  PGM_CHECK(do_non_secure_storage_access)	/* 3e */
> -PGM_CHECK_DEFAULT			/* 3f */
> +PGM_CHECK(do_secure_storage_violation)	/* 3f */
>  PGM_CHECK(monitor_event_exception)	/* 40 */
>  PGM_CHECK_DEFAULT			/* 41 */
>  PGM_CHECK_DEFAULT			/* 42 */
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 4c8c063bce5b..20abb7c5c540 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(do_non_secure_storage_access);
>  
> +void do_secure_storage_violation(struct pt_regs *regs)
> +{
> +	char buf[TASK_COMM_LEN];
> +
> +	/*
> +	 * Either KVM messed up the secure guest mapping or the same
> +	 * page is mapped into multiple secure guests.
> +	 *
> +	 * This exception is only triggered when a guest 2 is running
> +	 * and can therefore never occur in kernel context.
> +	 */
> +	printk_ratelimited(KERN_WARNING
> +			   "Secure storage violation in task: %s, pid %d\n",
> +			   get_task_comm(buf, current), task_pid_nr(current));

Why get_task_comm() and task_pid_nr() instead of simply current->comm
and current->pid?
Also: is the dmesg message of any value?

> +	send_sig(SIGSEGV, current, 0);
> +}
> +NOKPROBE_SYMBOL(do_secure_storage_violation);

Why is this NOKPROBE? Can this deadlock?

Re: [PATCH 1/2] s390x: uv: Add destroy page call

[Heiko Carstens]

On Thu, Sep 03, 2020 at 09:14:34AM -0400, Janosch Frank wrote:
> +int uv_destroy_page(unsigned long paddr)
> +{
> +	struct uv_cb_cfs uvcb = {
> +		.header.cmd = UVC_CMD_DESTR_SEC_STOR,
> +		.header.len = sizeof(uvcb),
> +		.paddr = paddr
> +	};
> +
> +	if (uv_call(0, (u64)&uvcb))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +
>  /*
>   * Requests the Ultravisor to encrypt a guest page and make it
>   * accessible to the host for paging (export).
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 373542ca1113..cfb0017f33a7 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr,
>  	pte_t pte = READ_ONCE(*ptep);
>  
>  	if (pte_present(pte))
> -		WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK));
> +		WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK));

Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that
function return void?

Re: [PATCH 2/2] s390x: Add 3f program exception handler

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3127 bytes --]

On 9/4/20 12:35 PM, Heiko Carstens wrote:
> On Thu, Sep 03, 2020 at 09:14:35AM -0400, Janosch Frank wrote:
>> Program exception 3f (secure storage violation) can only be detected
>> when the CPU is running in SIE with a format 4 state description,
>> e.g. running a protected guest. Because of this and because user
>> space partly controls the guest memory mapping and can trigger this
>> exception, we want to send a SIGSEGV to the process running the guest
>> and not panic the kernel.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> CC: <stable@vger.kernel.org> # 5.7+
>> Fixes: 084ea4d611a3 ("s390/mm: add (non)secure page access exceptions handlers")
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>  arch/s390/kernel/pgm_check.S |  2 +-
>>  arch/s390/mm/fault.c         | 23 +++++++++++++++++++++++
>>  2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kernel/pgm_check.S b/arch/s390/kernel/pgm_check.S
>> index 2c27907a5ffc..9a92638360ee 100644
>> --- a/arch/s390/kernel/pgm_check.S
>> +++ b/arch/s390/kernel/pgm_check.S
>> @@ -80,7 +80,7 @@ PGM_CHECK(do_dat_exception)		/* 3b */
>>  PGM_CHECK_DEFAULT			/* 3c */
>>  PGM_CHECK(do_secure_storage_access)	/* 3d */
>>  PGM_CHECK(do_non_secure_storage_access)	/* 3e */
>> -PGM_CHECK_DEFAULT			/* 3f */
>> +PGM_CHECK(do_secure_storage_violation)	/* 3f */
>>  PGM_CHECK(monitor_event_exception)	/* 40 */
>>  PGM_CHECK_DEFAULT			/* 41 */
>>  PGM_CHECK_DEFAULT			/* 42 */
>> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
>> index 4c8c063bce5b..20abb7c5c540 100644
>> --- a/arch/s390/mm/fault.c
>> +++ b/arch/s390/mm/fault.c
>> @@ -859,6 +859,24 @@ void do_non_secure_storage_access(struct pt_regs *regs)
>>  }
>>  NOKPROBE_SYMBOL(do_non_secure_storage_access);
>>  
>> +void do_secure_storage_violation(struct pt_regs *regs)
>> +{
>> +	char buf[TASK_COMM_LEN];
>> +
>> +	/*
>> +	 * Either KVM messed up the secure guest mapping or the same
>> +	 * page is mapped into multiple secure guests.
>> +	 *
>> +	 * This exception is only triggered when a guest 2 is running
>> +	 * and can therefore never occur in kernel context.
>> +	 */
>> +	printk_ratelimited(KERN_WARNING
>> +			   "Secure storage violation in task: %s, pid %d\n",
>> +			   get_task_comm(buf, current), task_pid_nr(current));
> 
> Why get_task_comm() and task_pid_nr() instead of simply current->comm
> and current->pid?

Normally if there are functions to get data I assume those should be used.

> Also: is the dmesg message of any value?

Yes, it's import for administrators to know that an exception caused
this segfault and not some memory shenanigans.

As the exception only occurs if a guest runs in unsupported modes like
sharing the memory between two secure guests it's a good first
indication what went wrong.

> 
>> +	send_sig(SIGSEGV, current, 0);
>> +}
>> +NOKPROBE_SYMBOL(do_secure_storage_violation);>
> Why is this NOKPROBE? Can this deadlock?
Right, we don't need to export this at all, this was copied over from
the export function above.




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] s390x: uv: Add destroy page call

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1315 bytes --]

On 9/4/20 12:39 PM, Heiko Carstens wrote:
> On Thu, Sep 03, 2020 at 09:14:34AM -0400, Janosch Frank wrote:
>> +int uv_destroy_page(unsigned long paddr)
>> +{
>> +	struct uv_cb_cfs uvcb = {
>> +		.header.cmd = UVC_CMD_DESTR_SEC_STOR,
>> +		.header.len = sizeof(uvcb),
>> +		.paddr = paddr
>> +	};
>> +
>> +	if (uv_call(0, (u64)&uvcb))
>> +		return -EINVAL;
>> +	return 0;
>> +}
>> +
>> +

I need to remove one of those \n

>>  /*
>>   * Requests the Ultravisor to encrypt a guest page and make it
>>   * accessible to the host for paging (export).
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 373542ca1113..cfb0017f33a7 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr,
>>  	pte_t pte = READ_ONCE(*ptep);
>>  
>>  	if (pte_present(pte))
>> -		WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK));
>> +		WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK));
> 
> Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that
> function return void?
> 
If you prefer that, I'll change the patch.

I think we'd need a proper print of the return codes of the UVC anyway,
the warn isn't very helpful if you want to debug after the fact.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] s390x: uv: Add destroy page call

[Heiko Carstens]

On Fri, Sep 04, 2020 at 01:38:53PM +0200, Janosch Frank wrote:
> >>   * Requests the Ultravisor to encrypt a guest page and make it
> >>   * accessible to the host for paging (export).
> >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> >> index 373542ca1113..cfb0017f33a7 100644
> >> --- a/arch/s390/mm/gmap.c
> >> +++ b/arch/s390/mm/gmap.c
> >> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr,
> >>  	pte_t pte = READ_ONCE(*ptep);
> >>  
> >>  	if (pte_present(pte))
> >> -		WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK));
> >> +		WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK));
> > 
> > Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that
> > function return void?
> > 
> If you prefer that, I'll change the patch.

Seems to be better to me. Otherwise you start to sprinkle WARN_ONs all
over the code, _if_ there would be more callers.

> I think we'd need a proper print of the return codes of the UVC anyway,
> the warn isn't very helpful if you want to debug after the fact.

Maybe a new debug feature? Well, but that's something that hasn't do
anything with this code.

Re: [PATCH 2/2] s390x: Add 3f program exception handler

[Heiko Carstens]

On Fri, Sep 04, 2020 at 01:33:28PM +0200, Janosch Frank wrote:
> >> +	printk_ratelimited(KERN_WARNING
> >> +			   "Secure storage violation in task: %s, pid %d\n",
> >> +			   get_task_comm(buf, current), task_pid_nr(current));
> > 
> > Why get_task_comm() and task_pid_nr() instead of simply current->comm
> > and current->pid?
> 
> Normally if there are functions to get data I assume those should be used.

Could be used, however I don't see why you need that extra complexity
here for both of them.

> > Also: is the dmesg message of any value?
> Yes, it's import for administrators to know that an exception caused
> this segfault and not some memory shenanigans.
> 
> As the exception only occurs if a guest runs in unsupported modes like
> sharing the memory between two secure guests it's a good first
> indication what went wrong.

Yes, fine with me. Just not sure of how help this is when pid
namespaces come into play...

Re: [PATCH 1/2] s390x: uv: Add destroy page call

[Janosch Frank]

On 9/4/20 2:10 PM, Heiko Carstens wrote:
> On Fri, Sep 04, 2020 at 01:38:53PM +0200, Janosch Frank wrote:
>>>>   * Requests the Ultravisor to encrypt a guest page and make it
>>>>   * accessible to the host for paging (export).
>>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>>> index 373542ca1113..cfb0017f33a7 100644
>>>> --- a/arch/s390/mm/gmap.c
>>>> +++ b/arch/s390/mm/gmap.c
>>>> @@ -2679,7 +2679,7 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr,
>>>>  	pte_t pte = READ_ONCE(*ptep);
>>>>  
>>>>  	if (pte_present(pte))
>>>> -		WARN_ON_ONCE(uv_convert_from_secure(pte_val(pte) & PAGE_MASK));
>>>> +		WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK));
>>>
>>> Why not put the WARN_ON_ONCE() into uv_destroy_page() and make that
>>> function return void?
>>>
>> If you prefer that, I'll change the patch.
> 
> Seems to be better to me. Otherwise you start to sprinkle WARN_ONs all
> over the code, _if_ there would be more callers.

The other call sites currently don't care about the return codes which
is not optimal.

I'd prefer to leave it as is and put a debug item on the todo list which
takes care of providing more debug data on error.

> 
>> I think we'd need a proper print of the return codes of the UVC anyway,
>> the warn isn't very helpful if you want to debug after the fact.
> 
> Maybe a new debug feature? Well, but that's something that hasn't do
> anything with this code.