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

[Thomas Huth <thuth@redhat.com>]

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