On Thu, 9 Jan 2020 17:58:11 +0100
Thomas Huth <thuth@xxxxxxxxxx> wrote:
> On 09/01/2020 17.50, Claudio Imbrenda wrote:
> > On Thu, 9 Jan 2020 17:43:55 +0100
> > Thomas Huth <thuth@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxx>
>
