From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:51394 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728131AbgLQPqI (ORCPT ); Thu, 17 Dec 2020 10:46:08 -0500 References: <20201211100039.63597-1-frankja@linux.ibm.com> <20201211100039.63597-6-frankja@linux.ibm.com> <0bb4934a-23b6-bf4f-2742-3892c17c81d0@redhat.com> From: Janosch Frank Subject: Re: [kvm-unit-tests PATCH v3 5/8] s390x: sie: Add SIE to lib Message-ID: <365acc5e-0f57-ed9e-cee3-b321827fd2b6@linux.ibm.com> Date: Thu, 17 Dec 2020 16:45:21 +0100 MIME-Version: 1.0 In-Reply-To: <0bb4934a-23b6-bf4f-2742-3892c17c81d0@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit List-ID: To: Thomas Huth , kvm@vger.kernel.org Cc: david@redhat.com, borntraeger@de.ibm.com, imbrenda@linux.ibm.com, cohuck@redhat.com, linux-s390@vger.kernel.org On 12/17/20 10:37 AM, Thomas Huth wrote: > On 11/12/2020 11.00, Janosch Frank wrote: >> This commit adds the definition of the SIE control block struct and >> the assembly to execute SIE and save/restore guest registers. >> >> Signed-off-by: Janosch Frank >> --- >> lib/s390x/asm-offsets.c | 13 +++ >> lib/s390x/asm/arch_def.h | 7 ++ >> lib/s390x/interrupt.c | 7 ++ >> lib/s390x/sie.h | 197 +++++++++++++++++++++++++++++++++++++++ >> s390x/asm/lib.S | 56 +++++++++++ >> 5 files changed, 280 insertions(+) >> create mode 100644 lib/s390x/sie.h >> >> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c >> index ee94ed3..35697de 100644 >> --- a/lib/s390x/asm-offsets.c >> +++ b/lib/s390x/asm-offsets.c >> @@ -8,6 +8,7 @@ >> #include >> #include >> #include >> +#include >> >> int main(void) >> { >> @@ -69,6 +70,18 @@ int main(void) >> OFFSET(GEN_LC_ARS_SA, lowcore, ars_sa); >> OFFSET(GEN_LC_CRS_SA, lowcore, crs_sa); >> OFFSET(GEN_LC_PGM_INT_TDB, lowcore, pgm_int_tdb); >> + OFFSET(__SF_GPRS, stack_frame, gprs); >> + OFFSET(__SF_SIE_CONTROL, stack_frame, empty1[0]); >> + OFFSET(__SF_SIE_SAVEAREA, stack_frame, empty1[1]); >> + OFFSET(__SF_SIE_REASON, stack_frame, empty1[2]); >> + OFFSET(__SF_SIE_FLAGS, stack_frame, empty1[3]); >> + OFFSET(SIE_SAVEAREA_HOST_GRS, vm_save_area, host.grs[0]); >> + OFFSET(SIE_SAVEAREA_HOST_FPRS, vm_save_area, host.fprs[0]); >> + OFFSET(SIE_SAVEAREA_HOST_FPC, vm_save_area, host.fpc); >> + OFFSET(SIE_SAVEAREA_GUEST_GRS, vm_save_area, guest.grs[0]); >> + OFFSET(SIE_SAVEAREA_GUEST_FPRS, vm_save_area, guest.fprs[0]); >> + OFFSET(SIE_SAVEAREA_GUEST_FPC, vm_save_area, guest.fpc); >> + >> >> return 0; >> } >> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >> index f3ab830..5a13cf2 100644 >> --- a/lib/s390x/asm/arch_def.h >> +++ b/lib/s390x/asm/arch_def.h >> @@ -8,6 +8,13 @@ >> #ifndef _ASM_S390X_ARCH_DEF_H_ >> #define _ASM_S390X_ARCH_DEF_H_ >> >> +struct stack_frame { >> + unsigned long back_chain; >> + unsigned long empty1[5]; >> + unsigned long gprs[10]; >> + unsigned int empty2[8]; > > I think you can drop empty2 ? Since I don't need to allocate it I could also remove the gprs. We only use empty1 right now as far as I know. > >> +}; >> + >> struct psw { >> uint64_t mask; >> uint64_t addr; >> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c >> index bac8862..3858096 100644 >> --- a/lib/s390x/interrupt.c >> +++ b/lib/s390x/interrupt.c >> @@ -11,6 +11,7 @@ >> #include >> #include >> #include >> +#include >> >> static bool pgm_int_expected; >> static bool ext_int_expected; >> @@ -57,6 +58,12 @@ void register_pgm_cleanup_func(void (*f)(void)) >> >> static void fixup_pgm_int(void) >> { >> + /* If we have an error on SIE we directly move to sie_exit */ >> + if (lc->pgm_old_psw.addr >= (uint64_t)&sie_entry && >> + lc->pgm_old_psw.addr <= (uint64_t)&sie_entry + 10) { > > Can you please explain that "magic" number 10 in the comment? I think using sie_exit would make more sense than explaining that sie_entry + 10 bytes is the location of sie_exit. > >> + lc->pgm_old_psw.addr = (uint64_t)&sie_exit; >> + } >> + >> switch (lc->pgm_int_code) { >> case PGM_INT_CODE_PRIVILEGED_OPERATION: >> /* Normal operation is in supervisor state, so this exception >> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h >> new file mode 100644 >> index 0000000..b00bdf4 >> --- /dev/null >> +++ b/lib/s390x/sie.h > [...] >> +extern u64 sie_entry; >> +extern u64 sie_exit; > > Maybe better: > > extern uint16_t sie_entry[]; > extern uint16_t sie_exit[]; > > ? > > Or even: > > extern void sie_entry(); > extern void sie_exit(); Definitely better since I don't return values in sie_exit anymore (I used to before). > > ? > > Thomas >