Re: [kvm-unit-tests PATCH v2 2/8] s390x: Fully commit to stack save area for exceptions

Thomas Huth
2021-02-17T15:37:58Z

On 17/02/2021 15.41, Janosch Frank wrote:
> Having two sets of macros for saving registers on exceptions makes
> maintaining harder. Also we have limited space in the lowcore to save
> stuff and by using the stack as a save area, we can stack exceptions.
>
> So let's use the SAVE/RESTORE_REGS_STACK as the default. When we also
> move the diag308 macro over we can remove the old SAVE/RESTORE_REGS
> macros.
[...]
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 9c4e330a..31c2fc66 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -8,13 +8,30 @@
> #ifndef _ASM_S390X_ARCH_DEF_H_
> #define _ASM_S390X_ARCH_DEF_H_
>
> -/*
> - * We currently only specify the stack frame members needed for the
> - * SIE library code.
> - */
> struct stack_frame {
> - unsigned long back_chain;
> - unsigned long empty1[5];
> + struct stack_frame *back_chain;
> + u64 reserved;
> + /* GRs 2 - 5 */
> + unsigned long argument_area[4];
> + /* GRs 6 - 15 */
> + unsigned long grs[10];
> + /* FPRs 0, 2, 4, 6 */
> + s64 fprs[4];
> +};

For consistency, could you please replace the "unsigned long" with u64, or
even switch to uint64_t completely?

Currently, we have:

$ grep -r u64 lib/s390x/ | wc -l
8
$ grep -r uint64 lib/s390x/ | wc -l
94

... so uint64_t seems to be the better choice.

> +struct stack_frame_int {
> + struct stack_frame *back_chain;
> + u64 reserved;
> + /*
> + * The GRs are offset compatible with struct stack_frame so we
> + * can easily fetch GR14 for backtraces.
> + */
> + u64 grs0[14];
> + u64 grs1[2];

Which registers go into grs0 and which ones into grs1? And why is there a
split at all? A comment would be really helpful!

> + u32 res;

res = reserved? Please add a comment.

> + u32 fpc;
> + u64 fprs[16];
> + u64 crs[16];
> };

Similar, switch to uint32_t and uint64_t ?

> diff --git a/s390x/macros.S b/s390x/macros.S
> index e51a557a..d7eeeb55 100644
> --- a/s390x/macros.S
> +++ b/s390x/macros.S
> @@ -3,9 +3,10 @@
> * s390x assembly macros
> *
> * Copyright (c) 2017 Red Hat Inc
> - * Copyright (c) 2020 IBM Corp.
> + * Copyright (c) 2020, 2021 IBM Corp.
> *
> * Authors:
> + * Janosch Frank <frankja@linux.ibm.com>
> * Pierre Morel <pmorel@linux.ibm.com>
> * David Hildenbrand <david@redhat.com>
> */
> @@ -41,36 +42,45 @@
>
> /* Save registers on the stack (r15), so we can have stacked interrupts. */
> .macro SAVE_REGS_STACK
> - /* Allocate a stack frame for 15 general registers */
> - slgfi %r15, 15 * 8
> + /* Allocate a full stack frame */
> + slgfi %r15, 32 * 8 + 4 * 8

How did you come up with that number? That does neither match stack
stack_frame nor stack_frame_int, if I got this right. Please add a comment
to the code to explain the numbers.

> /* Store registers r0 to r14 on the stack */
> - stmg %r0, %r14, 0(%r15)
> - /* Allocate a stack frame for 16 floating point registers */
> - /* The size of a FP register is the size of an double word */
> - slgfi %r15, 16 * 8
> + stmg %r2, %r15, STACK_FRAME_INT_GRS0(%r15)

Storing up to r14 should be sufficent since you store r15 again below?

> + stg %r0, STACK_FRAME_INT_GRS1(%r15)
> + stg %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
> + /* Store the gr15 value before we allocated the new stack */
> + lgr %r0, %r15
> + algfi %r0, 32 * 8 + 4 * 8
> + stg %r0, 13 * 8 + STACK_FRAME_INT_GRS0(%r15)
> + stg %r0, STACK_FRAME_INT_BACKCHAIN(%r15)
> + /*
> + * Store CR0 and load initial CR0 so AFP is active and we can
> + * access all fprs to save them.
> + */
> + stctg %c0,%c15,STACK_FRAME_INT_CRS(%r15)
> + larl %r1, initial_cr0
> + lctlg %c0, %c0, 0(%r1)
> /* Save fp register on stack: offset to SP is multiple of reg number */
> .irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> - std \i, \i * 8(%r15)
> + std \i, \i * 8 + STACK_FRAME_INT_FPRS(%r15)
> .endr

So you saved 16 GRs, 16 CRs and 16 FPRs onto the stack, that's at least 16 *
3 * 8 = 48 * 8 bytes ... but you only decreased the stack by 32 * 8 + 4 * 8
bytes initially ... is this a bug, or do I miss something?

Thomas