[PATCH v1 00/12] s390x: skey related fixes, cleanups, and memory device preparations

[PATCH v1 00/12] s390x: skey related fixes, cleanups, and memory device preparations

[David Hildenbrand]

This series fixes multiple TCG issues related to storage key instructions,
fixes some TCG issues related to LOAD REAL ADDRESS and skeys, implements
lazy skey enablement under TCG, and prepares the whole skey infrastructure
for dealing with addresses beyond initial RAM (e.g., memory devices like
virtio-mem). Along, some cleanups.

To prepare for memory devices / memory hotplug, my goal was to get rid of
as many ms->ram_size users as possible. Unfortunately, I stumbled over
many other things on the way :) The remaining s390x users of ms->ram_size
are:

a) hw/s390x/ipl.c: loading the FW. Won't need adjustment.
b) hw/s390x/s390-skeys.c: allocating the array for storage keys. Will need
   adjustment for memory devices.
c) hw/s390x/s390-stattrib-kvm.c: will need adjustments in the future when
   using memory devices with CMM.
d) hw/s390x/s390-virtio-ccw.c: fixing up / handling initital ram. Won't
   need adjustment.
e) hw/s390x/sclp.c: same as d)

Especially patch 9-11 also affect KVM. The remaining ones mostly only
affect TCG.

Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Janosch Frank <frankja@linux.ibm.com>
Cc: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: Jason J. Herne <jjherne@linux.ibm.com>
Cc: qemu-s390x@nongnu.org

David Hildenbrand (12):
  s390x/tcg: wrap address for RRBE
  s390x/tcg: fix ignoring bit 63 when setting the storage key in SSKE
  s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE
  s390x/tcg: check for addressing exceptions for for RRBE, SSKE and ISKE
  s390x/mmu_helper: no need to pass access type to mmu_translate_asce()
  s390x/mmu_helper: fixup mmu_translate() documentation
  s390x/mmu_helper: move address validation into mmu_translate*()
  s390x/mmu_helper: avoid setting the storage key if nothing changed
  hw/s390x/s390-skeys: use memory mapping to detect which storage keys
    to migrate
  hw/s390x/s390-skeys: use memory mapping to detect which storage keys
    to dump
  hw/s390x/s390-skeys: check if an address is valid before dumping the
    key
  hw/s390x/s390-skeys: lazy storage key enablement under TCG

 hw/s390x/s390-skeys.c           | 183 +++++++++++++++++++++-----------
 include/hw/s390x/storage-keys.h |  63 +++++++++++
 target/s390x/helper.h           |   6 +-
 target/s390x/mmu_helper.c       |  70 ++++++++----
 target/s390x/s390x-internal.h   |   1 +
 target/s390x/tcg/excp_helper.c  |  13 ---
 target/s390x/tcg/mem_helper.c   |  54 +++++++---
 7 files changed, 277 insertions(+), 113 deletions(-)

-- 
2.31.1

[PATCH v1 01/12] s390x/tcg: wrap address for RRBE

[David Hildenbrand]

Let's wrap the address just like for SSKE and ISKE.

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

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 21a4de4067..e0befd0f03 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2223,11 +2223,12 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
+    uint64_t addr = wrap_address(env, r2);
     static S390SKeysState *ss;
     static S390SKeysClass *skeyclass;
     uint8_t re, key;
 
-    if (r2 > ms->ram_size) {
+    if (addr > ms->ram_size) {
         return 0;
     }
 
@@ -2236,14 +2237,14 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
         skeyclass = S390_SKEYS_GET_CLASS(ss);
     }
 
-    if (skeyclass->get_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
+    if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
         return 0;
     }
 
     re = key & (SK_R | SK_C);
     key &= ~SK_R;
 
-    if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
+    if (skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
         return 0;
     }
    /*
-- 
2.31.1

Re: [PATCH v1 01/12] s390x/tcg: wrap address for RRBE

[Thomas Huth]

On 05/08/2021 17.27, David Hildenbrand wrote:
> Let's wrap the address just like for SSKE and ISKE.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/tcg/mem_helper.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index 21a4de4067..e0befd0f03 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -2223,11 +2223,12 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>   uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
>   {
>       MachineState *ms = MACHINE(qdev_get_machine());
> +    uint64_t addr = wrap_address(env, r2);
>       static S390SKeysState *ss;
>       static S390SKeysClass *skeyclass;
>       uint8_t re, key;
>   
> -    if (r2 > ms->ram_size) {
> +    if (addr > ms->ram_size) {
>           return 0;
>       }
>   
> @@ -2236,14 +2237,14 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
>           skeyclass = S390_SKEYS_GET_CLASS(ss);
>       }
>   
> -    if (skeyclass->get_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
> +    if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
>           return 0;
>       }
>   
>       re = key & (SK_R | SK_C);
>       key &= ~SK_R;
>   
> -    if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
> +    if (skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
>           return 0;
>       }

That's certainly the right thing to do!

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[David Hildenbrand]

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

[Thomas Huth]

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

[Thomas Huth]

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

[David Hildenbrand]

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

[PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[David Hildenbrand]

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

Re: [PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[Thomas Huth]

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

Re: [PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[David Hildenbrand]

> 
> 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

Re: [PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[Thomas Huth]

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

Re: [PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[David Hildenbrand]

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

Re: [PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[Cornelia Huck]

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.

Re: [PATCH v1 03/12] s390x/tcg: convert real to absolute address for RRBE, SSKE and ISKE

[David Hildenbrand]

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

[PATCH v1 04/12] s390x/tcg: check for addressing exceptions for for RRBE, SSKE and ISKE

[David Hildenbrand]

Let's replace the ram_size check by a proper physical address space
check (for example, to prepare for memory hotplug), trigger addressing
exceptions and trace the return value of the storage key getter/setter.

Provide an helper mmu_absolute_addr_valid() to be used in other context
soon. Always test for "read" instead of "write" as we are not actually
modifying the page itself.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.h         |  6 +++---
 target/s390x/mmu_helper.c     |  8 ++++++++
 target/s390x/s390x-internal.h |  1 +
 target/s390x/tcg/mem_helper.c | 37 +++++++++++++++++++++++------------
 4 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 6215ca00bc..271b081e8c 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -336,9 +336,9 @@ DEF_HELPER_FLAGS_4(stctl, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_4(stctg, TCG_CALL_NO_WG, void, env, i32, i64, i32)
 DEF_HELPER_FLAGS_2(testblock, TCG_CALL_NO_WG, i32, env, i64)
 DEF_HELPER_FLAGS_3(tprot, TCG_CALL_NO_WG, i32, env, i64, i64)
-DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64)
-DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64)
-DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64)
+DEF_HELPER_2(iske, i64, env, i64)
+DEF_HELPER_3(sske, void, env, i64, i64)
+DEF_HELPER_2(rrbe, i32, env, i64)
 DEF_HELPER_4(mvcs, i32, env, i64, i64, i64)
 DEF_HELPER_4(mvcp, i32, env, i64, i64, i64)
 DEF_HELPER_4(sigp, i32, env, i64, i32, i32)
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index d779a9fc51..0620b1803e 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -94,6 +94,14 @@ target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr)
     return raddr;
 }
 
+bool mmu_absolute_addr_valid(target_ulong addr, bool is_write)
+{
+    return address_space_access_valid(&address_space_memory,
+                                      addr & TARGET_PAGE_MASK,
+                                      TARGET_PAGE_SIZE, is_write,
+                                      MEMTXATTRS_UNSPECIFIED);
+}
+
 static inline bool read_table_entry(CPUS390XState *env, hwaddr gaddr,
                                     uint64_t *entry)
 {
diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 5506f185e8..d246d26b04 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -373,6 +373,7 @@ void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
 
 
 /* mmu_helper.c */
+bool mmu_absolute_addr_valid(target_ulong addr, bool is_write);
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags, uint64_t *tec);
 int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index dd506d8d17..a84795cfa3 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -28,6 +28,7 @@
 #include "qemu/int128.h"
 #include "qemu/atomic128.h"
 #include "tcg/tcg.h"
+#include "trace.h"
 
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/s390x/storage-keys.h"
@@ -2171,15 +2172,15 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
 /* insert storage key extended */
 uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
     static S390SKeysState *ss;
     static S390SKeysClass *skeyclass;
     uint64_t addr = wrap_address(env, r2);
     uint8_t key;
+    int rc;
 
     addr = mmu_real2abs(env, addr);
-    if (addr > ms->ram_size) {
-        return 0;
+    if (!mmu_absolute_addr_valid(addr, false)) {
+        trigger_pgm_exception(env, PGM_ADDRESSING);
     }
 
     if (unlikely(!ss)) {
@@ -2187,7 +2188,9 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
         skeyclass = S390_SKEYS_GET_CLASS(ss);
     }
 
-    if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
+    rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (rc) {
+        trace_get_skeys_nonzero(rc);
         return 0;
     }
     return key;
@@ -2196,15 +2199,15 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 /* set storage key extended */
 void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
     static S390SKeysState *ss;
     static S390SKeysClass *skeyclass;
     uint64_t addr = wrap_address(env, r2);
     uint8_t key;
+    int rc;
 
     addr = mmu_real2abs(env, addr);
-    if (addr > ms->ram_size) {
-        return;
+    if (!mmu_absolute_addr_valid(addr, false)) {
+        trigger_pgm_exception(env, PGM_ADDRESSING);
     }
 
     if (unlikely(!ss)) {
@@ -2213,7 +2216,11 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
     }
 
     key = r1 & 0xfe;
-    skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (rc) {
+        trace_set_skeys_nonzero(rc);
+        return 0;
+    }
    /*
     * As we can only flush by virtual address and not all the entries
     * that point to a physical address we have to flush the whole TLB.
@@ -2224,15 +2231,15 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 /* reset reference bit extended */
 uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
     uint64_t addr = wrap_address(env, r2);
     static S390SKeysState *ss;
     static S390SKeysClass *skeyclass;
     uint8_t re, key;
+    int rc;
 
     addr = mmu_real2abs(env, addr);
-    if (addr > ms->ram_size) {
-        return 0;
+    if (!mmu_absolute_addr_valid(addr, false)) {
+        trigger_pgm_exception(env, PGM_ADDRESSING);
     }
 
     if (unlikely(!ss)) {
@@ -2240,14 +2247,18 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
         skeyclass = S390_SKEYS_GET_CLASS(ss);
     }
 
-    if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
+    rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (rc) {
+        trace_get_skeys_nonzero(rc);
         return 0;
     }
 
     re = key & (SK_R | SK_C);
     key &= ~SK_R;
 
-    if (skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
+    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (rc) {
+        trace_set_skeys_nonzero(rc);
         return 0;
     }
    /*
-- 
2.31.1

Re: [PATCH v1 04/12] s390x/tcg: check for addressing exceptions for for RRBE, SSKE and ISKE

[David Hildenbrand]

>   void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>   {
> -    MachineState *ms = MACHINE(qdev_get_machine());
>       static S390SKeysState *ss;
>       static S390SKeysClass *skeyclass;
>       uint64_t addr = wrap_address(env, r2);
>       uint8_t key;
> +    int rc;
>   
>       addr = mmu_real2abs(env, addr);
> -    if (addr > ms->ram_size) {
> -        return;
> +    if (!mmu_absolute_addr_valid(addr, false)) {
> +        trigger_pgm_exception(env, PGM_ADDRESSING);
>       }
>   
>       if (unlikely(!ss)) {
> @@ -2213,7 +2216,11 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
>       }
>   
>       key = r1 & 0xfe;
> -    skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
> +    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
> +    if (rc) {
> +        trace_set_skeys_nonzero(rc);
> +        return 0;

^ this has to go.


-- 
Thanks,

David / dhildenb

[PATCH v1 05/12] s390x/mmu_helper: no need to pass access type to mmu_translate_asce()

[David Hildenbrand]

The access type is unused.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 0620b1803e..167f1b1455 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -125,7 +125,7 @@ static inline bool read_table_entry(CPUS390XState *env, hwaddr gaddr,
 
 static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
                               uint64_t asc, uint64_t asce, target_ulong *raddr,
-                              int *flags, int rw)
+                              int *flags)
 {
     const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
                        s390_has_feat(S390_FEAT_EDAT);
@@ -428,7 +428,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
     }
 
     /* perform the DAT translation */
-    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags, rw);
+    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags);
     if (unlikely(r)) {
         return r;
     }
-- 
2.31.1

Re: [PATCH v1 05/12] s390x/mmu_helper: no need to pass access type to mmu_translate_asce()

[Thomas Huth]

On 05/08/2021 17.27, David Hildenbrand wrote:
> The access type is unused.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/mmu_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 0620b1803e..167f1b1455 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -125,7 +125,7 @@ static inline bool read_table_entry(CPUS390XState *env, hwaddr gaddr,
>   
>   static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>                                 uint64_t asc, uint64_t asce, target_ulong *raddr,
> -                              int *flags, int rw)
> +                              int *flags)
>   {
>       const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
>                          s390_has_feat(S390_FEAT_EDAT);
> @@ -428,7 +428,7 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>       }
>   
>       /* perform the DAT translation */
> -    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags, rw);
> +    r = mmu_translate_asce(env, vaddr, asc, asce, raddr, flags);
>       if (unlikely(r)) {
>           return r;
>       }
> 

