On Wed, 27 Apr 2022 12:06:09 +0200
Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote:
> Improve readability by making the return value of tprot() an enum.
>
> No functional change intended.
Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
but see nit below
>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
> ---
> 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);
>
