| Message ID | 20200109161625.154894-4-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: SCLP Unit test | expand |
On 09/01/2020 17.16, Claudio Imbrenda wrote: > Add a wrapper for the SET PREFIX and STORE PREFIX instructions, and > use it instead of using inline assembly everywhere. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > lib/s390x/asm/arch_def.h | 10 ++++++++++ > s390x/intercept.c | 33 +++++++++++++-------------------- > 2 files changed, 23 insertions(+), 20 deletions(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index 1a5e3c6..465fe0f 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, unsigned long sccb) > return cc; > } > > +static inline void spx(uint32_t *new_prefix) Looking at this a second time ... why is new_prefix a pointer? A normal value should be sufficient here, shouldn't it? > +{ > + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); > +} > + > +static inline void stpx(uint32_t *current_prefix) > +{ > + asm volatile("stpx %0" : "=Q" (*current_prefix)); > +} > + Thomas
On Thu, 9 Jan 2020 17:43:55 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 09/01/2020 17.16, Claudio Imbrenda wrote: > > Add a wrapper for the SET PREFIX and STORE PREFIX instructions, and > > use it instead of using inline assembly everywhere. > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > --- > > lib/s390x/asm/arch_def.h | 10 ++++++++++ > > s390x/intercept.c | 33 +++++++++++++-------------------- > > 2 files changed, 23 insertions(+), 20 deletions(-) > > > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > > index 1a5e3c6..465fe0f 100644 > > --- a/lib/s390x/asm/arch_def.h > > +++ b/lib/s390x/asm/arch_def.h > > @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, > > unsigned long sccb) return cc; > > } > > > > +static inline void spx(uint32_t *new_prefix) > > Looking at this a second time ... why is new_prefix a pointer? A > normal value should be sufficient here, shouldn't it? no. if you look at the code in the same patch, intercept.c at some points needs to pass "wrong" pointers to spx and stpx in order to test them, so this needs to be a pointer the instructions themselves expect pointers (base register + offset) > > +{ > > + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); > > +} > > + > > +static inline void stpx(uint32_t *current_prefix) > > +{ > > + asm volatile("stpx %0" : "=Q" (*current_prefix)); > > +} > > + > > Thomas >
On 09/01/2020 17.50, Claudio Imbrenda wrote: > On Thu, 9 Jan 2020 17:43:55 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> On 09/01/2020 17.16, Claudio Imbrenda wrote: >>> Add a wrapper for the SET PREFIX and STORE PREFIX instructions, and >>> use it instead of using inline assembly everywhere. >>> >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> >>> --- >>> lib/s390x/asm/arch_def.h | 10 ++++++++++ >>> s390x/intercept.c | 33 +++++++++++++-------------------- >>> 2 files changed, 23 insertions(+), 20 deletions(-) >>> >>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >>> index 1a5e3c6..465fe0f 100644 >>> --- a/lib/s390x/asm/arch_def.h >>> +++ b/lib/s390x/asm/arch_def.h >>> @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, >>> unsigned long sccb) return cc; >>> } >>> >>> +static inline void spx(uint32_t *new_prefix) >> >> Looking at this a second time ... why is new_prefix a pointer? A >> normal value should be sufficient here, shouldn't it? > > no. if you look at the code in the same patch, intercept.c at some > points needs to pass "wrong" pointers to spx and stpx in order to test > them, so this needs to be a pointer > > the instructions themselves expect pointers (base register + offset) Ah, you're right, that "Q" constraint always confuses me... I guess you could do it without pointers when using the "r" constraint, but it's likely better to do it the same way as stpx, so your patch should be fine. >>> +{ >>> + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); >>> +} >>> + >>> +static inline void stpx(uint32_t *current_prefix) >>> +{ >>> + asm volatile("stpx %0" : "=Q" (*current_prefix)); >>> +} >>> + Reviewed-by: Thomas Huth <thuth@redhat.com>
On 1/9/20 5:58 PM, Thomas Huth wrote: > On 09/01/2020 17.50, Claudio Imbrenda wrote: >> On Thu, 9 Jan 2020 17:43:55 +0100 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> On 09/01/2020 17.16, Claudio Imbrenda wrote: >>>> Add a wrapper for the SET PREFIX and STORE PREFIX instructions, and >>>> use it instead of using inline assembly everywhere. >>>> >>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> >>>> --- >>>> lib/s390x/asm/arch_def.h | 10 ++++++++++ >>>> s390x/intercept.c | 33 +++++++++++++-------------------- >>>> 2 files changed, 23 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >>>> index 1a5e3c6..465fe0f 100644 >>>> --- a/lib/s390x/asm/arch_def.h >>>> +++ b/lib/s390x/asm/arch_def.h >>>> @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, >>>> unsigned long sccb) return cc; >>>> } >>>> >>>> +static inline void spx(uint32_t *new_prefix) >>> >>> Looking at this a second time ... why is new_prefix a pointer? A >>> normal value should be sufficient here, shouldn't it? >> >> no. if you look at the code in the same patch, intercept.c at some >> points needs to pass "wrong" pointers to spx and stpx in order to test >> them, so this needs to be a pointer >> >> the instructions themselves expect pointers (base register + offset) > > Ah, you're right, that "Q" constraint always confuses me... I guess you > could do it without pointers when using the "r" constraint, but it's > likely better to do it the same way as stpx, so your patch should be fine. Honestly, I'd rather have stpx return a u32 than passing a ptr. That's how the kernel does it and is in-line with epswe/lpswe and sctlg/lctlg which are already in the library. Also, if possible names like set_prefix and store_prefix (or better get_prefix) prefix would make it much more readable. > >>>> +{ >>>> + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); >>>> +} >>>> + >>>> +static inline void stpx(uint32_t *current_prefix) >>>> +{ >>>> + asm volatile("stpx %0" : "=Q" (*current_prefix)); >>>> +} >>>> + > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On Thu, 9 Jan 2020 17:58:11 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 09/01/2020 17.50, Claudio Imbrenda wrote: > > On Thu, 9 Jan 2020 17:43:55 +0100 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> On 09/01/2020 17.16, Claudio Imbrenda wrote: > >>> Add a wrapper for the SET PREFIX and STORE PREFIX instructions, > >>> and use it instead of using inline assembly everywhere. > >>> > >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > >>> --- > >>> lib/s390x/asm/arch_def.h | 10 ++++++++++ > >>> s390x/intercept.c | 33 +++++++++++++-------------------- > >>> 2 files changed, 23 insertions(+), 20 deletions(-) > >>> > >>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > >>> index 1a5e3c6..465fe0f 100644 > >>> --- a/lib/s390x/asm/arch_def.h > >>> +++ b/lib/s390x/asm/arch_def.h > >>> @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, > >>> unsigned long sccb) return cc; > >>> } > >>> > >>> +static inline void spx(uint32_t *new_prefix) > >> > >> Looking at this a second time ... why is new_prefix a pointer? A > >> normal value should be sufficient here, shouldn't it? > > > > no. if you look at the code in the same patch, intercept.c at some > > points needs to pass "wrong" pointers to spx and stpx in order to > > test them, so this needs to be a pointer > > > > the instructions themselves expect pointers (base register + > > offset) > > Ah, you're right, that "Q" constraint always confuses me... I guess > you could do it without pointers when using the "r" constraint, but actually no :) I think "r" allows for register 0, which is handled specially when used as base register, so we'd need at least "a". but, by using Q, the compiler generates the "best" combination of opcodes, so for example spx((void *)1L) becomes simply "SPX 1" and so on > it's likely better to do it the same way as stpx, so your patch > should be fine. > > >>> +{ > >>> + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); > >>> +} > >>> + > >>> +static inline void stpx(uint32_t *current_prefix) > >>> +{ > >>> + asm volatile("stpx %0" : "=Q" (*current_prefix)); > >>> +} > >>> + > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On Thu, 9 Jan 2020 18:05:42 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 1/9/20 5:58 PM, Thomas Huth wrote: > > On 09/01/2020 17.50, Claudio Imbrenda wrote: > >> On Thu, 9 Jan 2020 17:43:55 +0100 > >> Thomas Huth <thuth@redhat.com> wrote: > >> > >>> On 09/01/2020 17.16, Claudio Imbrenda wrote: > >>>> Add a wrapper for the SET PREFIX and STORE PREFIX instructions, > >>>> and use it instead of using inline assembly everywhere. > >>>> > >>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > >>>> --- > >>>> lib/s390x/asm/arch_def.h | 10 ++++++++++ > >>>> s390x/intercept.c | 33 +++++++++++++-------------------- > >>>> 2 files changed, 23 insertions(+), 20 deletions(-) > >>>> > >>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > >>>> index 1a5e3c6..465fe0f 100644 > >>>> --- a/lib/s390x/asm/arch_def.h > >>>> +++ b/lib/s390x/asm/arch_def.h > >>>> @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, > >>>> unsigned long sccb) return cc; > >>>> } > >>>> > >>>> +static inline void spx(uint32_t *new_prefix) > >>> > >>> Looking at this a second time ... why is new_prefix a pointer? A > >>> normal value should be sufficient here, shouldn't it? > >> > >> no. if you look at the code in the same patch, intercept.c at some > >> points needs to pass "wrong" pointers to spx and stpx in order to > >> test them, so this needs to be a pointer > >> > >> the instructions themselves expect pointers (base register + > >> offset) > > > > Ah, you're right, that "Q" constraint always confuses me... I guess > > you could do it without pointers when using the "r" constraint, but > > it's likely better to do it the same way as stpx, so your patch > > should be fine. > > Honestly, I'd rather have stpx return a u32 than passing a ptr. that's what I had done initially, but it doesn't work, see above for the reasons why we need a pointer > That's how the kernel does it and is in-line with epswe/lpswe and > sctlg/lctlg which are already in the library. the kernel does not need to test wrong addresses. I could have spx accept an int and stpx return an int, but then intercept.c would still need some inline assembly for SPX and STPX > Also, if possible names like set_prefix and store_prefix (or better > get_prefix) prefix would make it much more readable. this can be done, but that's not how all the other wrappers are > > > >>>> +{ > >>>> + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); > >>>> +} > >>>> + > >>>> +static inline void stpx(uint32_t *current_prefix) > >>>> +{ > >>>> + asm volatile("stpx %0" : "=Q" (*current_prefix)); > >>>> +} > >>>> + > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > > >
On 1/9/20 6:13 PM, Claudio Imbrenda wrote: > On Thu, 9 Jan 2020 18:05:42 +0100 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> On 1/9/20 5:58 PM, Thomas Huth wrote: >>> On 09/01/2020 17.50, Claudio Imbrenda wrote: >>>> On Thu, 9 Jan 2020 17:43:55 +0100 >>>> Thomas Huth <thuth@redhat.com> wrote: >>>> >>>>> On 09/01/2020 17.16, Claudio Imbrenda wrote: >>>>>> Add a wrapper for the SET PREFIX and STORE PREFIX instructions, >>>>>> and use it instead of using inline assembly everywhere. >>>>>> >>>>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> >>>>>> --- >>>>>> lib/s390x/asm/arch_def.h | 10 ++++++++++ >>>>>> s390x/intercept.c | 33 +++++++++++++-------------------- >>>>>> 2 files changed, 23 insertions(+), 20 deletions(-) >>>>>> >>>>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >>>>>> index 1a5e3c6..465fe0f 100644 >>>>>> --- a/lib/s390x/asm/arch_def.h >>>>>> +++ b/lib/s390x/asm/arch_def.h >>>>>> @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, >>>>>> unsigned long sccb) return cc; >>>>>> } >>>>>> >>>>>> +static inline void spx(uint32_t *new_prefix) >>>>> >>>>> Looking at this a second time ... why is new_prefix a pointer? A >>>>> normal value should be sufficient here, shouldn't it? >>>> >>>> no. if you look at the code in the same patch, intercept.c at some >>>> points needs to pass "wrong" pointers to spx and stpx in order to >>>> test them, so this needs to be a pointer >>>> >>>> the instructions themselves expect pointers (base register + >>>> offset) >>> >>> Ah, you're right, that "Q" constraint always confuses me... I guess >>> you could do it without pointers when using the "r" constraint, but >>> it's likely better to do it the same way as stpx, so your patch >>> should be fine. >> >> Honestly, I'd rather have stpx return a u32 than passing a ptr. > > that's what I had done initially, but it doesn't work, see above for > the reasons why we need a pointer I prefer having the "normal" usage in the library and the abnormal usage as inline assembly, that's most often why we use inline assembly anyway (apart from the lib). The library doesn't need to fit every use-case, but rather serve the most used things to increase development speed and readability. > >> That's how the kernel does it and is in-line with epswe/lpswe and >> sctlg/lctlg which are already in the library. > > the kernel does not need to test wrong addresses. > > I could have spx accept an int and stpx return an int, but then > intercept.c would still need some inline assembly for SPX and STPX > >> Also, if possible names like set_prefix and store_prefix (or better >> get_prefix) prefix would make it much more readable. > > this can be done, but that's not how all the other wrappers are Maybe it's just me, but I always confuse stpx with spx since the designers did not use lpx/stpx which is much more obvious. > >>> >>>>>> +{ >>>>>> + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); >>>>>> +} >>>>>> + >>>>>> +static inline void stpx(uint32_t *current_prefix) >>>>>> +{ >>>>>> + asm volatile("stpx %0" : "=Q" (*current_prefix)); >>>>>> +} >>>>>> + >>> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> >> >> >
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index 1a5e3c6..465fe0f 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -284,4 +284,14 @@ static inline int servc(uint32_t command, unsigned long sccb) return cc; } +static inline void spx(uint32_t *new_prefix) +{ + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); +} + +static inline void stpx(uint32_t *current_prefix) +{ + asm volatile("stpx %0" : "=Q" (*current_prefix)); +} + #endif diff --git a/s390x/intercept.c b/s390x/intercept.c index 3696e33..3b53633 100644 --- a/s390x/intercept.c +++ b/s390x/intercept.c @@ -26,27 +26,24 @@ static void test_stpx(void) uint32_t new_prefix = (uint32_t)(intptr_t)pagebuf; /* Can we successfully change the prefix? */ - asm volatile ( - " stpx %0\n" - " spx %2\n" - " stpx %1\n" - " spx %0\n" - : "+Q"(old_prefix), "+Q"(tst_prefix) - : "Q"(new_prefix)); + stpx(&old_prefix); + spx(&new_prefix); + stpx(&tst_prefix); + spx(&old_prefix); report(old_prefix == 0 && tst_prefix == new_prefix, "store prefix"); expect_pgm_int(); low_prot_enable(); - asm volatile(" stpx 0(%0) " : : "r"(8)); + stpx((void *)8L); low_prot_disable(); check_pgm_int_code(PGM_INT_CODE_PROTECTION); expect_pgm_int(); - asm volatile(" stpx 0(%0) " : : "r"(1)); + stpx((void *)1L); check_pgm_int_code(PGM_INT_CODE_SPECIFICATION); expect_pgm_int(); - asm volatile(" stpx 0(%0) " : : "r"(-8L)); + stpx((void *)-8L); check_pgm_int_code(PGM_INT_CODE_ADDRESSING); } @@ -63,22 +60,18 @@ static void test_spx(void) * some facility bits there ... at least some of them should be * set in our buffer afterwards. */ - asm volatile ( - " stpx %0\n" - " spx %1\n" - " stfl 0\n" - " spx %0\n" - : "+Q"(old_prefix) - : "Q"(new_prefix) - : "memory"); + stpx(&old_prefix); + spx(&new_prefix); + asm volatile (" stfl 0" : : : "memory"); + spx(&old_prefix); report(pagebuf[GEN_LC_STFL] != 0, "stfl to new prefix"); expect_pgm_int(); - asm volatile(" spx 0(%0) " : : "r"(1)); + spx((void *)1L); check_pgm_int_code(PGM_INT_CODE_SPECIFICATION); expect_pgm_int(); - asm volatile(" spx 0(%0) " : : "r"(-8L)); + spx((void *)-8L); check_pgm_int_code(PGM_INT_CODE_ADDRESSING); }
Add a wrapper for the SET PREFIX and STORE PREFIX instructions, and use it instead of using inline assembly everywhere. Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- lib/s390x/asm/arch_def.h | 10 ++++++++++ s390x/intercept.c | 33 +++++++++++++-------------------- 2 files changed, 23 insertions(+), 20 deletions(-)