Fixes: 81d7e3bc45 ("s390x/mmu: Inject DAT exceptions from a single place")

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v1 05/12] s390x/mmu_helper: no need to pass access type to mmu_translate_asce()

[David Hildenbrand]

On 06.08.21 09:30, Thomas Huth wrote:
> Fixes: 81d7e3bc45 ("s390x/mmu: Inject DAT exceptions from a single place")
> 
> Reviewed-by: Thomas Huth<thuth@redhat.com>

I'm usually a bit careful with Fixes tags if we're not fixing real BUGs. 
At least in the kernel people will really complain if you do that 
(because it might result in backports of patches that are absolutely not 
worth backporting and makes actual bugfixes harder to track). But maybe 
QEMUs policy is different, so I can add it.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 05/12] s390x/mmu_helper: no need to pass access type to mmu_translate_asce()

[Thomas Huth]

On 06/08/2021 09.34, David Hildenbrand wrote:
> On 06.08.21 09:30, Thomas Huth wrote:
>> Fixes: 81d7e3bc45 ("s390x/mmu: Inject DAT exceptions from a single place")
>>
>> Reviewed-by: Thomas Huth<thuth@redhat.com>
> 
> I'm usually a bit careful with Fixes tags if we're not fixing real BUGs. At 
> least in the kernel people will really complain if you do that (because it 
> might result in backports of patches that are absolutely not worth 
> backporting and makes actual bugfixes harder to track). But maybe QEMUs 
> policy is different, so I can add it.

Fair point. Maybe simply mention the commit id in the patch description?

  Thomas

Re: [PATCH v1 05/12] s390x/mmu_helper: no need to pass access type to mmu_translate_asce()

[David Hildenbrand]

On 06.08.21 09:36, Thomas Huth wrote:
> On 06/08/2021 09.34, David Hildenbrand wrote:
>> On 06.08.21 09:30, Thomas Huth wrote:
>>> Fixes: 81d7e3bc45 ("s390x/mmu: Inject DAT exceptions from a single place")
>>>
>>> Reviewed-by: Thomas Huth<thuth@redhat.com>
>>
>> I'm usually a bit careful with Fixes tags if we're not fixing real BUGs. At
>> least in the kernel people will really complain if you do that (because it
>> might result in backports of patches that are absolutely not worth
>> backporting and makes actual bugfixes harder to track). But maybe QEMUs
>> policy is different, so I can add it.
> 
> Fair point. Maybe simply mention the commit id in the patch description?

Agreed, thanks!


-- 
Thanks,

David / dhildenb

[PATCH v1 06/12] s390x/mmu_helper: fixup mmu_translate() documentation

[David Hildenbrand]

Looks like we forgot to adjust documentation of one parameter.

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

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 167f1b1455..ca25dadb5b 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -374,7 +374,8 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
  * @param asc    address space control (one of the PSW_ASC_* modes)
  * @param raddr  the translated address is stored to this pointer
  * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
- * @param exc    true = inject a program check if a fault occurred
+ * @param tec    the translation exception code if stored to this pointer if
+ *               there is an exception to raise
  * @return       0 = success, != 0, the exception to raise
  */
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
-- 
2.31.1

Re: [PATCH v1 06/12] s390x/mmu_helper: fixup mmu_translate() documentation

[Thomas Huth]

On 05/08/2021 17.27, David Hildenbrand wrote:
> Looks like we forgot to adjust documentation of one parameter.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/mmu_helper.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 167f1b1455..ca25dadb5b 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -374,7 +374,8 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>    * @param asc    address space control (one of the PSW_ASC_* modes)
>    * @param raddr  the translated address is stored to this pointer
>    * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
> - * @param exc    true = inject a program check if a fault occurred
> + * @param tec    the translation exception code if stored to this pointer if
> + *               there is an exception to raise
>    * @return       0 = success, != 0, the exception to raise
>    */
>   int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[David Hildenbrand]

Let's move address validation into mmu_translate() and
mmu_translate_real(). This allows for checking whether an absolute
address is valid before looking up the storage key. We can now get rid of
the ram_size check.

Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
a) We're not supposed to touch storage keys
b) We're not supposed to convert to an absolute address

Let's use a fake, negative MMUAccessType to teach mmu_translate() to
fix that handling and to not perform address validation.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
 target/s390x/tcg/excp_helper.c | 13 ------------
 target/s390x/tcg/mem_helper.c  |  2 +-
 3 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index ca25dadb5b..36ab4e9c81 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
 {
     static S390SKeysClass *skeyclass;
     static S390SKeysState *ss;
-    MachineState *ms = MACHINE(qdev_get_machine());
     uint8_t key;
     int rc;
 
-    if (unlikely(addr >= ms->ram_size)) {
-        return;
-    }
-
+    /*
+     * We excpect to be called with an absolute address that has already been
+     * validated, such that we can reliably use it to lookup the storage key.
+     */
     if (unlikely(!ss)) {
         ss = s390_get_skeys_device();
         skeyclass = S390_SKEYS_GET_CLASS(ss);
@@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
 /**
  * Translate a virtual (logical) address into a physical (absolute) address.
  * @param vaddr  the virtual address
- * @param rw     0 = read, 1 = write, 2 = code fetch
+ * @param rw     0 = read, 1 = write, 2 = code fetch, < 0 = load real address
  * @param asc    address space control (one of the PSW_ASC_* modes)
  * @param raddr  the translated address is stored to this pointer
  * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
@@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
     }
 
 nodat:
-    /* Convert real address -> absolute address */
-    *raddr = mmu_real2abs(env, *raddr);
+    if (rw >= 0) {
+        /* Convert real address -> absolute address */
+        *raddr = mmu_real2abs(env, *raddr);
 
-    mmu_handle_skey(*raddr, rw, flags);
+        if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
+            *tec = 0; /* unused */
+            return PGM_ADDRESSING;
+        }
+
+        mmu_handle_skey(*raddr, rw, flags);
+    }
     return 0;
 }
 
@@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages,
         if (ret) {
             return ret;
         }
-        if (!address_space_access_valid(&address_space_memory, pages[i],
-                                        TARGET_PAGE_SIZE, is_write,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            *tec = 0; /* unused */
-            return PGM_ADDRESSING;
-        }
         addr += TARGET_PAGE_SIZE;
     }
 
@@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
 
     *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
 
+    if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
+        /* unused */
+        *tec = 0;
+        return PGM_ADDRESSING;
+    }
+
     mmu_handle_skey(*addr, rw, flags);
     return 0;
 }
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index a61917d04f..3d6662a53c 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
         g_assert_not_reached();
     }
 
-    /* check out of RAM access */
-    if (!excp &&
-        !address_space_access_valid(&address_space_memory, raddr,
-                                    TARGET_PAGE_SIZE, access_type,
-                                    MEMTXATTRS_UNSPECIFIED)) {
-        MachineState *ms = MACHINE(qdev_get_machine());
-        qemu_log_mask(CPU_LOG_MMU,
-                      "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
-                      __func__, (uint64_t)raddr, (uint64_t)ms->ram_size);
-        excp = PGM_ADDRESSING;
-        tec = 0; /* unused */
-    }
-
     env->tlb_fill_exc = excp;
     env->tlb_fill_tec = tec;
 
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index a84795cfa3..9c1b9c7d06 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2456,7 +2456,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)
         tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
     }
 
