Re: [kvm-unit-tests PATCH v2 3/4] lib: s390x: sie: Memory rework

Christoph Schlameuss
2026-04-21T15:25:45Z

On Mon Apr 20, 2026 at 10:44 AM CEST, Janosch Frank wrote:
> Make sie_guest_create() directly alloc the guest's memory.
>
> Also 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>

with nit fixed:
Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
> lib/s390x/sie.c | 49 ++++++++++++++++++++++++---------------------
> lib/s390x/sie.h | 2 +-
> lib/s390x/snippet.h | 9 +++++----
> s390x/mvpg-sie.c | 2 +-
> s390x/pv-diags.c | 2 +-
> s390x/pv-edat1.c | 5 +----
> s390x/pv-icptcode.c | 4 ++--
> s390x/pv-ipl.c | 2 +-
> s390x/sie-dat.c | 2 +-
> s390x/sie.c | 13 ++++--------
> s390x/spec_ex-sie.c | 2 ++
> s390x/stfle-sie.c | 2 ++
> 12 files changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index b1b0a8a0..f0e1c41b 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -121,29 +121,6 @@ void sie_guest_sca_create(struct vm *vm)
> vm->sca->cpu[0].sda = (uint64_t)vm->sblk;
> }
>
> -/* Initializes the struct vm members like the SIE control block. */
> -void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len)
> -{
> - vm->sblk = alloc_page();
> - memset(vm->sblk, 0, PAGE_SIZE);
> - vm->sblk->cpuflags = CPUSTAT_ZARCH | CPUSTAT_RUNNING;
> - vm->sblk->ihcpu = 0xffff;
> - vm->sblk->prefix = 0;
> -
> - /* Guest memory chunks are always 1MB */
> - assert(!(guest_mem_len & ~HPAGE_MASK));
> - vm->guest_mem = (uint8_t *)guest_mem;
> - /* For non-PV guests we re-use the host's ASCE for ease of use */
> - vm->save_area.guest.asce = stctg(1);
> - /* Currently MSO/MSL is the easiest option */
> - vm->sblk->mso = (uint64_t)guest_mem;
> - vm->sblk->msl = (uint64_t)guest_mem + ((guest_mem_len - 1) & HPAGE_MASK);
> -
> - /* CRYCB needs to be in the first 2GB */
> - vm->crycb = alloc_pages_flags(0, AREA_DMA31);
> - vm->sblk->crycbd = (uint32_t)(uintptr_t)vm->crycb;
> -}
> -
> /**
> * sie_guest_alloc() - Allocate memory for a guest and map it in virtual address
> * space such that it is properly aligned.
> @@ -185,6 +162,31 @@ uint8_t *sie_guest_alloc(uint64_t guest_size)
> return guest_virt;
> }
>
> +/* Initializes the struct vm members like the SIE control block. */
> +void sie_guest_create(struct vm *vm, uint64_t guest_mem_len)
> +{
> + void *guest_mem = sie_guest_alloc(guest_mem_len);
> +
> + vm->sblk = alloc_page();
> + memset(vm->sblk, 0, PAGE_SIZE);
> + vm->sblk->cpuflags = CPUSTAT_ZARCH | CPUSTAT_RUNNING;
> + vm->sblk->ihcpu = 0xffff;
> + vm->sblk->prefix = 0;
> +
> + /* Guest memory chunks are always 1MB */
> + assert(!(guest_mem_len & ~HPAGE_MASK));
> + vm->guest_mem = (uint8_t *)guest_mem;
> + /* For non-PV guests we re-use the host's ASCE for ease of use */
> + vm->save_area.guest.asce = stctg(1);
> + /* Currently MSO/MSL is the easiest option */
> + vm->sblk->mso = (uint64_t)guest_mem;
> + vm->sblk->msl = (uint64_t)guest_mem + ((guest_mem_len - 1) & HPAGE_MASK);
> +
> + /* CRYCB needs to be in the first 2GB */
> + vm->crycb = alloc_pages_flags(0, AREA_DMA31);
> + vm->sblk->crycbd = (uint32_t)(uintptr_t)vm->crycb;
> +}
> +

