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

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

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!