| Message ID | 20220427100611.2119860-2-scgl@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v6,1/3] s390x: Give name to return value of tprot() | expand |
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); >
On 4/27/22 13:14, Claudio Imbrenda wrote: > 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/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 I thought about it, but decided against it. Firstly, because I preferred not to do any functional changes and secondly, I could not think of anything better. The prefix already tells you the meaning of the cc, so I don't know what to print that would not be redundant. [...]
On Wed, 27 Apr 2022 14:04:52 +0200 Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > On 4/27/22 13:14, Claudio Imbrenda wrote: > > 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/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 > > I thought about it, but decided against it. Firstly, because I preferred > not to do any functional changes and secondly, I could not think of anything > better. The prefix already tells you the meaning of the cc, so I don't know > what to print that would not be redundant. > > [...] fair enough
On 4/27/22 14:04, Janis Schoetterl-Glausch wrote: > On 4/27/22 13:14, Claudio Imbrenda wrote: >> 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/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 > > I thought about it, but decided against it. Firstly, because I preferred > not to do any functional changes and secondly, I could not think of anything > better. The prefix already tells you the meaning of the cc, so I don't know > what to print that would not be redundant. > > [...] I'm ok with that for now especially considering we're at v6 already and functionally this looks good. Let's add the series to the devel branch so the CI can have a look at it before we pick it. Reviewed-by: Janosch Frank <frankja@linux.ibm.com >
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"); 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);
Improve readability by making the return value of tprot() an enum. No functional change intended. 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(-)