Re: [kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

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

On Wed, 27 Apr 2022 12:06:09 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Improve readability by making the return value of tprot() an enum.
> 
> No functional change intended.

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

but see nit below

> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 11 +++++++++--
>  lib/s390x/sclp.c         |  6 +++---
>  s390x/tprot.c            | 24 ++++++++++++------------
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index bab3c374..46c370e6 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -228,7 +228,14 @@ static inline uint64_t stidp(void)
>  	return cpuid;
>  }
>  
> -static inline int tprot(unsigned long addr, char access_key)
> +enum tprot_permission {
> +	TPROT_READ_WRITE = 0,
> +	TPROT_READ = 1,
> +	TPROT_RW_PROTECTED = 2,
> +	TPROT_TRANSL_UNAVAIL = 3,
> +};
> +
> +static inline enum tprot_permission tprot(unsigned long addr, char access_key)
>  {
>  	int cc;
>  
> @@ -237,7 +244,7 @@ static inline int tprot(unsigned long addr, char access_key)
>  		"	ipm	%0\n"
>  		"	srl	%0,28\n"
>  		: "=d" (cc) : "a" (addr), "a" (access_key << 4) : "cc");
> -	return cc;
> +	return (enum tprot_permission)cc;
>  }
>  
>  static inline void lctlg(int cr, uint64_t value)
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 33985eb4..b8204c5f 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -198,7 +198,7 @@ int sclp_service_call(unsigned int command, void *sccb)
>  void sclp_memory_setup(void)
>  {
>  	uint64_t rnmax, rnsize;
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	assert(read_info);
>  
> @@ -222,9 +222,9 @@ void sclp_memory_setup(void)
>  	/* probe for r/w memory up to max memory size */
>  	while (ram_size < max_ram_size) {
>  		expect_pgm_int();
> -		cc = tprot(ram_size + storage_increment_size - 1, 0);
> +		permission = tprot(ram_size + storage_increment_size - 1, 0);
>  		/* stop once we receive an exception or have protected memory */
> -		if (clear_pgm_int() || cc != 0)
> +		if (clear_pgm_int() || permission != TPROT_READ_WRITE)
>  			break;
>  		ram_size += storage_increment_size;
>  	}
> diff --git a/s390x/tprot.c b/s390x/tprot.c
> index 460a0db7..8eb91c18 100644
> --- a/s390x/tprot.c
> +++ b/s390x/tprot.c
> @@ -20,26 +20,26 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
>  
>  static void test_tprot_rw(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("Page read/writeable");
>  
> -	cc = tprot((unsigned long)pagebuf, 0);
> -	report(cc == 0, "CC = 0");
> +	permission = tprot((unsigned long)pagebuf, 0);
> +	report(permission == TPROT_READ_WRITE, "CC = 0");

here and in all similar cases below: does it still make sense to have
"CC = 0" as message at this point? Maybe a more descriptive one would
be better

>  
>  	report_prefix_pop();
>  }
>  
>  static void test_tprot_ro(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("Page readonly");
>  
>  	protect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
>  
> -	cc = tprot((unsigned long)pagebuf, 0);
> -	report(cc == 1, "CC = 1");
> +	permission = tprot((unsigned long)pagebuf, 0);
> +	report(permission == TPROT_READ, "CC = 1");
>  
>  	unprotect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
>  
> @@ -48,28 +48,28 @@ static void test_tprot_ro(void)
>  
>  static void test_tprot_low_addr_prot(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("low-address protection");
>  
>  	low_prot_enable();
> -	cc = tprot(0, 0);
> +	permission = tprot(0, 0);
>  	low_prot_disable();
> -	report(cc == 1, "CC = 1");
> +	report(permission == TPROT_READ, "CC = 1");
>  
>  	report_prefix_pop();
>  }
>  
>  static void test_tprot_transl_unavail(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("Page translation unavailable");
>  
>  	protect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
>  
> -	cc = tprot((unsigned long)pagebuf, 0);
> -	report(cc == 3, "CC = 3");
> +	permission = tprot((unsigned long)pagebuf, 0);
> +	report(permission == TPROT_TRANSL_UNAVAIL, "CC = 3");
>  
>  	unprotect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
>