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

Alexander Gordeev
2025-09-18T14:19:16Z

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

> + return rc;
> +}
...

Thanks!