-    exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec);
+    exc = mmu_translate(env, addr, -1, asc, &ret, &flags, &tec);
     if (exc) {
         cc = 3;
         ret = exc | 0x80000000;
-- 
2.31.1

Re: [PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[Thomas Huth]

On 05/08/2021 17.27, David Hildenbrand wrote:
> Let's move address validation into mmu_translate() and
> mmu_translate_real(). This allows for checking whether an absolute
> address is valid before looking up the storage key. We can now get rid of
> the ram_size check.
> 
> Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
> a) We're not supposed to touch storage keys
> b) We're not supposed to convert to an absolute address
> 
> Let's use a fake, negative MMUAccessType to teach mmu_translate() to
> fix that handling and to not perform address validation.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
>   target/s390x/tcg/excp_helper.c | 13 ------------
>   target/s390x/tcg/mem_helper.c  |  2 +-
>   3 files changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index ca25dadb5b..36ab4e9c81 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>   {
>       static S390SKeysClass *skeyclass;
>       static S390SKeysState *ss;
> -    MachineState *ms = MACHINE(qdev_get_machine());
>       uint8_t key;
>       int rc;
>   
> -    if (unlikely(addr >= ms->ram_size)) {
> -        return;
> -    }
> -
> +    /*
> +     * We excpect to be called with an absolute address that has already been
> +     * validated, such that we can reliably use it to lookup the storage key.
> +     */
>       if (unlikely(!ss)) {
>           ss = s390_get_skeys_device();
>           skeyclass = S390_SKEYS_GET_CLASS(ss);
> @@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>   /**
>    * Translate a virtual (logical) address into a physical (absolute) address.
>    * @param vaddr  the virtual address
> - * @param rw     0 = read, 1 = write, 2 = code fetch
> + * @param rw     0 = read, 1 = write, 2 = code fetch, < 0 = load real address
>    * @param asc    address space control (one of the PSW_ASC_* modes)
>    * @param raddr  the translated address is stored to this pointer
>    * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
> @@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>       }
>   
>   nodat:
> -    /* Convert real address -> absolute address */
> -    *raddr = mmu_real2abs(env, *raddr);
> +    if (rw >= 0) {
> +        /* Convert real address -> absolute address */
> +        *raddr = mmu_real2abs(env, *raddr);
>   
> -    mmu_handle_skey(*raddr, rw, flags);
> +        if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
> +            *tec = 0; /* unused */
> +            return PGM_ADDRESSING;
> +        }
> +
> +        mmu_handle_skey(*raddr, rw, flags);
> +    }
>       return 0;
>   }
>   
> @@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages,
>           if (ret) {
>               return ret;
>           }
> -        if (!address_space_access_valid(&address_space_memory, pages[i],
> -                                        TARGET_PAGE_SIZE, is_write,
> -                                        MEMTXATTRS_UNSPECIFIED)) {
> -            *tec = 0; /* unused */
> -            return PGM_ADDRESSING;
> -        }
>           addr += TARGET_PAGE_SIZE;
>       }
>   
> @@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
>   
>       *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
>   
> +    if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
> +        /* unused */
> +        *tec = 0;
> +        return PGM_ADDRESSING;
> +    }
> +
>       mmu_handle_skey(*addr, rw, flags);
>       return 0;
>   }
> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
> index a61917d04f..3d6662a53c 100644
> --- a/target/s390x/tcg/excp_helper.c
> +++ b/target/s390x/tcg/excp_helper.c
> @@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>           g_assert_not_reached();
>       }
>   
> -    /* check out of RAM access */
> -    if (!excp &&
> -        !address_space_access_valid(&address_space_memory, raddr,
> -                                    TARGET_PAGE_SIZE, access_type,
> -                                    MEMTXATTRS_UNSPECIFIED)) {
> -        MachineState *ms = MACHINE(qdev_get_machine());
> -        qemu_log_mask(CPU_LOG_MMU,
> -                      "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
> -                      __func__, (uint64_t)raddr, (uint64_t)ms->ram_size);
> -        excp = PGM_ADDRESSING;
> -        tec = 0; /* unused */
> -    }
> -
>       env->tlb_fill_exc = excp;
>       env->tlb_fill_tec = tec;
>   
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index a84795cfa3..9c1b9c7d06 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -2456,7 +2456,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)
>           tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
>       }
>   
> -    exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec);
> +    exc = mmu_translate(env, addr, -1, asc, &ret, &flags, &tec);

Do we maybe want a #define for this -1 instead? OTOH, you've added a proper 
comment to the function description, so that should be ok, too.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[David Hildenbrand]

On 06.08.21 10:18, Thomas Huth wrote:
> On 05/08/2021 17.27, David Hildenbrand wrote:
>> Let's move address validation into mmu_translate() and
>> mmu_translate_real(). This allows for checking whether an absolute
>> address is valid before looking up the storage key. We can now get rid of
>> the ram_size check.
>>
>> Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
>> a) We're not supposed to touch storage keys
>> b) We're not supposed to convert to an absolute address
>>
>> Let's use a fake, negative MMUAccessType to teach mmu_translate() to
>> fix that handling and to not perform address validation.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
>>    target/s390x/tcg/excp_helper.c | 13 ------------
>>    target/s390x/tcg/mem_helper.c  |  2 +-
>>    3 files changed, 22 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>> index ca25dadb5b..36ab4e9c81 100644
>> --- a/target/s390x/mmu_helper.c
>> +++ b/target/s390x/mmu_helper.c
>> @@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>>    {
>>        static S390SKeysClass *skeyclass;
>>        static S390SKeysState *ss;
>> -    MachineState *ms = MACHINE(qdev_get_machine());
>>        uint8_t key;
>>        int rc;
>>    
>> -    if (unlikely(addr >= ms->ram_size)) {
>> -        return;
>> -    }
>> -
>> +    /*
>> +     * We excpect to be called with an absolute address that has already been
>> +     * validated, such that we can reliably use it to lookup the storage key.
>> +     */
>>        if (unlikely(!ss)) {
>>            ss = s390_get_skeys_device();
>>            skeyclass = S390_SKEYS_GET_CLASS(ss);
>> @@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>>    /**
>>     * Translate a virtual (logical) address into a physical (absolute) address.
>>     * @param vaddr  the virtual address
>> - * @param rw     0 = read, 1 = write, 2 = code fetch
>> + * @param rw     0 = read, 1 = write, 2 = code fetch, < 0 = load real address
>>     * @param asc    address space control (one of the PSW_ASC_* modes)
>>     * @param raddr  the translated address is stored to this pointer
>>     * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this pointer
>> @@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
>>        }
>>    
>>    nodat:
>> -    /* Convert real address -> absolute address */
>> -    *raddr = mmu_real2abs(env, *raddr);
>> +    if (rw >= 0) {
>> +        /* Convert real address -> absolute address */
>> +        *raddr = mmu_real2abs(env, *raddr);
>>    
>> -    mmu_handle_skey(*raddr, rw, flags);
>> +        if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
>> +            *tec = 0; /* unused */
>> +            return PGM_ADDRESSING;
>> +        }
>> +
>> +        mmu_handle_skey(*raddr, rw, flags);
>> +    }
>>        return 0;
>>    }
>>    
>> @@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages,
>>            if (ret) {
>>                return ret;
>>            }
>> -        if (!address_space_access_valid(&address_space_memory, pages[i],
>> -                                        TARGET_PAGE_SIZE, is_write,
>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>> -            *tec = 0; /* unused */
>> -            return PGM_ADDRESSING;
>> -        }
>>            addr += TARGET_PAGE_SIZE;
>>        }
>>    
>> @@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,
>>    
>>        *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
>>    
>> +    if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
>> +        /* unused */
>> +        *tec = 0;
>> +        return PGM_ADDRESSING;
>> +    }
>> +
>>        mmu_handle_skey(*addr, rw, flags);
>>        return 0;
>>    }
>> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
>> index a61917d04f..3d6662a53c 100644
>> --- a/target/s390x/tcg/excp_helper.c
>> +++ b/target/s390x/tcg/excp_helper.c
>> @@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>            g_assert_not_reached();
>>        }
>>    
>> -    /* check out of RAM access */
>> -    if (!excp &&
>> -        !address_space_access_valid(&address_space_memory, raddr,
>> -                                    TARGET_PAGE_SIZE, access_type,
>> -                                    MEMTXATTRS_UNSPECIFIED)) {
>> -        MachineState *ms = MACHINE(qdev_get_machine());
>> -        qemu_log_mask(CPU_LOG_MMU,
>> -                      "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
>> -                      __func__, (uint64_t)raddr, (uint64_t)ms->ram_size);
>> -        excp = PGM_ADDRESSING;
>> -        tec = 0; /* unused */
>> -    }
>> -
>>        env->tlb_fill_exc = excp;
>>        env->tlb_fill_tec = tec;
>>    
>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>> index a84795cfa3..9c1b9c7d06 100644
>> --- a/target/s390x/tcg/mem_helper.c
>> +++ b/target/s390x/tcg/mem_helper.c
>> @@ -2456,7 +2456,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)
>>            tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
>>        }
>>    
>> -    exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec);
>> +    exc = mmu_translate(env, addr, -1, asc, &ret, &flags, &tec);
> 
> Do we maybe want a #define for this -1 instead? OTOH, you've added a proper
> comment to the function description, so that should be ok, too.

Ideally, I'd have used a completely new MMU_* type. But affecting all 
users in QEMU for one special case and having to handle it consequently 
accordingly all over the place feels wrong.

Where would you put the define?

Thanks!

> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[Thomas Huth]

On 06/08/2021 10.20, David Hildenbrand wrote:
> On 06.08.21 10:18, Thomas Huth wrote:
>> On 05/08/2021 17.27, David Hildenbrand wrote:
>>> Let's move address validation into mmu_translate() and
>>> mmu_translate_real(). This allows for checking whether an absolute
>>> address is valid before looking up the storage key. We can now get rid of
>>> the ram_size check.
>>>
>>> Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
>>> a) We're not supposed to touch storage keys
>>> b) We're not supposed to convert to an absolute address
>>>
>>> Let's use a fake, negative MMUAccessType to teach mmu_translate() to
>>> fix that handling and to not perform address validation.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
>>>    target/s390x/tcg/excp_helper.c | 13 ------------
>>>    target/s390x/tcg/mem_helper.c  |  2 +-
>>>    3 files changed, 22 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>>> index ca25dadb5b..36ab4e9c81 100644
>>> --- a/target/s390x/mmu_helper.c
>>> +++ b/target/s390x/mmu_helper.c
>>> @@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int 
>>> rw, int *flags)
>>>    {
>>>        static S390SKeysClass *skeyclass;
>>>        static S390SKeysState *ss;
>>> -    MachineState *ms = MACHINE(qdev_get_machine());
>>>        uint8_t key;
>>>        int rc;
>>> -    if (unlikely(addr >= ms->ram_size)) {
>>> -        return;
>>> -    }
>>> -
>>> +    /*
>>> +     * We excpect to be called with an absolute address that has already 
>>> been
>>> +     * validated, such that we can reliably use it to lookup the storage 
>>> key.
>>> +     */
>>>        if (unlikely(!ss)) {
>>>            ss = s390_get_skeys_device();
>>>            skeyclass = S390_SKEYS_GET_CLASS(ss);
>>> @@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int 
>>> rw, int *flags)
>>>    /**
>>>     * Translate a virtual (logical) address into a physical (absolute) 
>>> address.
>>>     * @param vaddr  the virtual address
>>> - * @param rw     0 = read, 1 = write, 2 = code fetch
>>> + * @param rw     0 = read, 1 = write, 2 = code fetch, < 0 = load real 
>>> address
>>>     * @param asc    address space control (one of the PSW_ASC_* modes)
>>>     * @param raddr  the translated address is stored to this pointer
>>>     * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this 
>>> pointer
>>> @@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong 
>>> vaddr, int rw, uint64_t asc,
>>>        }
>>>    nodat:
>>> -    /* Convert real address -> absolute address */
>>> -    *raddr = mmu_real2abs(env, *raddr);
>>> +    if (rw >= 0) {
>>> +        /* Convert real address -> absolute address */
>>> +        *raddr = mmu_real2abs(env, *raddr);
>>> -    mmu_handle_skey(*raddr, rw, flags);
>>> +        if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
>>> +            *tec = 0; /* unused */
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +
>>> +        mmu_handle_skey(*raddr, rw, flags);
>>> +    }
>>>        return 0;
>>>    }
>>> @@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr, 
>>> int nr_pages,
>>>            if (ret) {
>>>                return ret;
>>>            }
>>> -        if (!address_space_access_valid(&address_space_memory, pages[i],
>>> -                                        TARGET_PAGE_SIZE, is_write,
>>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>>> -            *tec = 0; /* unused */
>>> -            return PGM_ADDRESSING;
>>> -        }
>>>            addr += TARGET_PAGE_SIZE;
>>>        }
>>> @@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env, 
>>> target_ulong raddr, int rw,
>>>        *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
>>> +    if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
>>> +        /* unused */
>>> +        *tec = 0;
>>> +        return PGM_ADDRESSING;
>>> +    }
>>> +
>>>        mmu_handle_skey(*addr, rw, flags);
>>>        return 0;
>>>    }
>>> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
>>> index a61917d04f..3d6662a53c 100644
>>> --- a/target/s390x/tcg/excp_helper.c
>>> +++ b/target/s390x/tcg/excp_helper.c
>>> @@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, 
>>> int size,
>>>            g_assert_not_reached();
>>>        }
>>> -    /* check out of RAM access */
>>> -    if (!excp &&
>>> -        !address_space_access_valid(&address_space_memory, raddr,
>>> -                                    TARGET_PAGE_SIZE, access_type,
>>> -                                    MEMTXATTRS_UNSPECIFIED)) {
>>> -        MachineState *ms = MACHINE(qdev_get_machine());
>>> -        qemu_log_mask(CPU_LOG_MMU,
>>> -                      "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
>>> -                      __func__, (uint64_t)raddr, (uint64_t)ms->ram_size);
>>> -        excp = PGM_ADDRESSING;
>>> -        tec = 0; /* unused */
>>> -    }
>>> -
>>>        env->tlb_fill_exc = excp;
>>>        env->tlb_fill_tec = tec;
>>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>>> index a84795cfa3..9c1b9c7d06 100644
>>> --- a/target/s390x/tcg/mem_helper.c
>>> +++ b/target/s390x/tcg/mem_helper.c
>>> @@ -2456,7 +2456,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t 
>>> addr)
>>>            tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
>>>        }
>>> -    exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec);
>>> +    exc = mmu_translate(env, addr, -1, asc, &ret, &flags, &tec);
>>
>> Do we maybe want a #define for this -1 instead? OTOH, you've added a proper
>> comment to the function description, so that should be ok, too.
> 
> Ideally, I'd have used a completely new MMU_* type. But affecting all users 
> in QEMU for one special case and having to handle it consequently 
> accordingly all over the place feels wrong.
> 
> Where would you put the define?

