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

Claudio Imbrenda
2025-09-15T11:49:57Z

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