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

Claudio Imbrenda
2025-09-18T14:46:16Z

On Thu, 18 Sep 2025 16:19:07 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> On Wed, Sep 10, 2025 at 08:07:31PM +0200, Claudio Imbrenda wrote:
> > +static inline int __kvm_s390_faultin_read_gpa(struct kvm *kvm, struct guest_fault *f, gpa_t gaddr,
> > + unsigned long *val)
> > +{
> > + phys_addr_t phys_addr;
> > + int rc;
> > +
> > + rc = __kvm_s390_faultin_gfn(kvm, f, gpa_to_gfn(gaddr), false);
> > + if (!rc) {
> > + phys_addr = PFN_PHYS(f->pfn) | offset_in_page(gaddr);
>
> pfn_to_phys() is more consistent with the phys_to_virt() below ;)
>
> > + *val = *(unsigned long *)phys_to_virt(phys_addr);
> > + }
> > + return rc;
> > +}
> > +
> > #endif /* __KVM_S390_GACCESS_H */
> ...
> > +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)
> > +{
> > + 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);
>
>
> Calling release_faultin_multiple() on the whole range, before all gfns invalidated?

nothing is getting invalidated here, only a check whether the gfns
(may) already been invalidated. in which case we give up and release
the whole range

kvm_release_faultin_page() will return and do nothing if the struct
page * is NULL

I guess the name of the function in common code is misleading, I will
add some comments to explain what's going on here

> Tolerate invalidation of entries that are (!guest_faults[i].valid)? But then why the
> continue above?
>
> This function is a mistery to me, but that is - I am sure - due to my KVM ignorance...
>
> > + 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++)
>
> Unless n_pages could ever be zero, the below reads better.
>
> > + rc = __kvm_s390_faultin_gfn(kvm, guest_faults + i, start + i, write_attempt);
>
> if (rc)
> break;

yeah I guess it's more readable that way

>
> > + return rc;
> > +}
> ...
>
> Thanks!