I agree that this should not go into the common header ... so maybe into 
s390x-internal.h ?

  Thomas

Re: [PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[David Hildenbrand]

On 06.08.21 10:22, Thomas Huth wrote:
> On 06/08/2021 10.20, David Hildenbrand wrote:
>> On 06.08.21 10:18, Thomas Huth wrote:
>>> On 05/08/2021 17.27, David Hildenbrand wrote:
>>>> Let's move address validation into mmu_translate() and
>>>> mmu_translate_real(). This allows for checking whether an absolute
>>>> address is valid before looking up the storage key. We can now get rid of
>>>> the ram_size check.
>>>>
>>>> Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
>>>> a) We're not supposed to touch storage keys
>>>> b) We're not supposed to convert to an absolute address
>>>>
>>>> Let's use a fake, negative MMUAccessType to teach mmu_translate() to
>>>> fix that handling and to not perform address validation.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>     target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
>>>>     target/s390x/tcg/excp_helper.c | 13 ------------
>>>>     target/s390x/tcg/mem_helper.c  |  2 +-
>>>>     3 files changed, 22 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>>>> index ca25dadb5b..36ab4e9c81 100644
>>>> --- a/target/s390x/mmu_helper.c
>>>> +++ b/target/s390x/mmu_helper.c
>>>> @@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int
>>>> rw, int *flags)
>>>>     {
>>>>         static S390SKeysClass *skeyclass;
>>>>         static S390SKeysState *ss;
>>>> -    MachineState *ms = MACHINE(qdev_get_machine());
>>>>         uint8_t key;
>>>>         int rc;
>>>> -    if (unlikely(addr >= ms->ram_size)) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> +    /*
>>>> +     * We excpect to be called with an absolute address that has already
>>>> been
>>>> +     * validated, such that we can reliably use it to lookup the storage
>>>> key.
>>>> +     */
>>>>         if (unlikely(!ss)) {
>>>>             ss = s390_get_skeys_device();
>>>>             skeyclass = S390_SKEYS_GET_CLASS(ss);
>>>> @@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int
>>>> rw, int *flags)
>>>>     /**
>>>>      * Translate a virtual (logical) address into a physical (absolute)
>>>> address.
>>>>      * @param vaddr  the virtual address
>>>> - * @param rw     0 = read, 1 = write, 2 = code fetch
>>>> + * @param rw     0 = read, 1 = write, 2 = code fetch, < 0 = load real
>>>> address
>>>>      * @param asc    address space control (one of the PSW_ASC_* modes)
>>>>      * @param raddr  the translated address is stored to this pointer
>>>>      * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this
>>>> pointer
>>>> @@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong
>>>> vaddr, int rw, uint64_t asc,
>>>>         }
>>>>     nodat:
>>>> -    /* Convert real address -> absolute address */
>>>> -    *raddr = mmu_real2abs(env, *raddr);
>>>> +    if (rw >= 0) {
>>>> +        /* Convert real address -> absolute address */
>>>> +        *raddr = mmu_real2abs(env, *raddr);
>>>> -    mmu_handle_skey(*raddr, rw, flags);
>>>> +        if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
>>>> +            *tec = 0; /* unused */
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +
>>>> +        mmu_handle_skey(*raddr, rw, flags);
>>>> +    }
>>>>         return 0;
>>>>     }
>>>> @@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr,
>>>> int nr_pages,
>>>>             if (ret) {
>>>>                 return ret;
>>>>             }
>>>> -        if (!address_space_access_valid(&address_space_memory, pages[i],
>>>> -                                        TARGET_PAGE_SIZE, is_write,
>>>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>>>> -            *tec = 0; /* unused */
>>>> -            return PGM_ADDRESSING;
>>>> -        }
>>>>             addr += TARGET_PAGE_SIZE;
>>>>         }
>>>> @@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env,
>>>> target_ulong raddr, int rw,
>>>>         *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
>>>> +    if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
>>>> +        /* unused */
>>>> +        *tec = 0;
>>>> +        return PGM_ADDRESSING;
>>>> +    }
>>>> +
>>>>         mmu_handle_skey(*addr, rw, flags);
>>>>         return 0;
>>>>     }
>>>> diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
>>>> index a61917d04f..3d6662a53c 100644
>>>> --- a/target/s390x/tcg/excp_helper.c
>>>> +++ b/target/s390x/tcg/excp_helper.c
>>>> @@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address,
>>>> int size,
>>>>             g_assert_not_reached();
>>>>         }
>>>> -    /* check out of RAM access */
>>>> -    if (!excp &&
>>>> -        !address_space_access_valid(&address_space_memory, raddr,
>>>> -                                    TARGET_PAGE_SIZE, access_type,
>>>> -                                    MEMTXATTRS_UNSPECIFIED)) {
>>>> -        MachineState *ms = MACHINE(qdev_get_machine());
>>>> -        qemu_log_mask(CPU_LOG_MMU,
>>>> -                      "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
>>>> -                      __func__, (uint64_t)raddr, (uint64_t)ms->ram_size);
>>>> -        excp = PGM_ADDRESSING;
>>>> -        tec = 0; /* unused */
>>>> -    }
>>>> -
>>>>         env->tlb_fill_exc = excp;
>>>>         env->tlb_fill_tec = tec;
>>>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>>>> index a84795cfa3..9c1b9c7d06 100644
>>>> --- a/target/s390x/tcg/mem_helper.c
>>>> +++ b/target/s390x/tcg/mem_helper.c
>>>> @@ -2456,7 +2456,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t
>>>> addr)
>>>>             tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
>>>>         }
>>>> -    exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec);
>>>> +    exc = mmu_translate(env, addr, -1, asc, &ret, &flags, &tec);
>>>
>>> Do we maybe want a #define for this -1 instead? OTOH, you've added a proper
>>> comment to the function description, so that should be ok, too.
>>
>> Ideally, I'd have used a completely new MMU_* type. But affecting all users
>> in QEMU for one special case and having to handle it consequently
>> accordingly all over the place feels wrong.
>>
>> Where would you put the define?
> 
> I agree that this should not go into the common header ... so maybe into
> s390x-internal.h ?

Maybe calling it MMU_S390_LRA ?


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[Thomas Huth]

