Re: [PATCH v5 30/31] KVM: s390: arm64: Implement basic page fault handler

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The check for `PGM_PROTECTION` in `kvm_s390_cur_gmap_fault_is_write()` does not apply `PGM_INT_CODE_MASK` to `hai.pic`.
- [Low] The local variable `esr` is assigned but never used.
--

--- Patch [30]: [PATCH v5 30/31] KVM: s390: arm64: Implement basic page fault handler ---
commit 4547e2330db1908b3d5e4eeca0c723949d349711
Author: Steffen Eiden <[email protected]>

    KVM: s390: arm64: Implement basic page fault handler
    
    Add host functionality to page in guest memory. If the guest does
    something unexpected or illegal exit to userspace abort guest execution.

> diff --git a/arch/s390/kvm/arm64/mmu.c b/arch/s390/kvm/arm64/mmu.c
> new file mode 100644
> index 0000000000000..71cd582f24ff8
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/mmu.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/kvm_host.h>
> +
> +#include <arm64/kvm_emulate.h>
> +
> +#include <faultin.h>
> +
> +static inline bool kvm_s390_cur_gmap_fault_is_write(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.sae_block.hai.pic == PGM_PROTECTION ||
> +	       vcpu->arch.sae_block.hai.teid.fsi == TEID_FSI_STORE;
> +}

[Severity: High]
Does this code handle concurrent event flags correctly? 

If a protection fault occurs with concurrent event flags set (e.g., the PER bit
0x80) in the Program Interruption Code, hai.pic will contain a value like
0x84. 

Since this compares hai.pic directly to PGM_PROTECTION (0x04) without
applying PGM_INT_CODE_MASK, won't this result in a false evaluation?

If it fails to recognize it as a write fault, user_mem_abort() would call
kvm_s390_faultin_gfn() with f.write_attempt = 0, causing the host to map the
page read-only instead of making it writable. When the guest resumes, it could
immediately trap again with the same protection fault, leading to an infinite
page fault loop.

[ ... ]

> +int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_memory_slot *memslot;
> +	bool translation = false;
> +	phys_addr_t fault_ipa;
> +	unsigned long esr;
> +	unsigned long hva;
> +	bool write_fault;
> +	bool guest_size_err;
> +	bool writable;
> +	bool is_iabt;
> +	int ret;
> +	gfn_t gfn;
> +	int idx;
> +
> +	esr = kvm_vcpu_get_esr(vcpu);

[Severity: Low]
This isn't a bug, but is esr used anywhere else in this function?

It appears this local variable is assigned but never used, which could lead to a
-Wunused-but-set-variable compiler warning.

> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=30