Re: [qemu-s390x] [PATCH-for-4.2 v1 5/6] s390x/mmu: Better storage key reference and change bit handling

[Cornelia Huck]

On Mon, 12 Aug 2019 13:27:36 +0200
David Hildenbrand <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>