On 06/08/2021 10.23, David Hildenbrand wrote:
> On 06.08.21 10:22, Thomas Huth wrote:
>> On 06/08/2021 10.20, David Hildenbrand wrote:
>>> On 06.08.21 10:18, Thomas Huth wrote:
>>>> On 05/08/2021 17.27, David Hildenbrand wrote:
>>>>> Let's move address validation into mmu_translate() and
>>>>> mmu_translate_real(). This allows for checking whether an absolute
>>>>> address is valid before looking up the storage key. We can now get rid of
>>>>> the ram_size check.
>>>>>
>>>>> Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
>>>>> a) We're not supposed to touch storage keys
>>>>> b) We're not supposed to convert to an absolute address
>>>>>
>>>>> Let's use a fake, negative MMUAccessType to teach mmu_translate() to
>>>>> fix that handling and to not perform address validation.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>     target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
>>>>>     target/s390x/tcg/excp_helper.c | 13 ------------
>>>>>     target/s390x/tcg/mem_helper.c  |  2 +-
>>>>>     3 files changed, 22 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>>>>> index ca25dadb5b..36ab4e9c81 100644
>>>>> --- a/target/s390x/mmu_helper.c
>>>>> +++ b/target/s390x/mmu_helper.c
>>>>> @@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int
>>>>> rw, int *flags)
>>>>>     {
>>>>>         static S390SKeysClass *skeyclass;
>>>>>         static S390SKeysState *ss;
>>>>> -    MachineState *ms = MACHINE(qdev_get_machine());
>>>>>         uint8_t key;
>>>>>         int rc;
>>>>> -    if (unlikely(addr >= ms->ram_size)) {
>>>>> -        return;
>>>>> -    }
>>>>> -
>>>>> +    /*
>>>>> +     * We excpect to be called with an absolute address that has already
>>>>> been
>>>>> +     * validated, such that we can reliably use it to lookup the storage
>>>>> key.
>>>>> +     */
>>>>>         if (unlikely(!ss)) {
>>>>>             ss = s390_get_skeys_device();
>>>>>             skeyclass = S390_SKEYS_GET_CLASS(ss);
>>>>> @@ -370,7 +369,7 @@ static void mmu_handle_skey(target_ulong addr, int
>>>>> rw, int *flags)
>>>>>     /**
>>>>>      * Translate a virtual (logical) address into a physical (absolute)
>>>>> address.
>>>>>      * @param vaddr  the virtual address
>>>>> - * @param rw     0 = read, 1 = write, 2 = code fetch
>>>>> + * @param rw     0 = read, 1 = write, 2 = code fetch, < 0 = load real
>>>>> address
>>>>>      * @param asc    address space control (one of the PSW_ASC_* modes)
>>>>>      * @param raddr  the translated address is stored to this pointer
>>>>>      * @param flags  the PAGE_READ/WRITE/EXEC flags are stored to this
>>>>> pointer
>>>>> @@ -449,10 +448,17 @@ int mmu_translate(CPUS390XState *env, target_ulong
>>>>> vaddr, int rw, uint64_t asc,
>>>>>         }
>>>>>     nodat:
>>>>> -    /* Convert real address -> absolute address */
>>>>> -    *raddr = mmu_real2abs(env, *raddr);
>>>>> +    if (rw >= 0) {
>>>>> +        /* Convert real address -> absolute address */
>>>>> +        *raddr = mmu_real2abs(env, *raddr);
>>>>> -    mmu_handle_skey(*raddr, rw, flags);
>>>>> +        if (!mmu_absolute_addr_valid(*raddr, rw == MMU_DATA_STORE)) {
>>>>> +            *tec = 0; /* unused */
>>>>> +            return PGM_ADDRESSING;
>>>>> +        }
>>>>> +
>>>>> +        mmu_handle_skey(*raddr, rw, flags);
>>>>> +    }
>>>>>         return 0;
>>>>>     }
>>>>> @@ -473,12 +479,6 @@ static int translate_pages(S390CPU *cpu, vaddr addr,
>>>>> int nr_pages,
>>>>>             if (ret) {
>>>>>                 return ret;
>>>>>             }
>>>>> -        if (!address_space_access_valid(&address_space_memory, pages[i],
>>>>> -                                        TARGET_PAGE_SIZE, is_write,
>>>>> -                                        MEMTXATTRS_UNSPECIFIED)) {
>>>>> -            *tec = 0; /* unused */
>>>>> -            return PGM_ADDRESSING;
>>>>> -        }
>>>>>             addr += TARGET_PAGE_SIZE;
>>>>>         }
>>>>> @@ -588,6 +588,12 @@ int mmu_translate_real(CPUS390XState *env,
>>>>> target_ulong raddr, int rw,
>>>>>         *addr = mmu_real2abs(env, raddr & TARGET_PAGE_MASK);
>>>>> +    if (!mmu_absolute_addr_valid(*addr, rw == MMU_DATA_STORE)) {
>>>>> +        /* unused */
>>>>> +        *tec = 0;
>>>>> +        return PGM_ADDRESSING;
>>>>> +    }
>>>>> +
>>>>>         mmu_handle_skey(*addr, rw, flags);
>>>>>         return 0;
>>>>>     }
>>>>> diff --git a/target/s390x/tcg/excp_helper.c 
>>>>> b/target/s390x/tcg/excp_helper.c
>>>>> index a61917d04f..3d6662a53c 100644
>>>>> --- a/target/s390x/tcg/excp_helper.c
>>>>> +++ b/target/s390x/tcg/excp_helper.c
>>>>> @@ -150,19 +150,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address,
>>>>> int size,
>>>>>             g_assert_not_reached();
>>>>>         }
>>>>> -    /* check out of RAM access */
>>>>> -    if (!excp &&
>>>>> -        !address_space_access_valid(&address_space_memory, raddr,
>>>>> -                                    TARGET_PAGE_SIZE, access_type,
>>>>> -                                    MEMTXATTRS_UNSPECIFIED)) {
>>>>> -        MachineState *ms = MACHINE(qdev_get_machine());
>>>>> -        qemu_log_mask(CPU_LOG_MMU,
>>>>> -                      "%s: raddr %" PRIx64 " > ram_size %" PRIx64 "\n",
>>>>> -                      __func__, (uint64_t)raddr, (uint64_t)ms->ram_size);
>>>>> -        excp = PGM_ADDRESSING;
>>>>> -        tec = 0; /* unused */
>>>>> -    }
>>>>> -
>>>>>         env->tlb_fill_exc = excp;
>>>>>         env->tlb_fill_tec = tec;
>>>>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>>>>> index a84795cfa3..9c1b9c7d06 100644
>>>>> --- a/target/s390x/tcg/mem_helper.c
>>>>> +++ b/target/s390x/tcg/mem_helper.c
>>>>> @@ -2456,7 +2456,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t
>>>>> addr)
>>>>>             tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
>>>>>         }
>>>>> -    exc = mmu_translate(env, addr, 0, asc, &ret, &flags, &tec);
>>>>> +    exc = mmu_translate(env, addr, -1, asc, &ret, &flags, &tec);
>>>>
>>>> Do we maybe want a #define for this -1 instead? OTOH, you've added a proper
>>>> comment to the function description, so that should be ok, too.
>>>
>>> Ideally, I'd have used a completely new MMU_* type. But affecting all users
>>> in QEMU for one special case and having to handle it consequently
>>> accordingly all over the place feels wrong.
>>>
>>> Where would you put the define?
>>
>> I agree that this should not go into the common header ... so maybe into
>> s390x-internal.h ?
> 
> Maybe calling it MMU_S390_LRA ?

Sounds good!

  Thomas

Re: [PATCH v1 07/12] s390x/mmu_helper: move address validation into mmu_translate*()

[Thomas Huth]

On 05/08/2021 17.27, David Hildenbrand wrote:
> Let's move address validation into mmu_translate() and
> mmu_translate_real(). This allows for checking whether an absolute
> address is valid before looking up the storage key. We can now get rid of
> the ram_size check.
> 
> Interestingly, we're already handling LOAD REAL ADDRESS wrong, because
> a) We're not supposed to touch storage keys
> b) We're not supposed to convert to an absolute address
> 
> Let's use a fake, negative MMUAccessType to teach mmu_translate() to
> fix that handling and to not perform address validation.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/mmu_helper.c      | 36 ++++++++++++++++++++--------------
>   target/s390x/tcg/excp_helper.c | 13 ------------
>   target/s390x/tcg/mem_helper.c  |  2 +-
>   3 files changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index ca25dadb5b..36ab4e9c81 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -301,14 +301,13 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>   {
>       static S390SKeysClass *skeyclass;
>       static S390SKeysState *ss;
> -    MachineState *ms = MACHINE(qdev_get_machine());
>       uint8_t key;
>       int rc;
>   
> -    if (unlikely(addr >= ms->ram_size)) {
> -        return;
> -    }
> -
> +    /*
> +     * We excpect to be called with an absolute address that has already been

s/excpect/expect/

[PATCH v1 08/12] s390x/mmu_helper: avoid setting the storage key if nothing changed

[David Hildenbrand]

Avoid setting the key if nothing changed.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 36ab4e9c81..0c2c39a970 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -301,7 +301,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
 {
     static S390SKeysClass *skeyclass;
     static S390SKeysState *ss;
-    uint8_t key;
+    uint8_t key, old_key;
     int rc;
 
     /*
@@ -337,6 +337,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
         trace_get_skeys_nonzero(rc);
         return;
     }
+    old_key = key;
 
     switch (rw) {
     case MMU_DATA_LOAD:
@@ -360,9 +361,11 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
     /* Any store/fetch sets the reference bit */
     key |= SK_R;
 
-    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
-    if (rc) {
-        trace_set_skeys_nonzero(rc);
+    if (key != old_key) {
+        rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+        if (rc) {
+            trace_set_skeys_nonzero(rc);
+        }
     }
 }
 
-- 
2.31.1

Re: [PATCH v1 08/12] s390x/mmu_helper: avoid setting the storage key if nothing changed

[Thomas Huth]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Avoid setting the key if nothing changed.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/mmu_helper.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 36ab4e9c81..0c2c39a970 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -301,7 +301,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>   {
>       static S390SKeysClass *skeyclass;
>       static S390SKeysState *ss;
> -    uint8_t key;
> +    uint8_t key, old_key;
>       int rc;
>   
>       /*
> @@ -337,6 +337,7 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>           trace_get_skeys_nonzero(rc);
>           return;
>       }
> +    old_key = key;
>   
>       switch (rw) {
>       case MMU_DATA_LOAD:
> @@ -360,9 +361,11 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
>       /* Any store/fetch sets the reference bit */
>       key |= SK_R;
>   
> -    rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
> -    if (rc) {
> -        trace_set_skeys_nonzero(rc);
> +    if (key != old_key) {
> +        rc = skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
> +        if (rc) {
> +            trace_set_skeys_nonzero(rc);
> +        }
>       }
>   }
>   
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 09/12] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to migrate

[David Hildenbrand]

Let's use the guest_phys_blocks API to get physical memory regions
that are well defined inside our physical address space and migrate the
storage keys of these.

This is a prepearation for having memory besides initial ram defined in
the guest physical address space, for example, via memory devices. We
get rid of the ms->ram_size dependency.

Please note that we will usually have very little (--> 1) physical
ranges. With virtio-mem might have significantly more ranges in the
future. If that turns out to be a problem (e.g., total memory
footprint of the list), we could look into a memory mapping
API that avoids creation of a list and instead triggers a callback for
each range.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-skeys.c | 70 ++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 9a8d60d1d9..250685a95a 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -17,6 +17,7 @@
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
+#include "sysemu/memory_mapping.h"
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
@@ -257,10 +258,9 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
 {
     S390SKeysState *ss = S390_SKEYS(opaque);
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
-    MachineState *ms = MACHINE(qdev_get_machine());
-    uint64_t pages_left = ms->ram_size / TARGET_PAGE_SIZE;
-    uint64_t read_count, eos = S390_SKEYS_SAVE_FLAG_EOS;
-    vaddr cur_gfn = 0;
+    GuestPhysBlockList guest_phys_blocks;
+    GuestPhysBlock *block;
+    uint64_t pages, gfn;
     int error = 0;
     uint8_t *buf;
 
@@ -274,36 +274,52 @@ static void s390_storage_keys_save(QEMUFile *f, void *opaque)
         goto end_stream;
     }
 
-    /* We only support initial memory. Standby memory is not handled yet. */
-    qemu_put_be64(f, (cur_gfn * TARGET_PAGE_SIZE) | S390_SKEYS_SAVE_FLAG_SKEYS);
-    qemu_put_be64(f, pages_left);
-
-    while (pages_left) {
-        read_count = MIN(pages_left, S390_SKEYS_BUFFER_SIZE);
-
-        if (!error) {
-            error = skeyclass->get_skeys(ss, cur_gfn, read_count, buf);
-            if (error) {
-                /*
-                 * If error: we want to fill the stream with valid data instead
-                 * of stopping early so we pad the stream with 0x00 values and
-                 * use S390_SKEYS_SAVE_FLAG_ERROR to indicate failure to the
-                 * reading side.
-                 */
-                error_report("S390_GET_KEYS error %d", error);
-                memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
-                eos = S390_SKEYS_SAVE_FLAG_ERROR;
+    guest_phys_blocks_init(&guest_phys_blocks);
+    guest_phys_blocks_append(&guest_phys_blocks);
+
+    /* Send each contiguous physical memory range separately. */
+    QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+        assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
+        assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
+
+        gfn = block->target_start / TARGET_PAGE_SIZE;
+        pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE;
+        qemu_put_be64(f, block->target_start | S390_SKEYS_SAVE_FLAG_SKEYS);
+        qemu_put_be64(f, pages);
+
+        while (pages) {
+            const uint64_t cur_pages = MIN(pages, S390_SKEYS_BUFFER_SIZE);
+
+            if (!error) {
+                error = skeyclass->get_skeys(ss, gfn, cur_pages, buf);
+                if (error) {
+                    /*
+                     * Create a valid stream with all 0x00 and indicate
+                     * S390_SKEYS_SAVE_FLAG_ERROR to the destination.
+                     */
+                    error_report("S390_GET_KEYS error %d", error);
+                    memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
+                }
             }
+
+            qemu_put_buffer(f, buf, cur_pages);
+            gfn += cur_pages;
+            pages -= cur_pages;
         }
 
-        qemu_put_buffer(f, buf, read_count);
-        cur_gfn += read_count;
-        pages_left -= read_count;
+        if (error) {
+            break;
+        }
     }
 
+    guest_phys_blocks_free(&guest_phys_blocks);
     g_free(buf);
 end_stream:
-    qemu_put_be64(f, eos);
+    if (error) {
+        qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_ERROR);
+    } else {
+        qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
+    }
 }
 
 static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
-- 
2.31.1

Re: [PATCH v1 09/12] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to migrate

[Thomas Huth]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Let's use the guest_phys_blocks API to get physical memory regions
> that are well defined inside our physical address space and migrate the
> storage keys of these.
> 
> This is a prepearation for having memory besides initial ram defined in

s/prepearation/preparation/

> the guest physical address space, for example, via memory devices. We
> get rid of the ms->ram_size dependency.
> 
> Please note that we will usually have very little (--> 1) physical
> ranges. With virtio-mem might have significantly more ranges in the
> future. If that turns out to be a problem (e.g., total memory
> footprint of the list), we could look into a memory mapping
> API that avoids creation of a list and instead triggers a callback for
> each range.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-skeys.c | 70 ++++++++++++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 27 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 10/12] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to dump

