Re: [PATCH v4 6/7] KVM: s390: Introduce gmap_allocate_all_page_tables_best_effort()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: s390: pass DAT_WALK_USES_SKEYS to dat_entry_walk() on split
- [Low] kvm: s390: use return on kvm_s390_mmu_cache_topup() failure
--

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

KVM: s390: Introduce gmap_allocate_all_page_tables_best_effort()

This commit introduces gmap_allocate_all_page_tables_best_effort() to
walk all memslots of a running guest and allocate 4K page tables while
splitting existing large page mappings on a best-effort basis.

> diff --git a/arch/s390/kvm/gmap/gmap.c b/arch/s390/kvm/gmap/gmap.c
> index 04f3ab03a041a..3ff08232cbb36 100644
> --- a/arch/s390/kvm/gmap/gmap.c
> +++ b/arch/s390/kvm/gmap/gmap.c
> @@ -1524,3 +1524,40 @@ struct gmap *gmap_create_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *pare
>  	}
>  	return new;
>  }
> +
> +/* Bail out for any allocation failure. This is only a best effort to allocate all page tables. */
> +void gmap_allocate_all_page_tables_best_effort(struct gmap *gmap)
> +{
> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
> +	struct kvm_memory_slot *ms;
> +	struct kvm_memslots *slots;
> +	gfn_t gfn, start, end;
> +	union crste *crstep;
> +	union pte *ptep;
> +	int bkt;
> +
> +	mc = kvm_s390_new_mmu_cache();
> +	if (!mc)
> +		return;
> +
> +	guard(srcu)(&gmap->kvm->srcu);
> +
> +	slots = kvm_memslots(gmap->kvm);
> +	if (!slots || kvm_memslots_empty(slots))
> +		return;
> +
> +	kvm_for_each_memslot(ms, bkt, slots) {
> +		start = ALIGN(ms->base_gfn, _PAGE_ENTRIES);
> +		end = ALIGN_DOWN(ms->base_gfn + ms->npages, _PAGE_ENTRIES);
> +
> +		for (gfn = start; gfn < end; gfn += _PAGE_ENTRIES) {
> +			if (kvm_s390_mmu_cache_topup(mc))
> +				break;

[Severity: Low]
Should this be a return instead of break?

Using break here only exits the inner GFN loop for the current memslot,
so kvm_for_each_memslot() will continue iterating over any remaining
memslots and retrying allocations after kvm_s390_mmu_cache_topup(mc) has
failed.

> +
> +			guard(write_lock)(&gmap->kvm->mmu_lock);
> +
> +			dat_entry_walk(mc, gfn, gmap->asce, DAT_WALK_ALLOC | DAT_WALK_SPLIT,
> +				       TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);

[Severity: High]
Can splitting an aged large page here lose guest storage keys when
uses_skeys(gmap) is true?

When a 1M or 2G large page is aged via gmap_age_gfn() ->
gmap_clear_young_crste(), the entry is marked invalid (h.i = 1) while
the guest storage keys for its 4K frames remain in hardware storage keys.

When dat_entry_walk() splits that invalid large PMD via dat_split_ste(),
saving the hardware storage keys into the newly allocated 4K PGSTEs is
gated on uses_skeys (from DAT_WALK_USES_SKEYS):

arch/s390/kvm/gmap/dat.c:dat_split_ste() {
    ...
			pgste_init.pcl = uses_skeys && init.h.i;
			dat_init_pgstes(pt, pgste_init.val);
		} else {
			dat_init_page_table(pt, init.val, 0);
		}

		if (dat_pmdp_xchg_atomic(pmdp, old, new, gfn, asce)) {
			if (!pgste_init.pcl)
				return 0;
			for (i = 0; i < _PAGE_ENTRIES; i++) {
				union pgste pgste = pt->pgstes[i];

				pgste = dat_save_storage_key_into_pgste(pt->ptes[i], pgste);
				pgste_set_unlock(pt->ptes + i, pgste);
			}
    ...
}

Because DAT_WALK_USES_SKEYS is not passed to dat_entry_walk(),
pgste_init.pcl is false and the new PGSTEs are left zeroed while the new
PTEs have h.i = 1. When a subsequent minor fault validates the PTE via
_gmap_handle_minor_pte_fault() -> gmap_ptep_xchg() -> __dat_ptep_xchg(),
uses_skeys(gmap) is true, so __dat_ptep_xchg() restores the zeroed PGSTE
over the hardware storage key:

arch/s390/kvm/gmap/dat.c:__dat_ptep_xchg() {
    ...
	if (uses_skeys) {
		if (old.h.i && !new.h.i)
			/* Invalid to valid: restore storage keys from PGSTE. */
			dat_set_storage_key_from_pgste(new, pgste);
    ...
}

Should this pass (uses_skeys(gmap) ? DAT_WALK_USES_SKEYS : 0) in the
dat_entry_walk() flags, similar to gmap_protect_rmap()?

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