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

Nina Schoetterl-Glausch
2025-09-12T17:56:10Z

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