| Message ID | 20210805152804.100333-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: skey related fixes, cleanups, and memory device preparations | expand |
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
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
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.
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
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(-)