[v1] s390x: skey related fixes, cleanups, and memory device preparations

[PATCH v1 02/12] s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE

Posted by David Hildenbrand 4 years, 6 months ago

The last bit has to be ignored.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/tcg/mem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index e0befd0f03..3c0820dd74 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2210,7 +2210,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
         skeyclass = S390_SKEYS_GET_CLASS(ss);
     }
 
-    key = (uint8_t) r1;
+    key = r1 & 0xfe;
     skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
    /*
     * As we can only flush by virtual address and not all the entries
-- 
2.31.1

Re: [PATCH v1 02/12] s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE

Posted by Thomas Huth 4 years, 6 months ago

On 05/08/2021 17.27, David Hildenbrand wrote:
> The last bit has to be ignored.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/tcg/mem_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index e0befd0f03..3c0820dd74 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -2210,7 +2210,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>           skeyclass = S390_SKEYS_GET_CLASS(ss);
>       }
>   
> -    key = (uint8_t) r1;
> +    key = r1 & 0xfe;

I'm not sure about this one ... could you cite a sentence in the PoP where 
this is declared? For me it rather sounds like SSKE always sets the whole 
storage key...

  Thomas

Re: [PATCH v1 02/12] s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE

Posted by Thomas Huth 4 years, 6 months ago

On 06/08/2021 08.19, Thomas Huth wrote:
> On 05/08/2021 17.27, David Hildenbrand wrote:
>> The last bit has to be ignored.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   target/s390x/tcg/mem_helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>> index e0befd0f03..3c0820dd74 100644
>> --- a/target/s390x/tcg/mem_helper.c
>> +++ b/target/s390x/tcg/mem_helper.c
>> @@ -2210,7 +2210,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, 
>> uint64_t r2)
>>           skeyclass = S390_SKEYS_GET_CLASS(ss);
>>       }
>> -    key = (uint8_t) r1;
>> +    key = r1 & 0xfe;
> 
> I'm not sure about this one ... could you cite a sentence in the PoP where 
> this is declared? For me it rather sounds like SSKE always sets the whole 
> storage key...

Ah, never mind, I missed that the rightmost bit is undefined and thus this 
is likely ok. Did you check this on a real CPU, though?

  Thomas

Re: [PATCH v1 02/12] s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE

Posted by David Hildenbrand 4 years, 6 months ago

On 06.08.21 08:25, Thomas Huth wrote:
> On 06/08/2021 08.19, Thomas Huth wrote:
>> On 05/08/2021 17.27, David Hildenbrand wrote:
>>> The last bit has to be ignored.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    target/s390x/tcg/mem_helper.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>>> index e0befd0f03..3c0820dd74 100644
>>> --- a/target/s390x/tcg/mem_helper.c
>>> +++ b/target/s390x/tcg/mem_helper.c
>>> @@ -2210,7 +2210,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1,
>>> uint64_t r2)
>>>            skeyclass = S390_SKEYS_GET_CLASS(ss);
>>>        }
>>> -    key = (uint8_t) r1;
>>> +    key = r1 & 0xfe;
>>
>> I'm not sure about this one ... could you cite a sentence in the PoP where
>> this is declared? For me it rather sounds like SSKE always sets the whole
>> storage key...
> 
> Ah, never mind, I missed that the rightmost bit is undefined and thus this
> is likely ok. Did you check this on a real CPU, though?

The storage key is always 7 bit, never 8 bit:

10-134:

"The new seven-bit storage-key value, or selected bits
thereof, is obtained from bit positions 56-62 of general
register R1 ."

Similarly, ISKE gives you only 7 bit:

10-31:

"The seven-bit storage key is inserted in bit positions
56-62 of general register R 1 , and bit 63 is set to zero."


Right now we could SSKE 8 bit and extract again via ISKE 8 bit, which is 
against the architecture definition.


Also have a look at arch/s390/kvm/kvm-s390.c:kvm_s390_set_skeys() where 
we reject setting a key if the last bit is set, because storage keys are 
7 bit.

-- 
Thanks,

David / dhildenb