Re: [kvm-unit-tests PATCH v3 3/6] s390x: sie: switch to home space mode before entering SIE

Janosch Frank
2023-06-05T09:03:31Z

On 6/1/23 09:01, Nico Boehr wrote:
> This is to prepare for running guests without MSO/MSL, which is
> currently not possible.

This is a preparation patch for non-MSO/MSL guest support.

>
> We already have code in sie64a to setup a guest primary ASCE before
> entering SIE, so we can in theory switch to the page tables which
> translate gpa to hpa.

The important information that's missing here is that SIE always uses
the prim space for guest to host translations.

>
> But the host is running in primary space mode already, so changing the
> primary ASCE before entering SIE will also affect the host's code and
> data.

And that's why this is an issue.

For the time being we'll copy the primary ASCE into the home space ASCE.
No functional change is intended. But if a test intends to....

>
> To make this switch useful, the host should run in a different address
> space mode. Hence, set up and change to home address space mode before
> installing the guest ASCE.
>
> The home space ASCE is just copied over from the primary space ASCE, so
> no functional change is intended, also for tests that want to use
> MSO/MSL. If a test intends to use a different primary space ASCE, it can
> now just set the guest.asce in the save_area.
>
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
> lib/s390x/asm/arch_def.h | 1 +
> lib/s390x/sie.c | 18 ++++++++++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 84f6996c4d8c..099289e7550e 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -90,6 +90,7 @@ struct cpu {
> #define AS_HOME 3
>
> #define PSW_MASK_DAT 0x0400000000000000UL
> +#define PSW_MASK_HOME 0x0000C00000000000UL
> #define PSW_MASK_IO 0x0200000000000000UL
> #define PSW_MASK_EXT 0x0100000000000000UL
> #define PSW_MASK_KEY 0x00F0000000000000UL
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 9241b4b4a512..ffa8ec91a423 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -46,6 +46,8 @@ void sie_handle_validity(struct vm *vm)
>
> void sie(struct vm *vm)
> {
> + uint64_t old_cr13;
> +
> if (vm->sblk->sdf == 2)
> memcpy(vm->sblk->pv_grregs, vm->save_area.guest.grs,
> sizeof(vm->save_area.guest.grs));
> @@ -53,6 +55,16 @@ void sie(struct vm *vm)
> /* Reset icptcode so we don't trip over it below */
> vm->sblk->icptcode = 0;
>
> + /* set up home address space to match primary space */
> + old_cr13 = stctg(13);
> + lctlg(13, stctg(1));
> +
> + /* switch to home space so guest tables can be different from host */
> + psw_mask_set_bits(PSW_MASK_HOME);
> +
> + /* also handle all interruptions in home space while in SIE */
> + irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);

I'm wondering why this needs to be two calls when you clearly want to
have a convenience function that does a full space change.

We could introduce:

#define AS_REAL 4

static void mmu_set_addr_space(uint8_t space)
{
if (space < 4)
psw_mask_set_bits(space);
irq_set_dat_mode(space);
}

The "addr" in the function name is optional in my opinion.
@Claudio: What's your opinion?

> +
> while (vm->sblk->icptcode == 0) {
> sie64a(vm->sblk, &vm->save_area);
> sie_handle_validity(vm);
> @@ -60,6 +72,12 @@ void sie(struct vm *vm)
> vm->save_area.guest.grs[14] = vm->sblk->gg14;
> vm->save_area.guest.grs[15] = vm->sblk->gg15;
>
> + irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> + psw_mask_clear_bits(PSW_MASK_HOME);
> +
> + /* restore the old CR 13 */
> + lctlg(13, old_cr13);
> +
> if (vm->sblk->sdf == 2)
> memcpy(vm->save_area.guest.grs, vm->sblk->pv_grregs,
> sizeof(vm->save_area.guest.grs));