Any access sets the reference bit. In case we have a read-fault, we
should not allow writes to the TLB entry if the change bit was not
already set.

This is a preparation for proper storage-key reference/change bit handling
in TCG and a fix for KVM whereby read accesses would set the change
bit (old KVM versions without the ioctl to carry out the translation).

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

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 227a822e42..ba4b460ac6 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -421,14 +421,28 @@ nodat:
             return 0;
         }
 
-        if (*flags & PAGE_READ) {
-            key |= SK_R;
-        }
-
-        if (*flags & PAGE_WRITE) {
+        switch (rw) {
+        case MMU_DATA_LOAD:
+        case MMU_INST_FETCH:
+            /*
+             * The TLB entry has to remain write-protected on read-faults if
+             * the storage key does not indicate a change already. Otherwise
+             * we might miss setting the change bit on write accesses.
+             */
+            if (!(key & SK_C)) {
+                *flags &= ~PAGE_WRITE;
+            }
+            break;
+        case MMU_DATA_STORE:
             key |= SK_C;
+            break;
+        default:
+            g_assert_not_reached();
         }
 
+        /* Any store/fetch sets the reference bit */
+        key |= SK_R;
+
         r = skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
         if (r) {
             trace_set_skeys_nonzero(r);
-- 
2.21.0

On Mon, 12 Aug 2019 13:27:36 +0200
David Hildenbrand <david@redhat.com> wrote:

> Any access sets the reference bit. In case we have a read-fault, we
> should not allow writes to the TLB entry if the change bit was not
> already set.
> 
> This is a preparation for proper storage-key reference/change bit handling
> in TCG and a fix for KVM whereby read accesses would set the change
> bit (old KVM versions without the ioctl to carry out the translation).

That would be really old kvm versions, right? So no real need to e.g.
cc:stable?

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 227a822e42..ba4b460ac6 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -421,14 +421,28 @@ nodat:
>              return 0;
>          }
>  
> -        if (*flags & PAGE_READ) {
> -            key |= SK_R;
> -        }
> -
> -        if (*flags & PAGE_WRITE) {
> +        switch (rw) {
> +        case MMU_DATA_LOAD:
> +        case MMU_INST_FETCH:
> +            /*
> +             * The TLB entry has to remain write-protected on read-faults if
> +             * the storage key does not indicate a change already. Otherwise
> +             * we might miss setting the change bit on write accesses.
> +             */
> +            if (!(key & SK_C)) {
> +                *flags &= ~PAGE_WRITE;
> +            }
> +            break;
> +        case MMU_DATA_STORE:
>              key |= SK_C;
> +            break;
> +        default:
> +            g_assert_not_reached();
>          }
>  
> +        /* Any store/fetch sets the reference bit */
> +        key |= SK_R;
> +
>          r = skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key);
>          if (r) {
>              trace_set_skeys_nonzero(r);

I've stared at this for quite some time now and have convinced myself
that it looks sane.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

On 13.08.19 16:54, Cornelia Huck wrote:
> On Mon, 12 Aug 2019 13:27:36 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Any access sets the reference bit. In case we have a read-fault, we
>> should not allow writes to the TLB entry if the change bit was not
>> already set.
>>
>> This is a preparation for proper storage-key reference/change bit handling
>> in TCG and a fix for KVM whereby read accesses would set the change
>> bit (old KVM versions without the ioctl to carry out the translation).
> 
> That would be really old kvm versions, right? So no real need to e.g.
> cc:stable?

Yes - nothing a distribution ever supported AFAIK.

-- 

Thanks,

David / dhildenb