Re: [PATCH v1 3/6] KVM: s390: Refactor dat_set_slot()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Build failure: dat_delete_slot() is called with too many arguments in KVM_MR_MOVE.

Pre-existing issues:
- [High] Spurious failure of memslot deletion and VM crash due to concurrent hardware RC bit updates.
--

--- Patch [3]: [PATCH v1 3/6] KVM: s390: Refactor dat_set_slot() ---
commit 15c005eb250e6c4e079b0b85ff6c42d583b09782
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Refactor dat_set_slot()

This commit refactors dat_set_slot() and related functions to only take a
struct kvm_s390_mmu_cache. For dat_delete_slot(), mc is now NULL. This
serves as a prerequisite for moving gmap DAT table setup.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index f2ea013cb33e2..7e5dd5a1eb1ef 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c

[ ... ]

> @@ -873,10 +866,9 @@ static long _dat_slot_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_wal
>  static long _dat_slot_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
>  {
>  	union crste new_crste, crste = READ_ONCE(*crstep);
> -	struct slot_priv *p = walk->priv;
> +	struct kvm_s390_mmu_cache *mc = walk->priv;

[Severity: High]
This is a pre-existing issue, but does the atomic exchange in this function
need a retry loop?

If the hardware page table walker asynchronously updates the Reference or
Change (RC) bits on a valid large page CRSTE while KVM is deleting the slot,
it seems dat_crstep_xchg_atomic() can fail because the memory value no longer
matches crste:

arch/s390/kvm/dat.c:_dat_slot_crste() {
	...
	/* This table entry needs to be updated. */
	if (walk->start <= gfn && walk->end >= next) {
		if (!dat_crstep_xchg_atomic(crstep, crste, new_crste, gfn, walk->asce))
			return -EINVAL;
	...
}

Would this spurious -EINVAL propagate up to kvm_arch_prepare_memory_region()
and trigger KVM_BUG_ON(rc, kvm), causing the VM to panic?

> -	new_crste.val = p->token;
> -	new_crste.h.tt = crste.h.tt;
> +	new_crste = mc ? _CRSTE_EMPTY(crste.h.tt) : _CRSTE_HOLE(crste.h.tt);
>  
>  	/* Table entry already in the desired state. */
>  	if (crste.val == new_crste.val)

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 91b9758354574..0a08a3c7b023d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5860,7 +5860,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  
>  		switch (change) {
>  		case KVM_MR_DELETE:
> -			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
> +			rc = dat_delete_slot(kvm->arch.gmap->asce, old->base_gfn, old->npages);
>  			break;
>  		case KVM_MR_MOVE:
>  			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);

[Severity: Low]
Does this code fail to compile?

Since dat_delete_slot() was updated to only take 3 arguments, it looks like
the KVM_MR_MOVE case is still passing 4 arguments.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814144041.169025-1-imbrenda@linux.ibm.com?part=3