qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Thomas Huth
Subject: Re: [PATCH v1 12/12] hw/s390x/s390-skeys: lazy storage key enablement under TCG
Date: Fri, 6 Aug 2021 11:42:52 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0

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?




reply via email to

[Prev in Thread] Current Thread [Next in Thread]