Re: [PATCH v2 09/20] KVM: s390: KVM page table management functions: clear and replace

[Janosch Frank <frankja@linux.ibm.com>]

On 9/11/25 3:19 PM, Claudio Imbrenda wrote:
> On Thu, 11 Sep 2025 14:57:40 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 9/10/25 8:07 PM, Claudio Imbrenda wrote:
>>> Add page table management functions to be used for KVM guest (gmap)
>>> page tables.
>>>
>>> This patch adds functions to clear, replace or exchange DAT table
>>> entries.
>>>
>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> ---
>>>    arch/s390/kvm/dat.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
>>>    arch/s390/kvm/dat.h |  40 +++++++++++++++
>>>    2 files changed, 160 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
>>> index 326be78adcda..f26e3579bd77 100644
>>> --- a/arch/s390/kvm/dat.c
>>> +++ b/arch/s390/kvm/dat.c
>>> @@ -89,3 +89,123 @@ void dat_free_level(struct crst_table *table, bool owns_ptes)
>>>    	}
>>>    	dat_free_crst(table);
>>>    }
>>> +
>>> +/**
>>> + * dat_crstep_xchg - exchange a guest CRST with another
>>> + * @crstep: pointer to the CRST entry
>>> + * @new: replacement entry
>>> + * @gfn: the affected guest address
>>> + * @asce: the ASCE of the address space
>>> + *
>>> + * This function is assumed to be called with the guest_table_lock
>>> + * held.
>>> + */
>>> +void dat_crstep_xchg(union crste *crstep, union crste new, gfn_t gfn, union asce asce)
>>> +{
>>> +	if (crstep->h.i) {
>>> +		WRITE_ONCE(*crstep, new);
>>> +		return;
>>> +	} else if (cpu_has_edat2()) {
>>> +		crdte_crste(crstep, *crstep, new, gfn, asce);
>>> +		return;
>>> +	}
>>> +
>>> +	if (machine_has_tlb_guest())
>>> +		idte_crste(crstep, gfn, IDTE_GUEST_ASCE, asce, IDTE_GLOBAL);
>>> +	else if (cpu_has_idte())
>>> +		idte_crste(crstep, gfn, 0, NULL_ASCE, IDTE_GLOBAL);
>>> +	else
>>> +		csp_invalidate_crste(crstep);
>>
>> I'm wondering if we can make stfle 3 (DTE) a requirement for KVM or
>> Linux as a whole since it was introduced with z990 AFAIK.
> 
> AFAIK we don't support machines older than z10 anyway
> 
> but in that case we can only get rid of csp_invalidate_crste(), which
> is not much.
> 
> I can remove it, if you really think it's ugly

That was more of a general question, I think we have more of those 
checks in non-kvm code. Your code is fine as is.

> 
>>
>>> +	WRITE_ONCE(*crstep, new);
>>> +}
>>> +
>>> +/**
>>> + * dat_crstep_xchg_atomic - exchange a gmap pmd with another
>>> + * @crstep: pointer to the crste entry
>>> + * @old: expected old value
>>> + * @new: replacement entry
>>> + * @gfn: the affected guest address
>>> + * @asce: the asce of the address space
>>> + *
>>> + * This function should only be called on invalid crstes, or on crstes with
>>> + * FC = 1, as that guarantees the presence of CSPG.
>>> + *
>>> + * Return: true if the exchange was successful.
>>> + */
>>> +bool dat_crstep_xchg_atomic(union crste *crstep, union crste old, union crste new, gfn_t gfn,
>>> +			    union asce asce)
>>> +{
>>> +	if (old.h.i)
>>> +		return arch_try_cmpxchg((long *)crstep, &old.val, new.val);
>>> +	if (cpu_has_edat2())
>>> +		return crdte_crste(crstep, old, new, gfn, asce);
>>> +	if (cpu_has_idte())
>>> +		return cspg_crste(crstep, old, new);
>>> +
>>> +	WARN_ONCE(1, "Machine does not have CSPG and DAT table was not invalid.");
>>> +	return false;
>>> +}
>