Re: [PATCH v2 05/20] KVM: s390: Add helper functions for fault handling

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Wed, 2025-09-10 at 20:07 +0200, Claudio Imbrenda wrote:
> Add some helper functions for handling multiple guest faults at the
> same time.
> 
> This will be needed for VSIE, where a nested guest access also needs to
> access all the page tables that map it.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/gaccess.h  | 14 ++++++++++
>  arch/s390/kvm/kvm-s390.c | 44 +++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.h | 56 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 114 insertions(+)
> 
[...]

> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index c44fe0c3a097..dabcf65f58ff 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -22,6 +22,15 @@
> 
[...]

> +static inline void release_faultin_multiple(struct kvm *kvm, struct guest_fault *guest_faults,
> +					    int n, bool ignore)
> +{
> +	int i;
> +
> +	for (i = 0; i < n; i++) {
> +		kvm_release_faultin_page(kvm, guest_faults[i].page, ignore,
> +					 guest_faults[i].write_attempt);
> +		guest_faults[i].page = NULL;
> +	}
> +}
> +
> +static inline bool __kvm_s390_multiple_faults_need_retry(struct kvm *kvm, unsigned long seq,
> +							 struct guest_fault *guest_faults, int n,
> +							 bool unsafe)

The name of the function does not at all suggest that it releases guest pages.
Can you remove that and use

if (__kvm_s390_fault_array_needs_retry(...))
	release_faultin_array(...);

in the caller?
(I haven't yet looked at those)
"needs_retry" isn't telling me much right now, either.
What is being retried and why?
Comments would not hurt :)

> +{
> +	int i;
> +
> +	for (i = 0; i < n; i++) {
> +		if (!guest_faults[i].valid)
> +			continue;
> +		if ((unsafe && mmu_invalidate_retry_gfn_unsafe(kvm, seq, guest_faults[i].gfn)) ||
> +		    (!unsafe && mmu_invalidate_retry_gfn(kvm, seq, guest_faults[i].gfn))) {
> +			release_faultin_multiple(kvm, guest_faults, n, true);
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +static inline int __kvm_s390_faultin_gfn_range(struct kvm *kvm, struct guest_fault *guest_faults,
> +					       gfn_t start, int n_pages, bool write_attempt)
> +{
> +	int i, rc = 0;
> +
> +	for (i = 0; !rc && i < n_pages; i++)
> +		rc = __kvm_s390_faultin_gfn(kvm, guest_faults + i, start + i, write_attempt);
> +	return rc;
> +}
> +
> +#define release_faultin_array(kvm, array, ignore) \
> +	release_faultin_multiple(kvm, array, ARRAY_SIZE(array), ignore)
> +
> +#define __kvm_s390_fault_array_needs_retry(kvm, seq, array, unsafe) \
> +	__kvm_s390_multiple_faults_need_retry(kvm, seq, array, ARRAY_SIZE(array), unsafe)
> +
>  /* implemented in diag.c */
>  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu);
>  

-- 
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294