[David Hildenbrand]

Handle it similar to migration. Assert that we're holding the BQL, to
make sure we don't see concurrent modifications.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-skeys.c | 50 ++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 250685a95a..56a47fe180 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -110,11 +110,10 @@ void qmp_dump_skeys(const char *filename, Error **errp)
 {
     S390SKeysState *ss = s390_get_skeys_device();
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
-    MachineState *ms = MACHINE(qdev_get_machine());
-    const uint64_t total_count = ms->ram_size / TARGET_PAGE_SIZE;
-    uint64_t handled_count = 0, cur_count;
+    GuestPhysBlockList guest_phys_blocks;
+    GuestPhysBlock *block;
+    uint64_t pages, gfn;
     Error *lerr = NULL;
-    vaddr cur_gfn = 0;
     uint8_t *buf;
     int ret;
     int fd;
@@ -145,28 +144,39 @@ void qmp_dump_skeys(const char *filename, Error **errp)
         goto out;
     }
 
-    /* we'll only dump initial memory for now */
-    while (handled_count < total_count) {
-        /* Calculate how many keys to ask for & handle overflow case */
-        cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE);
+    assert(qemu_mutex_iothread_locked());
+    guest_phys_blocks_init(&guest_phys_blocks);
+    guest_phys_blocks_append(&guest_phys_blocks);
 
-        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
-        if (ret < 0) {
-            error_setg(errp, "get_keys error %d", ret);
-            goto out_free;
-        }
+    QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+        assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
+        assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
 
-        /* write keys to stream */
-        write_keys(f, buf, cur_gfn, cur_count, &lerr);
-        if (lerr) {
-            goto out_free;
-        }
+        gfn = block->target_start / TARGET_PAGE_SIZE;
+        pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE;
 
-        cur_gfn += cur_count;
-        handled_count += cur_count;
+        while (pages) {
+            const uint64_t cur_pages = MIN(pages, S390_SKEYS_BUFFER_SIZE);
+
+            ret = skeyclass->get_skeys(ss, gfn, cur_pages, buf);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "get_keys error");
+                goto out_free;
+            }
+
+            /* write keys to stream */
+            write_keys(f, buf, gfn, cur_pages, &lerr);
+            if (lerr) {
+                goto out_free;
+            }
+
+            gfn += cur_pages;
+            pages -= cur_pages;
+        }
     }
 
 out_free:
+    guest_phys_blocks_free(&guest_phys_blocks);
     error_propagate(errp, lerr);
     g_free(buf);
 out:
-- 
2.31.1

Re: [PATCH v1 10/12] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to dump

[Thomas Huth]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Handle it similar to migration. Assert that we're holding the BQL, to
> make sure we don't see concurrent modifications.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-skeys.c | 50 ++++++++++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 250685a95a..56a47fe180 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -110,11 +110,10 @@ void qmp_dump_skeys(const char *filename, Error **errp)
>   {
>       S390SKeysState *ss = s390_get_skeys_device();
>       S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
> -    MachineState *ms = MACHINE(qdev_get_machine());
> -    const uint64_t total_count = ms->ram_size / TARGET_PAGE_SIZE;
> -    uint64_t handled_count = 0, cur_count;
> +    GuestPhysBlockList guest_phys_blocks;
> +    GuestPhysBlock *block;
> +    uint64_t pages, gfn;
>       Error *lerr = NULL;
> -    vaddr cur_gfn = 0;
>       uint8_t *buf;
>       int ret;
>       int fd;
> @@ -145,28 +144,39 @@ void qmp_dump_skeys(const char *filename, Error **errp)
>           goto out;
>       }
>   
> -    /* we'll only dump initial memory for now */
> -    while (handled_count < total_count) {
> -        /* Calculate how many keys to ask for & handle overflow case */
> -        cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE);
> +    assert(qemu_mutex_iothread_locked());
> +    guest_phys_blocks_init(&guest_phys_blocks);
> +    guest_phys_blocks_append(&guest_phys_blocks);
>   
> -        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
> -        if (ret < 0) {
> -            error_setg(errp, "get_keys error %d", ret);
> -            goto out_free;
> -        }
> +    QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
> +        assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
> +        assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
>   
> -        /* write keys to stream */
> -        write_keys(f, buf, cur_gfn, cur_count, &lerr);
> -        if (lerr) {
> -            goto out_free;
> -        }
> +        gfn = block->target_start / TARGET_PAGE_SIZE;
> +        pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE;
>   
> -        cur_gfn += cur_count;
> -        handled_count += cur_count;
> +        while (pages) {
> +            const uint64_t cur_pages = MIN(pages, S390_SKEYS_BUFFER_SIZE);
> +
> +            ret = skeyclass->get_skeys(ss, gfn, cur_pages, buf);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "get_keys error");
> +                goto out_free;
> +            }
> +
> +            /* write keys to stream */
> +            write_keys(f, buf, gfn, cur_pages, &lerr);
> +            if (lerr) {
> +                goto out_free;
> +            }
> +
> +            gfn += cur_pages;
> +            pages -= cur_pages;
> +        }
>       }
>   
>   out_free:
> +    guest_phys_blocks_free(&guest_phys_blocks);
>       error_propagate(errp, lerr);
>       g_free(buf);
>   out:
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 11/12] hw/s390x/s390-skeys: check if an address is valid before dumping the key

[David Hildenbrand]

Let's validate the given address and report a proper error in case it's
not. All call paths now properly check the validity of the given GFN.
Remove the TODO.

The errors inside the getter and setter should only trigger if something
really goes wrong now, for example, with a broken migration stream. Or
when we forget to update the storage key allocation with memory hotplug.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-skeys.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 56a47fe180..53e16f1b9c 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
 #include "sysemu/memory_mapping.h"
+#include "exec/address-spaces.h"
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
@@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    if (!address_space_access_valid(&address_space_memory,
+                                    addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
+                                    false, MEMTXATTRS_UNSPECIFIED)) {
+        monitor_printf(mon, "Error: The given address is not valid\n");
+    }
+
     r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     if (r < 0) {
         monitor_printf(mon, "Error: %s\n", strerror(-r));
@@ -197,11 +204,6 @@ static int qemu_s390_skeys_enabled(S390SKeysState *ss)
     return 1;
 }
 
