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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 12 Sep 2025 19:56:02 +0200
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> 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?

yes and no, it would make the caller harder to read, because other
things need to happen too, and it's easier to have a wrapper that takes
care of cleanup.

I can surely give the function a better name, and add some comments; if
you insist, I can also split it as you mentioned above -- if you think
it's more readable after splitting, then so be it

> (I haven't yet looked at those)
> "needs_retry" isn't telling me much right now, either.
> What is being retried and why?

that's the name of the already existing common code function (actually,
the existing function has an even more confusing name)

the whole fault-in needs to be retried; something happened while we
were not holding locks, we don't know what, and the physical address we
want to fault-in might now be incorrect. hence we need to retry

> Comments would not hurt :)

will add some

> 
> > +{
> > +	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);
> >    
>