Re: [kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Wed, 12 Jul 2023 13:41:44 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Changing the PSW mask is currently little clumsy, since there is only the
> PSW_MASK_* defines. This makes it hard to change e.g. only the address
> space in the current PSW without a lot of bit fiddling.
> 
> Introduce a bitfield for the PSW mask. This makes this kind of
> modifications much simpler and easier to read.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 26 +++++++++++++++++++++++++-
>  s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index bb26e008cc68..53279572a9ee 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -37,12 +37,36 @@ struct stack_frame_int {
>  };
>  
>  struct psw {
> -	uint64_t	mask;
> +	union {
> +		uint64_t	mask;
> +		struct {
> +			uint8_t reserved00:1;
> +			uint8_t per:1;
> +			uint8_t reserved02:3;
> +			uint8_t dat:1;
> +			uint8_t io:1;
> +			uint8_t ext:1;
> +			uint8_t key:4;
> +			uint8_t reserved12:1;
> +			uint8_t mchk:1;
> +			uint8_t wait:1;
> +			uint8_t pstate:1;
> +			uint8_t as:2;
> +			uint8_t cc:2;
> +			uint8_t prg_mask:4;
> +			uint8_t reserved24:7;
> +			uint8_t ea:1;
> +			uint8_t ba:1;
> +			uint32_t reserved33:31;

since you will need to respin this anyway:

can you use uint64_t everywhere in the bitfield? it would look more
consistent and it would avoid some potential pitfalls of using
bitfields. In our case we're lucky because all fields are properly
aligned, but using uint64_t makes it easier to understand.

> +		};
> +	};
>  	uint64_t	addr;
>  };
> +_Static_assert(sizeof(struct psw) == 16, "PSW size");
>  
>  #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
>  
> +
>  struct short_psw {
>  	uint32_t	mask;
>  	uint32_t	addr;
> diff --git a/s390x/selftest.c b/s390x/selftest.c
> index 13fd36bc06f8..8d81ba312279 100644
> --- a/s390x/selftest.c
> +++ b/s390x/selftest.c
> @@ -74,6 +74,45 @@ static void test_malloc(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_psw_mask(void)
> +{
> +	uint64_t expected_key = 0xF;

I think we prefer lowercase hex

> +	struct psw test_psw = PSW(0, 0);
> +
> +	report_prefix_push("PSW mask");
> +	test_psw.dat = 1;
> +	report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.io = 1;
> +	report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.ext = 1;
> +	report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask);
> +
> +	test_psw.mask = expected_key << (63 - 11);
> +	report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key);
> +
> +	test_psw.mask = 1UL << (63 - 13);
> +	report(test_psw.mchk, "MCHK matches");
> +
> +	test_psw.mask = 0;
> +	test_psw.wait = 1;
> +	report(test_psw.mask == PSW_MASK_WAIT, "Wait matches expected=0x%016lx actual=0x%016lx", PSW_MASK_WAIT, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.pstate = 1;
> +	report(test_psw.mask == PSW_MASK_PSTATE, "Pstate matches expected=0x%016lx actual=0x%016lx", PSW_MASK_PSTATE, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.ea = 1;
> +	test_psw.ba = 1;
> +	report(test_psw.mask == PSW_MASK_64, "BA/EA matches expected=0x%016lx actual=0x%016lx", PSW_MASK_64, test_psw.mask);
> +
> +	report_prefix_pop();
> +}
> +
>  int main(int argc, char**argv)
>  {
>  	report_prefix_push("selftest");
> @@ -89,6 +128,7 @@ int main(int argc, char**argv)
>  	test_fp();
>  	test_pgm_int();
>  	test_malloc();
> +	test_psw_mask();
>  
>  	return report_summary();
>  }