Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 21 Jan 2022 13:28:01 +0100
From:   Claudio Imbrenda <imbrenda@linux.ibm.com>
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Cc:     Janosch Frank <frankja@linux.ibm.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [RFC PATCH v1 04/10] KVM: s390: selftests: Test TEST PROTECTION
 emulation
Message-ID: <20220121132801.12ea572f@p-imbrenda>
In-Reply-To: <06422388-8389-6954-00c7-7b582b4cf1bb@linux.ibm.com>
References: <20220118095210.1651483-1-scgl@linux.ibm.com>
        <20220118095210.1651483-5-scgl@linux.ibm.com>
        <c5ce5d0b-444b-ba33-a670-3bd3893af475@linux.ibm.com>
        <06422388-8389-6954-00c7-7b582b4cf1bb@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 21 Jan 2022 12:03:20 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

[...]

> >> +
> >> +static int set_storage_key(void *addr, uint8_t key)
> >> +{
> >> +=C2=A0=C2=A0=C2=A0 int not_mapped =3D 0;
> >> + =20
> >=20
> > Maybe add a short comment:
> > Check if address is mapped via lra and set the storage key if it is.
> >  =20
> >> +=C2=A0=C2=A0=C2=A0 asm volatile (
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 "lra=C2=A0=C2=A0=C2=A0 %[addr], 0(0,%[addr])\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "=C2=A0=C2=A0=C2=A0 jz=C2=
=A0=C2=A0=C2=A0 0f\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "=C2=A0=C2=A0=C2=A0 llill=
=C2=A0=C2=A0=C2=A0 %[not_mapped],1\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "=C2=A0=C2=A0=C2=A0 j=C2=
=A0=C2=A0=C2=A0 1f\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "0:=C2=A0=C2=A0=C2=A0 sske=
=C2=A0=C2=A0=C2=A0 %[key], %[addr]\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "1:"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : [addr] "+&a" (addr), [no=
t_mapped] "+r" (not_mapped) =20
> >=20
> > Shouldn't this be a "=3Dr" instead of a "+r" for not_mapped? =20
>=20
> I don't think so. We only write to it on one code path and the compiler m=
ustn't conclude
> that it can remove the =3D 0 assignment because the value gets overwritte=
n anyway.
>=20
> Initially I tried to implement the function like this:
>=20
> static int set_storage_key(void *addr, uint8_t key)
> {
>         asm goto ("lra  %[addr], 0(0,%[addr])\n\t"
>                   "jnz  %l[not_mapped]\n\t"
>                   "sske %[key], %[addr]\n"
>                 : [addr] "+&a" (addr)
>                 : [key] "r" (key)
>                 : "cc", "memory"
>                 : not_mapped
>         );
>         return 0;
> not_mapped:
>         return -1;
> }
>=20
> Which I think is nicer, but the compiler just optimized that completely a=
way.
> I have no clue why it (thinks it) is allowed to do that.
>=20
> >  =20
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : [key] "r" (key)
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : "cc"
> >> +=C2=A0=C2=A0=C2=A0 );
> >> +=C2=A0=C2=A0=C2=A0 return -not_mapped;
> >> +}
> >> +
> >> +enum permission {
> >> +=C2=A0=C2=A0=C2=A0 READ_WRITE =3D 0,
> >> +=C2=A0=C2=A0=C2=A0 READ =3D 1,
> >> +=C2=A0=C2=A0=C2=A0 NONE =3D 2,
> >> +=C2=A0=C2=A0=C2=A0 UNAVAILABLE =3D 3, =20
> >=20
> > TRANSLATION_NA ?
> > I'm not completely happy with these names but I've yet to come up with =
a better naming scheme here. =20
>=20
> Mentioning translation is a good idea. Don't think there is any harm in u=
sing
> TRANSLATION_NOT_AVAILABLE or TRANSLATION_UNAVAILABLE.

it's too long, it actually makes the code harder to read when used

maybe consider something like TRANSL_UNAVAIL as well

> >  =20
> >> +};
> >> +
> >> +static enum permission test_protection(void *addr, uint8_t key)
> >> +{
> >> +=C2=A0=C2=A0=C2=A0 uint64_t mask;
> >> +
> >> +=C2=A0=C2=A0=C2=A0 asm volatile (
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 "tprot=C2=A0=C2=A0=C2=A0 %[addr], 0(%[key])\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "=C2=A0=C2=A0=C2=A0 ipm=C2=
=A0=C2=A0=C2=A0 %[mask]\n"
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : [mask] "=3Dr" (mask)
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : [addr] "Q" (*(char *)add=
r),
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 [key] "a" (key)
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 : "cc"
> >> +=C2=A0=C2=A0=C2=A0 );
> >> +
> >> +=C2=A0=C2=A0=C2=A0 return (enum permission)mask >> 28; =20
> >=20
> > You could replace the shift with the "srl" that we normally do. =20
>=20
> I prefer keeping the asm as small as possible, C is just so much easier t=
o understand.

we use srl everywhere, but I agree that explicitly using C makes it
less obscure. and in the end the compiler should generate the same
instructions anyway.

my only comment about the above code is that you are casting the
uint64_t to enum permission _and then_ shifting. _technically_ it
should still work (enums are just ints), but I think it would
look cleaner if you write

	return (enum permission)(mask >> 28);

>=20
> [...]
>=20
> > It's __really__ hard to understand this since the state is changed both=
 by the guest and host. Please add comments to this and maybe also add some=
 to the test struct explaining why you expect the results for each test.
> >  =20
>=20
> I think I'll concentrate the comments at the tests array so we have one l=
ocation
> that lays out the complete logic and then one only has to check if the gu=
est
> and host match up with that, respectively, instead of having to model the=
ir interaction
> in ones head.
>=20
> I'll incorporate your other feedback, too.
>=20
> Thanks!