From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com ([207.211.31.120]:38650 "EHLO us-smtp-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1730676AbgAIQ6W (ORCPT ); Thu, 9 Jan 2020 11:58:22 -0500 Subject: Re: [kvm-unit-tests PATCH v6 3/4] s390x: lib: add SPX and STPX instruction wrapper References: <20200109161625.154894-1-imbrenda@linux.ibm.com> <20200109161625.154894-4-imbrenda@linux.ibm.com> <5c6f563e-3d09-5274-b050-a64122097e9b@redhat.com> <20200109175027.362d8440@p-imbrenda> From: Thomas Huth Message-ID: <3dc2cf13-4829-53cd-a0a6-734fdddeb0ac@redhat.com> Date: Thu, 9 Jan 2020 17:58:11 +0100 MIME-Version: 1.0 In-Reply-To: <20200109175027.362d8440@p-imbrenda> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-s390-owner@vger.kernel.org List-ID: To: Claudio Imbrenda Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org, david@redhat.com, borntraeger@de.ibm.com, frankja@linux.ibm.com On 09/01/2020 17.50, Claudio Imbrenda wrote: > On Thu, 9 Jan 2020 17:43:55 +0100 > Thomas Huth 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 >>> --- >>> 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