Re: [kvm-unit-tests PATCH v2 08/10] lib: s390x: Introduce snippet helpers

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

On 12/8/21 12:46, Claudio Imbrenda wrote:
> On Tue,  7 Dec 2021 16:00:03 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> These helpers reduce code duplication for PV snippet tests.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> although I'd prefer different names for the functions.
> 
> snippet_setup_guest sounds like it is actually doing something
> snippet-related, whereas it is just preparing an empty guest, the
> actual snippet part comes later when you call snippet_init.
> 
> Maybe rename snippet_setup_guest to something like
> "prepare_guest_for_snippet"? (or maybe something shorter, but you get
> what I mean) maybe even "prepare_1m_guest"

snippet_setup_guest_shell() ?

> 
>> ---
>>   lib/s390x/snippet.h | 103 ++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/uv.h      |  21 +++++++++
>>   2 files changed, 124 insertions(+)
>>
>> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
>> index 6b77a8a9..b17b2a4c 100644
>> --- a/lib/s390x/snippet.h
>> +++ b/lib/s390x/snippet.h
>> @@ -9,6 +9,10 @@
>>   #ifndef _S390X_SNIPPET_H_
>>   #define _S390X_SNIPPET_H_
>>   
>> +#include <sie.h>
>> +#include <uv.h>
>> +#include <asm/uv.h>
>> +
>>   /* This macro cuts down the length of the pointers to snippets */
>>   #define SNIPPET_NAME_START(type, file) \
>>   	_binary_s390x_snippets_##type##_##file##_gbin_start
>> @@ -26,6 +30,12 @@
>>   #define SNIPPET_HDR_LEN(type, file) \
>>   	((uintptr_t)SNIPPET_HDR_END(type, file) - (uintptr_t)SNIPPET_HDR_START(type, file))
>>   
>> +#define SNIPPET_PV_TWEAK0	0x42UL
>> +#define SNIPPET_PV_TWEAK1	0UL
>> +#define SNIPPET_OFF_C		0
>> +#define SNIPPET_OFF_ASM		0x4000
>> +
>> +
>>   /*
>>    * C snippet instructions start at 0x4000 due to the prefix and the
>>    * stack being before that. ASM snippets don't strictly need a stack
>> @@ -38,4 +48,97 @@ static const struct psw snippet_psw = {
>>   	.mask = PSW_MASK_64,
>>   	.addr = SNIPPET_ENTRY_ADDR,
>>   };
>> +
>> +/*
>> + * Sets up a snippet guest on top of an existing and initialized SIE
>> + * vm struct.
>> + * Once this function has finished without errors the guest can be started.
>> + *
>> + * @vm: VM that this function will populated, has to be initialized already
>> + * @gbin: Snippet gbin data pointer
>> + * @gbin_len: Length of the gbin data
>> + * @off: Offset from guest absolute 0x0 where snippet is copied to
>> + */
>> +static inline void snippet_init(struct vm *vm, const char *gbin,
>> +				uint64_t gbin_len, uint64_t off)
>> +{
>> +	uint64_t mso = vm->sblk->mso;
>> +
>> +	/* Copy test image to guest memory */
>> +	memcpy((void *)mso + off, gbin, gbin_len);
>> +
>> +	/* Setup guest PSW */
>> +	vm->sblk->gpsw = snippet_psw;
>> +
>> +	/*
>> +	 * We want to exit on PGM exceptions so we don't need
>> +	 * exception handlers in the guest.
>> +	 */
>> +	vm->sblk->ictl = ICTL_OPEREXC | ICTL_PINT;
>> +}
>> +
>> +/*
>> + * Sets up a snippet UV/PV guest on top of an existing and initialized
>> + * SIE vm struct.
>> + * Once this function has finished without errors the guest can be started.
>> + *
>> + * @vm: VM that this function will populated, has to be initialized already
>> + * @gbin: Snippet gbin data pointer
>> + * @hdr: Snippet SE header data pointer
>> + * @gbin_len: Length of the gbin data
>> + * @hdr_len: Length of the hdr data
>> + * @off: Offset from guest absolute 0x0 where snippet is copied to
>> + */
>> +static inline void snippet_pv_init(struct vm *vm, const char *gbin,
>> +				   const char *hdr, uint64_t gbin_len,
>> +				   uint64_t hdr_len, uint64_t off)
>> +{
>> +	uint64_t tweak[2] = {SNIPPET_PV_TWEAK0, SNIPPET_PV_TWEAK1};
>> +	uint64_t mso = vm->sblk->mso;
>> +	int i;
>> +
>> +	snippet_init(vm, gbin, gbin_len, off);
>> +
>> +	uv_create_guest(vm);
>> +	uv_set_se_hdr(vm->uv.vm_handle, (void *)hdr, hdr_len);
>> +
>> +	/* Unpack works on guest addresses so we only need off */
>> +	uv_unpack(vm, off, gbin_len, tweak[0]);
>> +	uv_verify_load(vm);
>> +
>> +	/*
>> +	 * Manually import:
>> +	 * - lowcore 0x0 - 0x1000 (asm)
>> +	 * - stack 0x3000 (C)
>> +	 */
>> +	for (i = 0; i < 4; i++) {
>> +		uv_import(vm->uv.vm_handle, mso + PAGE_SIZE * i);
>> +	}
>> +}
>> +
>> +/* Allocates and sets up a snippet based guest */
>> +static inline void snippet_setup_guest(struct vm *vm, bool is_pv)
>> +{
>> +	u8 *guest;
>> +
>> +	/* Allocate 1MB as guest memory */
>> +	guest = alloc_pages(8);
>> +	memset(guest, 0, HPAGE_SIZE);
>> +
>> +	/* Initialize the vm struct and allocate control blocks */
>> +	sie_guest_create(vm, (uint64_t)guest, HPAGE_SIZE);
>> +
>> +	if (is_pv) {
>> +		/* FMT4 needs a ESCA */
>> +		sie_guest_sca_create(vm);
>> +
>> +		/*
>> +		 * Initialize UV and setup the address spaces needed
>> +		 * to run a PV guest.
>> +		 */
>> +		uv_init();
>> +		uv_setup_asces();
>> +	}
>> +}
>> +
>>   #endif
>> diff --git a/lib/s390x/uv.h b/lib/s390x/uv.h
>> index 6ffe537a..8175d9c6 100644
>> --- a/lib/s390x/uv.h
>> +++ b/lib/s390x/uv.h
>> @@ -3,6 +3,7 @@
>>   #define _S390X_UV_H_
>>   
>>   #include <sie.h>
>> +#include <asm/pgtable.h>
>>   
>>   bool uv_os_is_guest(void);
>>   bool uv_os_is_host(void);
>> @@ -14,4 +15,24 @@ void uv_destroy_guest(struct vm *vm);
>>   int uv_unpack(struct vm *vm, uint64_t addr, uint64_t len, uint64_t
>> tweak); void uv_verify_load(struct vm *vm);
>>   
>> +/*
>> + * To run PV guests we need to setup a few things:
>> + * - A valid primary ASCE that contains the guest memory and has the
>> P bit set.
>> + * - A valid home space ASCE for the UV calls that use home space
>> addresses.
>> + */
>> +static inline void uv_setup_asces(void)
>> +{
>> +	uint64_t asce;
>> +
>> +	/* We need to have a valid primary ASCE to run guests. */
>> +	setup_vm();
>> +
>> +	/* Set P bit in ASCE as it is required for PV guests */
>> +	asce = stctg(1) | ASCE_P;
>> +	lctlg(1, asce);
>> +
>> +	/* Copy ASCE into home space CR */
>> +	lctlg(13, asce);
>> +}
>> +
>>   #endif /* UV_H */
>