Re: [PATCH v3 3/4] KVM: s390: Improve unmapping of large ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] KVM: s390: wrong IDTE option flag IPTE_GUEST_ASCE passed to idte_crste()
- [Critical] KVM: s390: missing table-type bits in idte_crste() for PUD invalidation
- [Medium] KVM: s390: _gmap_unmap_pmd_fast() fails to clear pgste prefix_notif bit
--

commit e343f26ffd40139c7f207f60f7208912b2666ca8
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Improve unmapping of large ranges

This commit speeds up gmap_unmap_gfn_range() for guest address ranges larger
than one segment (1M) on standard VMs by invalidating higher-level DAT
entries with idte_crste() and updating detached page tables in bulk rather
than invalidating each leaf PTE individually.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index 225fdae3d57ca..7ae0b4a1a6ca0 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c

[ ... ]

> @@ -462,6 +453,117 @@ bool gmap_unmap_gfn_range(struct gmap *gmap, struct kvm_memory_slot *slot, gfn_t
>  	return false;
>  }
>  
> +static long _gmap_unmap_pud_fast(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *w)
> +{
> +	union crste old = *crstep;
> +
> +	/* We only care about large puds */
> +	if (!old.h.fc)
> +		return 0;
> +	if (!gfn)
> +		gmap_unmap_prefix(w->priv, 0, next);
> +	if (old.s.fc1.vsie_notif)
> +		gmap_handle_vsie_unshadow_event(w->priv, gfn);
> +
> +	if (machine_has_tlb_guest())
> +		idte_crste(crstep, gfn, IPTE_GUEST_ASCE, w->asce, 0);
> +	else
> +		idte_crste(crstep, gfn, 0, w->asce, 0);
> +	*crstep = _CRSTE_EMPTY(old.h.tt);

[Severity: Critical]
Does idte_crste() set the required table-type bits in the R1 operand when
invalidating a Region-3 entry here in _gmap_unmap_pud_fast()?

Looking at arch/s390/kvm/gmap/dat.h:idte_crste():

	unsigned long table_origin = __pa(crste_table_start(crstep));
	unsigned long gaddr = gfn_to_gpa(gfn) & HPAGE_MASK;

table_origin does not OR in the Designation-Type bits (crstep->h.tt << 2),
unlike __pudp_idte() in arch/s390/include/asm/pgtable.h which sets
_ASCE_TYPE_REGION3 (0x04), or crdte_crste() in arch/s390/kvm/gmap/dat.h
which sets new.h.tt << 2.

Because crste_table_start(crstep) is 16K-aligned, bits 60-61 of table_origin
are always 00 (Segment Table). When gmap_unmap_gfn_range_fast() walks a 2G
large PUD (old.h.tt == TABLE_TYPE_REGION3), won't IDTE interpret the
Region-3 table as a Segment Table and index entry 0 using the Segment Index
bits of gaddr instead of the Region-3 Index, causing a translation
specification exception or corrupting entry 0 while leaving *crstep and its
2G TLB entries un-invalidated before *crstep is overwritten?

> +
> +	return 0;
> +}
> +
> +static long _gmap_unmap_pmd_fast(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *w)
> +{
> +	union crste old = *crstep;
> +	struct page_table *pt;
> +	gfn_t first, last;
> +	unsigned int i;
> +
> +	if (old.h.fc) {
> +		if (old.s.fc1.vsie_notif)
> +			gmap_handle_vsie_unshadow_event(w->priv, gfn);
> +
> +		if (machine_has_tlb_guest())
> +			idte_crste(crstep, gfn, IPTE_GUEST_ASCE, w->asce, 0);
> +		else
> +			idte_crste(crstep, gfn, 0, w->asce, 0);
> +		WRITE_ONCE(*crstep, _CRSTE_EMPTY(old.h.tt));
> +
> +		return 0;
> +	}
> +
> +	if (old.h.i)
> +		return 0;
> +
> +	pt = dereference_pmd(old.pmd);
> +	first = max(gfn, w->start) & (PTRS_PER_PTE - 1);
> +	last = (min(next, w->end) - 1) & (PTRS_PER_PTE - 1);
> +
> +	for (i = first; i <= last; i++)
> +		if (pte_needs_unshadow(pt->ptes[i], _PTE_EMPTY, pt->pgstes[i]))
> +			gmap_handle_vsie_unshadow_event(w->priv, gfn + i);
> +
> +	/* Detach the page table */
> +	if (machine_has_tlb_guest())
> +		idte_crste(crstep, gfn, IPTE_GUEST_ASCE, w->asce, 0);
> +	else
> +		idte_crste(crstep, gfn, 0, w->asce, 0);

[Severity: Critical]
Should the three idte_crste() calls in _gmap_unmap_pud_fast() and
_gmap_unmap_pmd_fast() pass IDTE_GUEST_ASCE instead of IPTE_GUEST_ASCE?

In arch/s390/include/asm/pgtable.h, the constants are defined as:

	#define IPTE_GUEST_ASCE	0x800
	...
	#define IDTE_PTOA	0x0800
	#define IDTE_NODAT	0x1000
	#define IDTE_GUEST_ASCE	0x2000

In idte_crste(), opt is ORed into the R2 operand (gaddr | opt). Passing
IPTE_GUEST_ASCE (0x800) sets IDTE_PTOA (bit 52, Clearing-by-ASCE) instead of
IDTE_GUEST_ASCE (0x2000, bit 50).

When machine_has_tlb_guest() is true, won't this execute IDTE in
Clearing-by-ASCE mode with non-zero R1 and gaddr operands (risking a
specification exception), fail to flush guest-ASCE TLB entries, and leave
*crstep valid in memory while pt->ptes[i] and pt->pgstes[i] are modified
below?

> +
> +	for (i = first; i <= last; i++) {
> +		if (pte_hole(pt->ptes[i]))
> +			continue;
> +		if (!pt->ptes[i].h.i && uses_skeys(w->priv))
> +			pt->pgstes[i] = dat_save_storage_key_into_pgste(pt->ptes[i], pt->pgstes[i]);
> +		pt->ptes[i] = _PTE_EMPTY;
> +		pt->pgstes[i].vsie_notif = 0;
> +		pt->pgstes[i].vsie_gmem = 0;
> +		pt->pgstes[i].zero = 0;
> +	}

[Severity: Medium]
Should this loop in _gmap_unmap_pmd_fast() also clear
pt->pgstes[i].prefix_notif?

In the slow path, _gmap_unmap_pte() -> gmap_ptep_xchg() -> _gmap_ptep_xchg()
clears pgste.prefix_notif when installing _PTE_EMPTY (newpte.h.i == 1):

	if (pgste.prefix_notif && (newpte.h.p || newpte.h.i)) {
		pgste.prefix_notif = 0;
		gmap_unmap_prefix(gmap, gfn, gfn + 1);
	}

Although gmap_unmap_gfn_range_fast() already calls gmap_unmap_prefix() on
the aligned range, leaving pt->pgstes[i].prefix_notif set on _PTE_EMPTY
entries means that when the page is later faulted back in via _gmap_link()
-> gmap_ptep_xchg(), a read fault (newpte.h.p == 1) will trigger an
unnecessary gmap_unmap_prefix(), while a write fault (newpte.h.p == 0 &&
newpte.h.i == 0) will keep the stale prefix_notif bit set on the new PTE.

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