-/*
- * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get
- * will have to make sure that the given gfn belongs to a memory region and not
- * a memory hole.
- */
 static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
                               uint64_t count, uint8_t *keys)
 {
-- 
2.31.1

Re: [PATCH v1 11/12] hw/s390x/s390-skeys: check if an address is valid before dumping the key

[Thomas Huth]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Let's validate the given address and report a proper error in case it's
> not. All call paths now properly check the validity of the given GFN.
> Remove the TODO.
> 
> The errors inside the getter and setter should only trigger if something
> really goes wrong now, for example, with a broken migration stream. Or
> when we forget to update the storage key allocation with memory hotplug.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-skeys.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 56a47fe180..53e16f1b9c 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -18,6 +18,7 @@
>   #include "qapi/qmp/qdict.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/memory_mapping.h"
> +#include "exec/address-spaces.h"
>   #include "sysemu/kvm.h"
>   #include "migration/qemu-file-types.h"
>   #include "migration/register.h"
> @@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
>           return;
>       }
>   
> +    if (!address_space_access_valid(&address_space_memory,
> +                                    addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
> +                                    false, MEMTXATTRS_UNSPECIFIED)) {
> +        monitor_printf(mon, "Error: The given address is not valid\n");

I think the code should return here?

> +    }
> +
>       r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
>       if (r < 0) {
>           monitor_printf(mon, "Error: %s\n", strerror(-r));
> @@ -197,11 +204,6 @@ static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>       return 1;
>   }
>   
> -/*
> - * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get
> - * will have to make sure that the given gfn belongs to a memory region and not
> - * a memory hole.
> - */
>   static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
>                                 uint64_t count, uint8_t *keys)
>   {
> 

  Thomas

Re: [PATCH v1 11/12] hw/s390x/s390-skeys: check if an address is valid before dumping the key

[David Hildenbrand]

On 06.08.21 10:53, Thomas Huth wrote:
> On 05/08/2021 17.28, David Hildenbrand wrote:
>> Let's validate the given address and report a proper error in case it's
>> not. All call paths now properly check the validity of the given GFN.
>> Remove the TODO.
>>
>> The errors inside the getter and setter should only trigger if something
>> really goes wrong now, for example, with a broken migration stream. Or
>> when we forget to update the storage key allocation with memory hotplug.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    hw/s390x/s390-skeys.c | 12 +++++++-----
>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 56a47fe180..53e16f1b9c 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -18,6 +18,7 @@
>>    #include "qapi/qmp/qdict.h"
>>    #include "qemu/error-report.h"
>>    #include "sysemu/memory_mapping.h"
>> +#include "exec/address-spaces.h"
>>    #include "sysemu/kvm.h"
>>    #include "migration/qemu-file-types.h"
>>    #include "migration/register.h"
>> @@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict)
>>            return;
>>        }
>>    
>> +    if (!address_space_access_valid(&address_space_memory,
>> +                                    addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
>> +                                    false, MEMTXATTRS_UNSPECIFIED)) {
>> +        monitor_printf(mon, "Error: The given address is not valid\n");
> 
> I think the code should return here?

Whoops, very right. Thanks!


-- 
Thanks,

David / dhildenb

[PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[David Hildenbrand]

Let's enable storage keys lazily under TCG, just as we do under KVM.
Only fairly old Linux versions actually make use of storage keys, so it
can be kind of wasteful to allocate quite some memory and track
changes and references if nobody cares.

We have to make sure to flush the TLB when enabling storage keys after
the VM was already running: otherwise it might happen that we don't
catch references or modifications afterwards.

Add proper documentation to all callbacks.

The kvm-unit-tests skey tests keeps on working with this change.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-skeys.c           | 51 +++++++++++++++++++++-----
 include/hw/s390x/storage-keys.h | 63 +++++++++++++++++++++++++++++++++
 target/s390x/mmu_helper.c       |  8 +++++
 target/s390x/tcg/mem_helper.c   |  9 +++++
 4 files changed, 123 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 53e16f1b9c..579bdf1d8a 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -190,18 +190,45 @@ out:
     fclose(f);
 }
 
-static void qemu_s390_skeys_init(Object *obj)
+static int qemu_s390_skeys_enabled(S390SKeysState *ss)
 {
-    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
-    MachineState *machine = MACHINE(qdev_get_machine());
+    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
 
-    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
-    skeys->keydata = g_malloc0(skeys->key_count);
+    /* Lockless check is sufficient. */
+    return !!skeys->keydata;
 }
 
-static int qemu_s390_skeys_enabled(S390SKeysState *ss)
+static int qemu_s390_skeys_enable(S390SKeysState *ss)
 {
-    return 1;
+    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
+    static gsize initialized;
+
+    if (likely(skeys->keydata)) {
+        return 1;
+    }
+
+    /*
+     * TODO: Modern Linux doesn't use storage keys unless running KVM guests
+     *       that use storage keys. Therefore, we keep it simple for now.
+     *
+     * 1) We should initialize to "referenced+changed" for an initial
+     *    over-indication. Let's avoid touching megabytes of data for now and
+     *    assume that any sane user will issue a storage key instruction before
+     *    actually relying on this data.
+     * 2) Relying on ram_size and allocating a big array is ugly. We should
+     *    allocate and manage storage key data per RAMBlock or optimally using
+     *    some sparse data structure.
+     * 3) We only ever have a single S390SKeysState, so relying on
+     *    g_once_init_enter() is good enough.
+     */
+    if (g_once_init_enter(&initialized)) {
+        MachineState *machine = MACHINE(qdev_get_machine());
+
+        skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
+        skeys->keydata = g_malloc0(skeys->key_count);
+        g_once_init_leave(&initialized, 1);
+    }
+    return 0;
 }
 
 static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
@@ -250,6 +277,7 @@ static void qemu_s390_skeys_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     skeyclass->skeys_enabled = qemu_s390_skeys_enabled;
+    skeyclass->skeys_enable = qemu_s390_skeys_enable;
     skeyclass->get_skeys = qemu_s390_skeys_get;
     skeyclass->set_skeys = qemu_s390_skeys_set;
 
@@ -260,7 +288,6 @@ static void qemu_s390_skeys_class_init(ObjectClass *oc, void *data)
 static const TypeInfo qemu_s390_skeys_info = {
     .name          = TYPE_QEMU_S390_SKEYS,
     .parent        = TYPE_S390_SKEYS,
-    .instance_init = qemu_s390_skeys_init,
     .instance_size = sizeof(QEMUS390SKeysState),
     .class_init    = qemu_s390_skeys_class_init,
     .class_size    = sizeof(S390SKeysClass),
@@ -340,6 +367,14 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
     int ret = 0;
 
+    /*
+     * Make sure to lazy-enable if required to be done explicitly. No need to
+     * flush any TLB as the VM is not running yet.
+     */
+    if (skeyclass->skeys_enable) {
+        skeyclass->skeys_enable(ss);
+    }
+
     while (!ret) {
         ram_addr_t addr;
         int flags;
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index 2888d42d0b..8b15809716 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -28,9 +28,72 @@ struct S390SKeysState {
 
 struct S390SKeysClass {
     DeviceClass parent_class;
+
+    /**
+     * @skeys_enabled:
+     *
+     * Check whether storage keys are enabled. If not enabled, they were not
+     * enabled lazily either by the guest via a storage key instruction or
+     * by the host during migration.
+     *
+     * If disabled, everything not explicitly triggered by the guest,
+     * such as outgoing migration or dirty/change tracking, should not touch
+     * storage keys and should not lazily enable it.
+     *
+     * @ks: the #S390SKeysState
+     *
+     * Returns 0 if not enabled and 1 if enabled.
+     */
     int (*skeys_enabled)(S390SKeysState *ks);
+
+    /**
+     * @skeys_enable:
+     *
+     * Lazily enable storage keys. If this function is not implemented,
+     * setting a storage key will lazily enable storage keys implicitly
+     * instead. TCG guests have to make sure to flush the TLB of all CPUs
+     * if storage keys were not enabled before this call.
+     *
+     * @ks: the #S390SKeysState
+     *
+     * Returns 0 if storage keys were not enabled before this call and 1 if
+     * they were already enabled.
+     */
+    int (*skeys_enable)(S390SKeysState *ks);
+
+    /**
+     * @get_skeys:
+     *
+     * Get storage keys for the given PFN range. This call will fail if
+     * storage keys have not been lazily enabled yet.
+     *
+     * Callers have to validate that a GFN is valid before this call.
+     *
+     * @ks: the #S390SKeysState
+     * @start_gfn: the start GFN to get storage keys for
+     * @count: the number of storage keys to get
+     * @keys: the byte array where storage keys will be stored to
+     *
+     * Returns 0 on success, returns an error if getting a storage key failed.
+     */
     int (*get_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count,
                      uint8_t *keys);
+    /**
+     * @set_skeys:
+     *
+     * Set storage keys for the given PFN range. This call will fail if
+     * storage keys have not been lazily enabled yet and implicit
+     * enablement is not supported.
+     *
+     * Callers have to validate that a GFN is valid before this call.
+     *
+     * @ks: the #S390SKeysState
+     * @start_gfn: the start GFN to set storage keys for
+     * @count: the number of storage keys to set
+     * @keys: the byte array where storage keys will be read from
+     *
+     * Returns 0 on success, returns an error if setting a storage key failed.
+     */
     int (*set_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count,
                      uint8_t *keys);
 };
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 0c2c39a970..41bb256ea8 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -313,6 +313,14 @@ static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
         skeyclass = S390_SKEYS_GET_CLASS(ss);
     }
 
+    /*
+     * Don't enable storage keys if they are still disabled, i.e., no actual
+     * storage key instruction was issued yet.
+     */
+    if (!skeyclass->skeys_enabled(ss)) {
+        return;
+    }
+
     /*
      * Whenever we create a new TLB entry, we set the storage key reference
      * bit. In case we allow write accesses, we set the storage key change
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 9c1b9c7d06..0b318c6178 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2186,6 +2186,9 @@ uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
     if (unlikely(!ss)) {
         ss = s390_get_skeys_device();
         skeyclass = S390_SKEYS_GET_CLASS(ss);
+        if (skeyclass->skeys_enable && !skeyclass->skeys_enable(ss)) {
+            tlb_flush_all_cpus_synced(env_cpu(env));
+        }
     }
 
     rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
@@ -2213,6 +2216,9 @@ void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
     if (unlikely(!ss)) {
         ss = s390_get_skeys_device();
         skeyclass = S390_SKEYS_GET_CLASS(ss);
+        if (skeyclass->skeys_enable && !skeyclass->skeys_enable(ss)) {
+            tlb_flush_all_cpus_synced(env_cpu(env));
+        }
     }
 
     key = r1 & 0xfe;
@@ -2245,6 +2251,9 @@ uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
     if (unlikely(!ss)) {
         ss = s390_get_skeys_device();
         skeyclass = S390_SKEYS_GET_CLASS(ss);
+        if (skeyclass->skeys_enable && !skeyclass->skeys_enable(ss)) {
+            tlb_flush_all_cpus_synced(env_cpu(env));
+        }
     }
 
     rc = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
-- 
2.31.1

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[Thomas Huth]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Let's enable storage keys lazily under TCG, just as we do under KVM.
> Only fairly old Linux versions actually make use of storage keys, so it
> can be kind of wasteful to allocate quite some memory and track
> changes and references if nobody cares.
> 
> We have to make sure to flush the TLB when enabling storage keys after
> the VM was already running: otherwise it might happen that we don't
> catch references or modifications afterwards.
> 
> Add proper documentation to all callbacks.
> 
> The kvm-unit-tests skey tests keeps on working with this change.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-skeys.c           | 51 +++++++++++++++++++++-----
>   include/hw/s390x/storage-keys.h | 63 +++++++++++++++++++++++++++++++++
>   target/s390x/mmu_helper.c       |  8 +++++
>   target/s390x/tcg/mem_helper.c   |  9 +++++
>   4 files changed, 123 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 53e16f1b9c..579bdf1d8a 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -190,18 +190,45 @@ out:
>       fclose(f);
>   }
>   
> -static void qemu_s390_skeys_init(Object *obj)
> +static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>   {
> -    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
> -    MachineState *machine = MACHINE(qdev_get_machine());
> +    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
>   
> -    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
> -    skeys->keydata = g_malloc0(skeys->key_count);
> +    /* Lockless check is sufficient. */
> +    return !!skeys->keydata;
>   }
>   
> -static int qemu_s390_skeys_enabled(S390SKeysState *ss)
> +static int qemu_s390_skeys_enable(S390SKeysState *ss)

Could you please call this qemu_s390_skeys_activate() so that it's not so 
easily confused with the ..._enabled() function?

> diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
> index 2888d42d0b..8b15809716 100644
> --- a/include/hw/s390x/storage-keys.h
> +++ b/include/hw/s390x/storage-keys.h
> @@ -28,9 +28,72 @@ struct S390SKeysState {
>   
>   struct S390SKeysClass {
>       DeviceClass parent_class;
> +
> +    /**
> +     * @skeys_enabled:
> +     *
> +     * Check whether storage keys are enabled. If not enabled, they were not
> +     * enabled lazily either by the guest via a storage key instruction or
> +     * by the host during migration.
> +     *
> +     * If disabled, everything not explicitly triggered by the guest,
> +     * such as outgoing migration or dirty/change tracking, should not touch
> +     * storage keys and should not lazily enable it.
> +     *
> +     * @ks: the #S390SKeysState
> +     *
> +     * Returns 0 if not enabled and 1 if enabled.
> +     */
>       int (*skeys_enabled)(S390SKeysState *ks);
> +
> +    /**
> +     * @skeys_enable:
> +     *
> +     * Lazily enable storage keys. If this function is not implemented,
> +     * setting a storage key will lazily enable storage keys implicitly
> +     * instead. TCG guests have to make sure to flush the TLB of all CPUs
> +     * if storage keys were not enabled before this call.
> +     *
> +     * @ks: the #S390SKeysState
> +     *
> +     * Returns 0 if storage keys were not enabled before this call and 1 if
> +     * they were already enabled.
> +     */
> +    int (*skeys_enable)(S390SKeysState *ks);
> +
> +    /**
> +     * @get_skeys:
> +     *
> +     * Get storage keys for the given PFN range. This call will fail if
> +     * storage keys have not been lazily enabled yet.

Shouldn't there be some modifications to qemu_s390_skeys_get() in that case? 
Or does "fail" mean that it crashes?

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[David Hildenbrand]

On 06.08.21 11:42, Thomas Huth wrote:
> On 05/08/2021 17.28, David Hildenbrand wrote:
>> Let's enable storage keys lazily under TCG, just as we do under KVM.
>> Only fairly old Linux versions actually make use of storage keys, so it
>> can be kind of wasteful to allocate quite some memory and track
>> changes and references if nobody cares.
>>
>> We have to make sure to flush the TLB when enabling storage keys after
>> the VM was already running: otherwise it might happen that we don't
>> catch references or modifications afterwards.
>>
>> Add proper documentation to all callbacks.
>>
>> The kvm-unit-tests skey tests keeps on working with this change.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    hw/s390x/s390-skeys.c           | 51 +++++++++++++++++++++-----
>>    include/hw/s390x/storage-keys.h | 63 +++++++++++++++++++++++++++++++++
>>    target/s390x/mmu_helper.c       |  8 +++++
>>    target/s390x/tcg/mem_helper.c   |  9 +++++
>>    4 files changed, 123 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 53e16f1b9c..579bdf1d8a 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -190,18 +190,45 @@ out:
>>        fclose(f);
>>    }
>>    
>> -static void qemu_s390_skeys_init(Object *obj)
>> +static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>    {
>> -    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>> -    MachineState *machine = MACHINE(qdev_get_machine());
>> +    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
>>    
>> -    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
>> -    skeys->keydata = g_malloc0(skeys->key_count);
>> +    /* Lockless check is sufficient. */
>> +    return !!skeys->keydata;
>>    }
>>    
>> -static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>> +static int qemu_s390_skeys_enable(S390SKeysState *ss)
> 
> Could you please call this qemu_s390_skeys_activate() so that it's not so
> easily confused with the ..._enabled() function?

Well, you enable it and you check if it's enabled ... so using different 
terminology is more confusing, no?

I could rename _enabled to is_enabled to make the difference easiert to 
sport here.

> 
>> diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
>> index 2888d42d0b..8b15809716 100644
>> --- a/include/hw/s390x/storage-keys.h
>> +++ b/include/hw/s390x/storage-keys.h
>> @@ -28,9 +28,72 @@ struct S390SKeysState {
>>    
>>    struct S390SKeysClass {
>>        DeviceClass parent_class;
>> +
>> +    /**
>> +     * @skeys_enabled:
>> +     *
>> +     * Check whether storage keys are enabled. If not enabled, they were not
>> +     * enabled lazily either by the guest via a storage key instruction or
>> +     * by the host during migration.
>> +     *
>> +     * If disabled, everything not explicitly triggered by the guest,
>> +     * such as outgoing migration or dirty/change tracking, should not touch
>> +     * storage keys and should not lazily enable it.
>> +     *
>> +     * @ks: the #S390SKeysState
>> +     *
>> +     * Returns 0 if not enabled and 1 if enabled.
>> +     */
>>        int (*skeys_enabled)(S390SKeysState *ks);
>> +
>> +    /**
>> +     * @skeys_enable:
>> +     *
>> +     * Lazily enable storage keys. If this function is not implemented,
>> +     * setting a storage key will lazily enable storage keys implicitly
>> +     * instead. TCG guests have to make sure to flush the TLB of all CPUs
>> +     * if storage keys were not enabled before this call.
>> +     *
>> +     * @ks: the #S390SKeysState
>> +     *
>> +     * Returns 0 if storage keys were not enabled before this call and 1 if
>> +     * they were already enabled.
>> +     */
>> +    int (*skeys_enable)(S390SKeysState *ks);
>> +
>> +    /**
>> +     * @get_skeys:
>> +     *
>> +     * Get storage keys for the given PFN range. This call will fail if
>> +     * storage keys have not been lazily enabled yet.
> 
> Shouldn't there be some modifications to qemu_s390_skeys_get() in that case?
> Or does "fail" mean that it crashes?

qemu_s390_skeys_get() should simply return -EINVAL because 
skeydev->key_count == 0. So don't think we need any modifications.

Makes sense?

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[Thomas Huth]

On 06/08/2021 15.18, David Hildenbrand wrote:
> On 06.08.21 11:42, Thomas Huth wrote:
>> On 05/08/2021 17.28, David Hildenbrand wrote:
>>> Let's enable storage keys lazily under TCG, just as we do under KVM.
>>> Only fairly old Linux versions actually make use of storage keys, so it
>>> can be kind of wasteful to allocate quite some memory and track
>>> changes and references if nobody cares.
>>>
>>> We have to make sure to flush the TLB when enabling storage keys after
>>> the VM was already running: otherwise it might happen that we don't
>>> catch references or modifications afterwards.
>>>
>>> Add proper documentation to all callbacks.
>>>
>>> The kvm-unit-tests skey tests keeps on working with this change.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    hw/s390x/s390-skeys.c           | 51 +++++++++++++++++++++-----
>>>    include/hw/s390x/storage-keys.h | 63 +++++++++++++++++++++++++++++++++
>>>    target/s390x/mmu_helper.c       |  8 +++++
>>>    target/s390x/tcg/mem_helper.c   |  9 +++++
>>>    4 files changed, 123 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>>> index 53e16f1b9c..579bdf1d8a 100644
>>> --- a/hw/s390x/s390-skeys.c
>>> +++ b/hw/s390x/s390-skeys.c
>>> @@ -190,18 +190,45 @@ out:
>>>        fclose(f);
>>>    }
>>> -static void qemu_s390_skeys_init(Object *obj)
>>> +static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>>    {
>>> -    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>>> -    MachineState *machine = MACHINE(qdev_get_machine());
>>> +    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
>>> -    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
>>> -    skeys->keydata = g_malloc0(skeys->key_count);
>>> +    /* Lockless check is sufficient. */
>>> +    return !!skeys->keydata;
>>>    }
>>> -static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>> +static int qemu_s390_skeys_enable(S390SKeysState *ss)
>>
>> Could you please call this qemu_s390_skeys_activate() so that it's not so
>> easily confused with the ..._enabled() function?
> 
> Well, you enable it and you check if it's enabled ... so using different 
> terminology is more confusing, no?
> 
> I could rename _enabled to is_enabled to make the difference easiert to 
> sport here.
> 
>>
>>> diff --git a/include/hw/s390x/storage-keys.h 
>>> b/include/hw/s390x/storage-keys.h
>>> index 2888d42d0b..8b15809716 100644
>>> --- a/include/hw/s390x/storage-keys.h
>>> +++ b/include/hw/s390x/storage-keys.h
>>> @@ -28,9 +28,72 @@ struct S390SKeysState {
>>>    struct S390SKeysClass {
>>>        DeviceClass parent_class;
>>> +
>>> +    /**
>>> +     * @skeys_enabled:
>>> +     *
>>> +     * Check whether storage keys are enabled. If not enabled, they were 
>>> not
>>> +     * enabled lazily either by the guest via a storage key instruction or
>>> +     * by the host during migration.
>>> +     *
>>> +     * If disabled, everything not explicitly triggered by the guest,
>>> +     * such as outgoing migration or dirty/change tracking, should not 
>>> touch
>>> +     * storage keys and should not lazily enable it.
>>> +     *
>>> +     * @ks: the #S390SKeysState
>>> +     *
>>> +     * Returns 0 if not enabled and 1 if enabled.
>>> +     */
>>>        int (*skeys_enabled)(S390SKeysState *ks);
>>> +
>>> +    /**
>>> +     * @skeys_enable:
>>> +     *
>>> +     * Lazily enable storage keys. If this function is not implemented,
>>> +     * setting a storage key will lazily enable storage keys implicitly
>>> +     * instead. TCG guests have to make sure to flush the TLB of all CPUs
>>> +     * if storage keys were not enabled before this call.
>>> +     *
>>> +     * @ks: the #S390SKeysState
>>> +     *
>>> +     * Returns 0 if storage keys were not enabled before this call and 1 if
>>> +     * they were already enabled.
>>> +     */
>>> +    int (*skeys_enable)(S390SKeysState *ks);
>>> +
>>> +    /**
>>> +     * @get_skeys:
>>> +     *
>>> +     * Get storage keys for the given PFN range. This call will fail if
>>> +     * storage keys have not been lazily enabled yet.
>>
>> Shouldn't there be some modifications to qemu_s390_skeys_get() in that case?
>> Or does "fail" mean that it crashes?
> 
> qemu_s390_skeys_get() should simply return -EINVAL because 
> skeydev->key_count == 0. So don't think we need any modifications.
> 
> Makes sense?

Ah, right, make sense, indeed.

  Thomas

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[David Hildenbrand]

>>> Shouldn't there be some modifications to qemu_s390_skeys_get() in that case?
>>> Or does "fail" mean that it crashes?
>>
>> qemu_s390_skeys_get() should simply return -EINVAL because
>> skeydev->key_count == 0. So don't think we need any modifications.
>>
>> Makes sense?
> 
> Ah, right, make sense, indeed.
> 

But I'll still make the clearer and also fixup the error messages that 
are getting printed. Thanks!

>    Thomas
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[Cornelia Huck]

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

> On 06.08.21 11:42, Thomas Huth wrote:
>> On 05/08/2021 17.28, David Hildenbrand wrote:
>>> -static void qemu_s390_skeys_init(Object *obj)
>>> +static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>>    {
>>> -    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>>> -    MachineState *machine = MACHINE(qdev_get_machine());
>>> +    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
>>>    
>>> -    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
>>> -    skeys->keydata = g_malloc0(skeys->key_count);
>>> +    /* Lockless check is sufficient. */
>>> +    return !!skeys->keydata;
>>>    }
>>>    
>>> -static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>> +static int qemu_s390_skeys_enable(S390SKeysState *ss)
>> 
>> Could you please call this qemu_s390_skeys_activate() so that it's not so
>> easily confused with the ..._enabled() function?
>
> Well, you enable it and you check if it's enabled ... so using different 
> terminology is more confusing, no?
>
> I could rename _enabled to is_enabled to make the difference easiert to 
> sport here.

is_enabled certainly is better than just enabled; otherwise, the two
function names are just way too similar.

Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG

[Thomas Huth]

On 06/08/2021 16.13, Cornelia Huck wrote:
> On Fri, Aug 06 2021, David Hildenbrand <david@redhat.com> wrote:
> 
>> On 06.08.21 11:42, Thomas Huth wrote:
>>> On 05/08/2021 17.28, David Hildenbrand wrote:
>>>> -static void qemu_s390_skeys_init(Object *obj)
>>>> +static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>>>     {
>>>> -    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>>>> -    MachineState *machine = MACHINE(qdev_get_machine());
>>>> +    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(ss);
>>>>     
>>>> -    skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
>>>> -    skeys->keydata = g_malloc0(skeys->key_count);
>>>> +    /* Lockless check is sufficient. */
>>>> +    return !!skeys->keydata;
>>>>     }
>>>>     
>>>> -static int qemu_s390_skeys_enabled(S390SKeysState *ss)
>>>> +static int qemu_s390_skeys_enable(S390SKeysState *ss)
>>>
>>> Could you please call this qemu_s390_skeys_activate() so that it's not so
>>> easily confused with the ..._enabled() function?
>>
>> Well, you enable it and you check if it's enabled ... so using different
>> terminology is more confusing, no?
>>
>> I could rename _enabled to is_enabled to make the difference easiert to
>> sport here.
> 
> is_enabled certainly is better than just enabled; otherwise, the two
> function names are just way too similar.
> 

Fine for me, too.

  Thomas