Re: [PATCH v7 08/21] KVM: s390: vsie: Create helpers to alloc and free vsie_pages

["Christoph Schlameuss" <schlameuss@linux.ibm.com>]

On Wed Sep 23, 2026 at 1:54 PM CEST, Janosch Frank wrote:
> On 9/18/26 5:09 PM, Christoph Schlameuss wrote:
>> Encapsulate allocation of struct vsie_page and free into its own methods
>> to allow this from multiple code paths.
>> 
>> Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com>
>> ---
>>   arch/s390/kvm/s390/vsie.c | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/s390/kvm/s390/vsie.c b/arch/s390/kvm/s390/vsie.c
>> index afc2e56fcabf..e7005bade757 100644
>> --- a/arch/s390/kvm/s390/vsie.c
>> +++ b/arch/s390/kvm/s390/vsie.c
>> @@ -1502,6 +1502,24 @@ static void put_vsie_page(struct vsie_page *vsie_page)
>>   	clear_bit(VSIE_PAGE_IN_USE, &vsie_page->flags);
>>   }
>>   
>> +static void free_vsie_page(struct vsie_page *vsie_page)
>> +{
>> +	free_page((unsigned long)vsie_page);
>> +}
>> +
>> +static struct vsie_page *alloc_vsie_page(struct kvm *kvm)
>> +{
>> +	struct vsie_page *vsie_page;
>> +
>> +	vsie_page = (struct vsie_page *)__get_free_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO | GFP_DMA);
>> +	if (!vsie_page)
>> +		return vsie_page;
>> +
>> +	/* Mark it as invalid until it resides in the tree. */
>> +	vsie_page->scb_gpa = ULONG_MAX;
>
> I'd appreciate a properly named constant in favor of the comment being 
> repeated. This has been a thorn in my side for a while now.
>
> #define VSIE_PAGE_INV ULONG_MAX
>

Yes, I like it. Makes it easier to grasp directly without the comments.

>> +	return vsie_page;
>> +}
>> +
>>   /*
>>    * Get or create a vsie page for a scb address.
>>    *
>> @@ -1537,7 +1555,7 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
>>   
>>   	mutex_lock(&kvm->arch.vsie.mutex);
>>   	if (kvm->arch.vsie.page_count < nr_vcpus) {
>> -		vsie_page = (void *)__get_free_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO | GFP_DMA);
>
> Can't we go the kzalloc route like Mike did for the other page 
> allocations? Maybe we can then also get rid of the free function that 
> you're introducing?
>
> https://lore.kernel.org/kvm/20260906-s390-kvm-v2-0-2cf6434e6646@kernel.org/
>

Sure, I do not see a reason not to as long as kzalloc() guarantees the page
alignment (which it does).
Will do the same for the vsie_sca allocation in the other patch (which then
should also be guaranteed to be page aligned continuous memory).

>> +		vsie_page = alloc_vsie_page(kvm);
>>   		if (!vsie_page) {
>>   			mutex_unlock(&kvm->arch.vsie.mutex);
>>   			return ERR_PTR(-ENOMEM);
>> @@ -1557,9 +1575,9 @@ static struct vsie_page *get_vsie_page(struct kvm *kvm, unsigned long addr)
>>   		if (vsie_page->scb_gpa != ULONG_MAX)
>>   			radix_tree_delete(&kvm->arch.vsie.addr_to_page,
>>   					  vsie_page->scb_gpa >> SCB_ALIGNMENT_SHIFT);