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

[Thomas Huth <thuth@redhat.com>]

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?