Re: [kvm-unit-tests 3/5] lib: s390x: sie: Free guest memory on destroy

["Nico Boehr" <nrb@linux.ibm.com>]

On Thu Apr 16, 2026 at 10:03 AM CEST, Janosch Frank wrote:
> On 4/15/26 16:21, Nico Boehr wrote:
>> On Wed Apr 15, 2026 at 10:45 AM CEST, Janosch Frank wrote:
>>> We never freed the memory that the sie library allocates as the guest
>>> ram on destruction of the VM. Most tests reuse the VM or just leak the
>>> memory since the standard allocation is one megabyte and tests only
>>> use single digit numbers of VMs.
>>>
>>> It's time to add automatic freeing to the sie library when a VM is
>>> destroyed.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>   lib/s390x/sie.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
>>> index a49c45c7..758ead77 100644
>>> --- a/lib/s390x/sie.c
>>> +++ b/lib/s390x/sie.c
>>> @@ -192,4 +192,5 @@ void sie_guest_destroy(struct vm *vm)
>>>   	free_page(vm->sblk);
>>>   	if (vm->sblk->ecb2 & ECB2_ESCA)
>>>   		free_page(vm->sca);
>>> +	free_pages((void *)virt_to_pte_phys(get_page_root(), vm->guest_mem));
>>>   }
>> 
>> Is there a particular reason why we have a two-step process of calling
>> sie_guest_alloc() for memory allocation
>> and then
>> sie_guest_create() for initializing the sie_block etc
>> 
>> but
>> sie_guest_destroy()
>> cleans up both?
>> 
>
> Historical reasons I guess.
> There've been a lot of changes to the SIE lib over the years, snippet 
> support itself isn't that consistent but it's currently good enough.
> s390x/sie.c is the only direct user of the allocation and even that 
> usage looks like it could be converted.

Yeah, I am a bit concerned about the intermixed dependencies and concerns
between the two. Especially having free_pages() here even when the guest may
(theoretically) have been allocated with something other than sie_guest_alloc().
It would be an error that's kind of hard to spot in a review since it's not
obvious that sie_guest_destroy() pairs with sie_guest_alloc() AND
sie_guest_create().

Would this be an option:
- add small function sie_guest_free() which frees only guest memory to sie lib
  this would be the counterpart to sie_guest_alloc()
- add function snippet_destroy_guest()
  which calls sie_guest_destroy() and sie_guest_free()
  this would be the counterpart to snippet_setup_guest()

Then this pairs nicely as follows for the SIE lib:
sie_guest_alloc() - sie_guest_free()
sie_guest_create() - sie_guest_destroy()

And the higher level snippet operations:
snippet_setup_guest() - snippet_destroy_guest()