On 13.03.2018 19:32, Thomas Huth wrote:
> On 13.03.2018 13:01, Janosch Frank wrote:
>> Storage keys are not used by Linux anymore, so let's show them some
>> love and test if the basics still work.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>> ---
>> lib/s390x/asm/mem.h | 61 ++++++++++++++++++++++++++++++++++
>> s390x/Makefile | 1 +
>> s390x/skey.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> s390x/unittests.cfg | 3 ++
>> 4 files changed, 159 insertions(+)
>> create mode 100644 lib/s390x/asm/mem.h
>> create mode 100644 s390x/skey.c
>>
>> diff --git a/lib/s390x/asm/mem.h b/lib/s390x/asm/mem.h
>> new file mode 100644
>> index 0000000..28772b0
>> --- /dev/null
>> +++ b/lib/s390x/asm/mem.h
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Physical memory management related functions and definitions.
>> + *
>> + * Copyright IBM Corp. 2017
>
> 2018 ?
Sure
>
>> + * Author(s): Janosch Frank <frankja@xxxxxxxxxx>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +#ifndef _ASM_S390_MEM_H
>> +#define _ASM_S390_MEM_H
>> +
>> +union skey {
>> + struct {
>> + uint8_t acc : 4;
>> + uint8_t fp : 1;
>> + uint8_t rf : 1;
>> + uint8_t ch : 1;
>> + uint8_t pad : 1;
>> + } str;
>> + uint8_t val;
>> +};
>> +
>> +static inline void set_storage_key(unsigned long addr,
>> + unsigned char skey,
>> + int nq)
>
> I did not spot any occurance of set_storage_key(..., true) in this
> patch, so do you really need this parameter?
No, I just added nq setting because I was at it, if you want I can
remove it.
>
>> +{
>> + if (nq && test_facility(14))
>
> I think you could simply drop the test_facility check here since the bit
> is ignored accoring to the PoP if the facility is not installed.
>
>> + asm volatile(".insn rrf,0xb22b0000,%0,%1,8,0"
>> + : : "d" (skey), "a" (addr));
>> + else
>> + asm volatile("sske %0,%1" : : "d" (skey), "a" (addr));
>> +}
>> +
>> +static inline unsigned long set_storage_key_mb(unsigned long addr,
>> + unsigned char skey)
>> +{
>> + if (!test_facility(8))
>> + return 0;
>
> You check that already in test_set_mb() ... so I'd either remove this
> if-statement or turn it into a assert(test_facility(8)).
Assert it is.
>
>> + /* As we only have one cpu and no concurrent skey changes,
>> + * we're able to use the non-quescing option (if available) to
>> + * speed things up a little.
>> + */
>> + if (test_facility(14))
>
> Again, no need to explicitly check for that facility here - the bit is
> ignored if the facility is not available.
Did not know that.
>
>> + asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],9,0"
>> + : [addr] "+a" (addr) : [skey] "d" (skey));
>> + else
>> + asm volatile(".insn rrf,0xb22b0000,%[skey],%[addr],1,0"
>> + : [addr] "+a" (addr) : [skey] "d" (skey));
>> + return addr;
>> +}
>> +
>> +static inline unsigned char get_storage_key(unsigned long addr)
>> +{
>> + unsigned char skey;
>> +
>> + asm volatile("iske %0,%1" : "=d" (skey) : "a" (addr));
>> + return skey;
>> +}
>> +#endif
>
> The other parts of the patch look fine to me.
>
> Thomas
>
Attachment:
signature.asc
Description: OpenPGP digital signature
