For RRBE, SSKE, and ISKE, we're dealing with real addresses, so we have to
convert to an absolute address first.

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

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 3c0820dd74..dd506d8d17 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2177,6 +2177,7 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
     uint64_t addr = wrap_address(env, r2);
     uint8_t key;
 
+    addr = mmu_real2abs(env, addr);
     if (addr > ms->ram_size) {
         return 0;
     }
@@ -2201,6 +2202,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
     uint64_t addr = wrap_address(env, r2);
     uint8_t key;
 
+    addr = mmu_real2abs(env, addr);
     if (addr > ms->ram_size) {
         return;
     }
@@ -2228,6 +2230,7 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
     static S390SKeysClass *skeyclass;
     uint8_t re, key;
 
+    addr = mmu_real2abs(env, addr);
     if (addr > ms->ram_size) {
         return 0;
     }
-- 
2.31.1

On 05/08/2021 17.27, David Hildenbrand wrote:
> For RRBE, SSKE, and ISKE, we're dealing with real addresses, so we have to
> convert to an absolute address first.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/tcg/mem_helper.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index 3c0820dd74..dd506d8d17 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -2177,6 +2177,7 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
>       uint64_t addr = wrap_address(env, r2);
>       uint8_t key;
>   
> +    addr = mmu_real2abs(env, addr);

Ack.

>       if (addr > ms->ram_size) {
>           return 0;
>       }
> @@ -2201,6 +2202,7 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>       uint64_t addr = wrap_address(env, r2);
>       uint8_t key;
>   
> +    addr = mmu_real2abs(env, addr);

According to the PoP:

"When the enhanced-DAT facility 1 is not installed, or
  when the facility is installed but the multiple-block
  control is zero, general register R 2 contains a real
  address. When the enhanced-DAT facility 1 is
  installed and the multiple-block control is one, gen-
  eral register R 2 contains an absolute address."

Don't we have to take that into consideration here, too?

>       if (addr > ms->ram_size) {
>           return;
>       }
> @@ -2228,6 +2230,7 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
>       static S390SKeysClass *skeyclass;
>       uint8_t re, key;
>   
> +    addr = mmu_real2abs(env, addr);
>       if (addr > ms->ram_size) {
>           return 0;
>       }

Ack for rrbe.

  Thomas

> 
> According to the PoP:
> 
> "When the enhanced-DAT facility 1 is not installed, or
>    when the facility is installed but the multiple-block
>    control is zero, general register R 2 contains a real
>    address. When the enhanced-DAT facility 1 is
>    installed and the multiple-block control is one, gen-
>    eral register R 2 contains an absolute address."
> 
> Don't we have to take that into consideration here, too?

We don't support EDAT1 a.k.a. huge pages yet. If we ever do, we have to 
further extend this code.

Thanks!

-- 
Thanks,

David / dhildenb

On 06/08/2021 08.52, David Hildenbrand wrote:
>>
>> According to the PoP:
>>
>> "When the enhanced-DAT facility 1 is not installed, or
>>    when the facility is installed but the multiple-block
>>    control is zero, general register R 2 contains a real
>>    address. When the enhanced-DAT facility 1 is
>>    installed and the multiple-block control is one, gen-
>>    eral register R 2 contains an absolute address."
>>
>> Don't we have to take that into consideration here, too?
> 
> We don't support EDAT1 a.k.a. huge pages yet. If we ever do, we have to 
> further extend this code.

Ok, then maybe add a comment or assert() to make sure that we don't forget?

  Thomas

On 06.08.21 09:11, Thomas Huth wrote:
> On 06/08/2021 08.52, David Hildenbrand wrote:
>>>
>>> According to the PoP:
>>>
>>> "When the enhanced-DAT facility 1 is not installed, or
>>>     when the facility is installed but the multiple-block
>>>     control is zero, general register R 2 contains a real
>>>     address. When the enhanced-DAT facility 1 is
>>>     installed and the multiple-block control is one, gen-
>>>     eral register R 2 contains an absolute address."
>>>
>>> Don't we have to take that into consideration here, too?
>>
>> We don't support EDAT1 a.k.a. huge pages yet. If we ever do, we have to
>> further extend this code.
> 
> Ok, then maybe add a comment or assert() to make sure that we don't forget?

Well, we'll need modifications and extensions all over the place to 
support EDAT1, so I'm not sure this will really help ... we'll have to 
carefully scan the PoP either way.

-- 
Thanks,

David / dhildenb

On Fri, Aug 06 2021, David Hildenbrand <david@redhat.com> wrote:

> On 06.08.21 09:11, Thomas Huth wrote:
>> On 06/08/2021 08.52, David Hildenbrand wrote:
>>>>
>>>> According to the PoP:
>>>>
>>>> "When the enhanced-DAT facility 1 is not installed, or
>>>>     when the facility is installed but the multiple-block
>>>>     control is zero, general register R 2 contains a real
>>>>     address. When the enhanced-DAT facility 1 is
>>>>     installed and the multiple-block control is one, gen-
>>>>     eral register R 2 contains an absolute address."
>>>>
>>>> Don't we have to take that into consideration here, too?
>>>
>>> We don't support EDAT1 a.k.a. huge pages yet. If we ever do, we have to
>>> further extend this code.
>> 
>> Ok, then maybe add a comment or assert() to make sure that we don't forget?
>
> Well, we'll need modifications and extensions all over the place to 
> support EDAT1, so I'm not sure this will really help ... we'll have to 
> carefully scan the PoP either way.

Something like
/* always real address, as long as we don't implement EDAT1 */
would still be useful, I think.

On 06.08.21 13:25, Cornelia Huck wrote:
> On Fri, Aug 06 2021, David Hildenbrand <david@redhat.com> wrote:
> 
>> On 06.08.21 09:11, Thomas Huth wrote:
>>> On 06/08/2021 08.52, David Hildenbrand wrote:
>>>>>
>>>>> According to the PoP:
>>>>>
>>>>> "When the enhanced-DAT facility 1 is not installed, or
>>>>>      when the facility is installed but the multiple-block
>>>>>      control is zero, general register R 2 contains a real
>>>>>      address. When the enhanced-DAT facility 1 is
>>>>>      installed and the multiple-block control is one, gen-
>>>>>      eral register R 2 contains an absolute address."
>>>>>
>>>>> Don't we have to take that into consideration here, too?
>>>>
>>>> We don't support EDAT1 a.k.a. huge pages yet. If we ever do, we have to
>>>> further extend this code.
>>>
>>> Ok, then maybe add a comment or assert() to make sure that we don't forget?
>>
>> Well, we'll need modifications and extensions all over the place to
>> support EDAT1, so I'm not sure this will really help ... we'll have to
>> carefully scan the PoP either way.
> 
> Something like
> /* always real address, as long as we don't implement EDAT1 */
> would still be useful, I think.

I am not a friend of describing what to be done with additional CPU 
features. We have the PoP for that: just imagine you read an old version 
of the PoP, the code as is would make perfect sense even without ever 
knowing what EDAT1 is -- and you can verify that the code is correct.

For now I added to the patch description:

"
In the future, when adding EDAT1 support, we'll have to pay attention to
SSKE handling, as we'll be dealing with absolute addresses when the
multiple-block control is one.
"

-- 
Thanks,

David / dhildenb