nit:
This looks like you are moving that unchanged. Is that intended? If it is please
add to the commit comment that this is moved unchanged at least or dont move it.

> /* Frees the memory that was gathered on initialization */
> void sie_guest_destroy(struct vm *vm)
> {
> @@ -192,4 +194,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_primary_page_root(), vm->guest_mem));
> }
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 3ec49ed0..85d691d5 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -59,7 +59,7 @@ static inline bool sie_is_pv(struct vm *vm)
> }
>
> void sie_guest_sca_create(struct vm *vm);
> -void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
> +void sie_guest_create(struct vm *vm, uint64_t guest_mem_len);
> void sie_guest_destroy(struct vm *vm);
>
> uint8_t *sie_guest_alloc(uint64_t guest_size);
> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
> index 910849aa..94688f49 100644
> --- a/lib/s390x/snippet.h
> +++ b/lib/s390x/snippet.h
> @@ -125,11 +125,8 @@ static inline void snippet_pv_init(struct vm *vm, const char *gbin,
> /* Allocates and sets up a snippet based guest */
> static inline void snippet_setup_guest(struct vm *vm, bool is_pv)
> {
> - const unsigned long guest_size = SZ_1M;
> - uint8_t *guest_start = sie_guest_alloc(guest_size);
> -
> /* Initialize the vm struct and allocate control blocks */
> - sie_guest_create(vm, (uint64_t)guest_start, guest_size);
> + sie_guest_create(vm, SZ_1M);
>
> if (is_pv) {
> /* FMT4 needs a ESCA */
> @@ -144,4 +141,8 @@ static inline void snippet_setup_guest(struct vm *vm, bool is_pv)
> }
> }
>
> +static inline void snippet_destroy_guest(struct vm *vm)
> +{
> + sie_guest_destroy(vm);
> +}
> #endif
> diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
> index 3050a8bb..21082c53 100644
> --- a/s390x/mvpg-sie.c
> +++ b/s390x/mvpg-sie.c
> @@ -118,7 +118,7 @@ int main(void)
> setup_guest();
> test_mvpg();
> test_mvpg_pei();
> - sie_guest_destroy(&vm);
> + snippet_destroy_guest(&vm);
>
> done:
> report_prefix_pop();
> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c
> index 09b83d59..b6f08dd2 100644
> --- a/s390x/pv-diags.c
> +++ b/s390x/pv-diags.c
> @@ -156,7 +156,7 @@ int main(void)
> test_diag_yield();
> test_diag_288();
> test_diag_500();
> - sie_guest_destroy(&vm);
> + snippet_destroy_guest(&vm);
>
> done:
> report_prefix_pop();
> diff --git a/s390x/pv-edat1.c b/s390x/pv-edat1.c
> index e423f2fc..ff762c75 100644
> --- a/s390x/pv-edat1.c
> +++ b/s390x/pv-edat1.c
> @@ -437,15 +437,12 @@ static bool check_facilities(void)
>
> static void init(void)
> {
> - uint8_t *guest_memory;
> -
> setup_vm();
>
> root = get_primary_page_root();
> ctl_set_bit(0, CTL0_EDAT);
>
> - guest_memory = alloc_pages(GUEST_ORDER - PAGE_SHIFT);
> - sie_guest_create(&vm, (uint64_t)guest_memory, GUEST_SIZE);
> + sie_guest_create(&vm, GUEST_SIZE);
> sie_guest_sca_create(&vm);
> uv_init();
> uv_setup_asces();
> diff --git a/s390x/pv-icptcode.c b/s390x/pv-icptcode.c
> index 5293306b..bdef3a05 100644
> --- a/s390x/pv-icptcode.c
> +++ b/s390x/pv-icptcode.c
> @@ -164,7 +164,7 @@ static void test_validity_handle_not_in_config(void)
>
> /* Destroy the second vm, since we don't need it for further tests */
> uv_destroy_guest(&vm2);
> - sie_guest_destroy(&vm2);
> + snippet_destroy_guest(&vm2);
>
> uv_destroy_guest(&vm);
> report_prefix_pop();
> @@ -368,7 +368,7 @@ int main(void)
> test_validity_handle_not_in_config();
> test_validity_already_running();
> test_validity_timing();
> - sie_guest_destroy(&vm);
> + snippet_destroy_guest(&vm);
>
> done:
> report_prefix_pop();
> diff --git a/s390x/pv-ipl.c b/s390x/pv-ipl.c
> index 61a1e0c0..1219573f 100644
> --- a/s390x/pv-ipl.c
> +++ b/s390x/pv-ipl.c
> @@ -135,7 +135,7 @@ int main(void)
> snippet_setup_guest(&vm, true);
> test_diag_308(0);
> test_diag_308(1);
> - sie_guest_destroy(&vm);
> + snippet_destroy_guest(&vm);
>
> done:
> report_prefix_pop();
> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> index e40e348f..a5ff0872 100644
> --- a/s390x/sie-dat.c
> +++ b/s390x/sie-dat.c
> @@ -101,7 +101,7 @@ int main(void)
>
> setup_guest();
> test_sie_dat();
> - sie_guest_destroy(&vm);
> + snippet_destroy_guest(&vm);
>
> done:
> report_prefix_pop();
> diff --git a/s390x/sie.c b/s390x/sie.c
> index ce5b6069..f08564b1 100644
> --- a/s390x/sie.c
> +++ b/s390x/sie.c
> @@ -20,7 +20,6 @@
> #include <sclp.h>
> #include <sie.h>
>
> -static u8 *guest;
> static u8 *guest_instr;
> static struct vm vm;
>
> @@ -70,7 +69,7 @@ static void test_epoch_ext(void)
> return;
> }
>
> - guest[0] = 0x00;
> + vm.guest_mem[0] = 0x00;
> memcpy(guest_instr, instr, sizeof(instr));
>
> vm.sblk->gpsw.addr = PAGE_SIZE * 2;
> @@ -82,19 +81,15 @@ static void test_epoch_ext(void)
> sie(&vm);
>
> /* ... should result in the same epoch extension here: */
> - report(guest[0] == 0x47, "epdx: different epoch is visible in the guest");
> + report(vm.guest_mem[0] == 0x47, "epdx: different epoch is visible in the guest");
> }
>
> static void setup_guest(void)
> {
> - setup_vm();
> -
> - guest = sie_guest_alloc(SZ_1M);
> + sie_guest_create(&vm, HPAGE_SIZE);
>
> /* The first two pages are the lowcore */
> - guest_instr = guest + PAGE_SIZE * 2;
> -
> - sie_guest_create(&vm, (uint64_t)guest, HPAGE_SIZE);
> + guest_instr = vm.guest_mem + PAGE_SIZE * 2;
> }
>
> int main(void)
> diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
> index fe2f23ee..6ab4144c 100644
> --- a/s390x/spec_ex-sie.c
> +++ b/s390x/spec_ex-sie.c
> @@ -71,6 +71,8 @@ static void test_spec_ex_sie(void)
> report_info("%s", msg);
> else
> report_info("Did not interpret initial exception");
> +
> + snippet_destroy_guest(&vm);
> report_prefix_pop();
> report_prefix_pop();
> }
> diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> index 21cf8ff8..8df1185c 100644
> --- a/s390x/stfle-sie.c
> +++ b/s390x/stfle-sie.c
> @@ -133,6 +133,8 @@ int main(int argc, char **argv)
> setup_guest();
> if (run_format_0)
> test_stfle_format_0();
> +
> + snippet_destroy_guest(&vm);
> out:
> return report_summary();
> }