From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:30244 "EHLO mx0b-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388101AbgAIR1Z (ORCPT ); Thu, 9 Jan 2020 12:27:25 -0500 Received: from pps.filterd (m0127361.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 009H2THn047581 for ; Thu, 9 Jan 2020 12:27:24 -0500 Received: from e06smtp04.uk.ibm.com (e06smtp04.uk.ibm.com [195.75.94.100]) by mx0a-001b2d01.pphosted.com with ESMTP id 2xdx6k6v0r-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 09 Jan 2020 12:27:23 -0500 Received: from localhost by e06smtp04.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 9 Jan 2020 17:27:22 -0000 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> <3dc2cf13-4829-53cd-a0a6-734fdddeb0ac@redhat.com> <920208a7-562f-9a54-11cc-6ece021469ec@linux.ibm.com> <20200109181327.6857f1d7@p-imbrenda> From: Janosch Frank Date: Thu, 9 Jan 2020 18:27:16 +0100 MIME-Version: 1.0 In-Reply-To: <20200109181327.6857f1d7@p-imbrenda> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IPc3Hq05fggeF2LONzgb4utJx2bIqwqOy" Message-Id: Sender: linux-s390-owner@vger.kernel.org List-ID: To: Claudio Imbrenda Cc: Thomas Huth , kvm@vger.kernel.org, linux-s390@vger.kernel.org, david@redhat.com, borntraeger@de.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --IPc3Hq05fggeF2LONzgb4utJx2bIqwqOy Content-Type: multipart/mixed; boundary="iNW0m3aBJalZVltgYj95coLkBgfdagUB3" --iNW0m3aBJalZVltgYj95coLkBgfdagUB3 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 1/9/20 6:13 PM, Claudio Imbrenda wrote: > On Thu, 9 Jan 2020 18:05:42 +0100 > Janosch Frank wrote: >=20 >> On 1/9/20 5:58 PM, Thomas Huth wrote: >>> On 09/01/2020 17.50, Claudio Imbrenda wrote: =20 >>>> On Thu, 9 Jan 2020 17:43:55 +0100 >>>> Thomas Huth wrote: >>>> =20 >>>>> On 09/01/2020 17.16, Claudio Imbrenda wrote: =20 >>>>>> 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; >>>>>> } >>>>>> =20 >>>>>> +static inline void spx(uint32_t *new_prefix) =20 >>>>> >>>>> Looking at this a second time ... why is new_prefix a pointer? A >>>>> normal value should be sufficient here, shouldn't it? =20 >>>> >>>> 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) =20 >>> >>> 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. =20 >> >> Honestly, I'd rather have stpx return a u32 than passing a ptr. >=20 > 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. >=20 >> That's how the kernel does it and is in-line with epswe/lpswe and >> sctlg/lctlg which are already in the library. >=20 > the kernel does not need to test wrong addresses. >=20 > 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 >=20 >> Also, if possible names like set_prefix and store_prefix (or better >> get_prefix) prefix would make it much more readable. >=20 > 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. >=20 >>> =20 >>>>>> +{ >>>>>> + asm volatile("spx %0" : : "Q" (*new_prefix) : "memory"); >>>>>> +} >>>>>> + >>>>>> +static inline void stpx(uint32_t *current_prefix) >>>>>> +{ >>>>>> + asm volatile("stpx %0" : "=3DQ" (*current_prefix)); >>>>>> +} >>>>>> + =20 >>> >>> Reviewed-by: Thomas Huth >>> =20 >> >> >=20 --iNW0m3aBJalZVltgYj95coLkBgfdagUB3-- --IPc3Hq05fggeF2LONzgb4utJx2bIqwqOy Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEwGNS88vfc9+v45Yq41TmuOI4ufgFAl4XYnQACgkQ41TmuOI4 ufjQcw//cTtdxQXThzApPoYqhsnPaETh3Opb199zABpa1v2MLwr/ARFjKzt7jeQs 447rMRSkQqlqB2iTgd8cALENKvXvmATogqRprN/zEnnWTRReAwFwCvL4t8a6Xnv8 kiaI3iTFciGLwFFZ+ele2fqgYHCn5EdHVlqxVPpZEZ79y8RiB6l7ywHueaW4aybS Gp0JXswA4/+MW481prHbOGu3qEl3eaUgWSCkO2Zx3aMIhsoZdqFN+qY9nshST2+p AhMK7gl/LP6ecj3Uu0fkn5Ri1LtA2/yoBeoeIXPGaMxBJziZHP8YrlLRT/Z59bhW S2VkWKHqQZots1X8Ek1DlrgpAVcPR8134RuNKB8ls1kQL1vzEb+/sBMxacacIQ09 F4v3z/9172cC3xeayhwf3hVSS8Ow5PPIYQBoK40e1i/amYpndZI7viZcQ3gM9DFg 6rU7SxyAcVPkdW0VL+UFNshFjJ8qfikc7a6dbtRf2hIYbUm6WaCKuHLRG4BKYnXr sJr0w05WOz4qZNsgzIFwAO3EJwuOc//KaXiB+GGcZEHY0q8BJqAQsX7C64o/UIeQ yFhMp5TosT0/1Gn9soFGgAT/PzFThsak73gKVZ4wOgb21HDrJqTTHgJrtiU3YzPM 7w3A7mdJuwzkDm2j+g40Qq53dUkDkkJFkdTLu7yFgKZ8kEnVGVs= =FssW -----END PGP SIGNATURE----- --IPc3Hq05fggeF2LONzgb4utJx2bIqwqOy--