[PATCH v4 00/14] Add SEV guest live migration support

[PATCH v4 00/14] Add SEV guest live migration support

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

AMD SEV encrypts the memory of VMs and because this encryption is done using
an address tweak, the hypervisor will not be able to simply copy ciphertext
between machines to migrate a VM. Instead the AMD SEV Key Management API
provides a set of functions which the hypervisor can use to package a
guest encrypted pages for migration, while maintaining the confidentiality
provided by AMD SEV.

The patch series add the support required in Qemu to perform the SEV
guest live migration. Before initiating the live migration a user
should use newly added 'migrate-set-sev-info' command to pass the
target machines certificate chain. See the docs/amd-memory-encryption.txt
for further details.

The complete tree with patch is available at:
https://github.com/AMDESE/qemu/tree/sev_live_migration_v4_1

Changes since v3:
 - Add new ConfidentialGuestMemoryEncryptionOps in 
   ConfidentialGuestSupportClass which will be used for migration of
   encrypted guests.
 - Add support for KVM_HC_MAP_GPA_RANGE hypercall and it's associated
   KVM_EXIT_HYPERCALL exit case which is currently used for SEV
   guest encrypted page status tracking. 
 - Add support for SEV guest encrypted page status tracking
   implemented using shared regions list.
 - Add support for userspace MSR filtering, which is currently used
   for MSR_KVM_MIGRATION_CONTROL for SEV guests to indicate if the
   guest is ready for migration. The KVM arch code calls into SEV
   guest specific code to delete the migrate blocker added during
   SEV_LAUNCH_FINISH.

Changes since v2:
 - Remove direct kvm_memcrpt calls from migration.
 - Add MemoryEcryptionOps in machine which will be used by migration
   instead of kvm_memcrypt calls.
 - drop the RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP. Now the RAM_SAVE_FLAG_ENCRYPTED_PAGE
   can be used for sending bitmap as well as guest RAM encrypted pages
 - add some bound checks on incoming data
 - drop migrate-sev-set-info object
 - extend the migrate-parameters to include the SEV specific certificate fields.
 - multiple fixes based on the review comments from Dave
 
Changes since v1:
 - use the dirty log sync APIs to also sync the page encryption bitmap
   when SEV is active.

Ashish Kalra (4):
  kvm: Add support for SEV shared regions list and KVM_EXIT_HYPERCALL.
  migration/ram: Force encrypted status for flash0 & flash1 devices.
  migration: for SEV live migration bump downtime limit to 1s.
  kvm: Add support for userspace MSR filtering and handling of
    MSR_KVM_MIGRATION_CONTROL.

Brijesh Singh (10):
  doc: update AMD SEV API spec web link
  doc: update AMD SEV to include Live migration flow
  migration.json: add AMD SEV specific migration parameters
  confidential guest support: introduce
    ConfidentialGuestMemoryEncryptionOps for encrypted VMs
  target/i386: sev: provide callback to setup outgoing context
  target/i386: sev: do not create launch context for an incoming guest
  target/i386: sev: add support to encrypt the outgoing page
  target/i386: sev: add support to load incoming encrypted page
  migration: add support to migrate shared regions list
  migration/ram: add support to send encrypted pages

 docs/amd-memory-encryption.txt            |  50 +-
 include/exec/confidential-guest-support.h |  27 +
 include/sysemu/sev.h                      |  15 +
 linux-headers/linux/kvm.h                 |   3 +
 migration/migration.c                     |  65 +++
 migration/migration.h                     |   1 +
 migration/ram.c                           | 170 +++++-
 monitor/hmp-cmds.c                        |  18 +
 qapi/migration.json                       |  40 +-
 target/i386/kvm/kvm.c                     | 107 ++++
 target/i386/sev.c                         | 602 +++++++++++++++++++++-
 target/i386/trace-events                  |   6 +
 12 files changed, 1091 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH v4 01/14] doc: update AMD SEV API spec web link

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 docs/amd-memory-encryption.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
index ffca382b5f..12ca25180e 100644
--- a/docs/amd-memory-encryption.txt
+++ b/docs/amd-memory-encryption.txt
@@ -88,8 +88,8 @@ expects.
 LAUNCH_FINISH finalizes the guest launch and destroys the cryptographic
 context.
 
-See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
-complete flow chart.
+See Secure Encrypted Virtualization Key Management API spec section
+'Launching a guest' usage flow  (Appendix A) for the complete flow chart.
 
 To launch a SEV guest
 
-- 
2.17.1

[PATCH v4 02/14] doc: update AMD SEV to include Live migration flow

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 docs/amd-memory-encryption.txt | 46 +++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
index 12ca25180e..0d9184532a 100644
--- a/docs/amd-memory-encryption.txt
+++ b/docs/amd-memory-encryption.txt
@@ -126,7 +126,51 @@ TODO
 
 Live Migration
 ----------------
-TODO
+AMD SEV encrypts the memory of VMs and because a different key is used
+in each VM, the hypervisor will be unable to simply copy the
+ciphertext from one VM to another to migrate the VM. Instead the AMD SEV Key
+Management API provides sets of function which the hypervisor can use
+to package a guest page for migration, while maintaining the confidentiality
+provided by AMD SEV.
+
+SEV guest VMs have the concept of private and shared memory. The private
+memory is encrypted with the guest-specific key, while shared memory may
+be encrypted with the hypervisor key. The migration APIs provided by the
+SEV API spec should be used for migrating the private pages.
+
+The KVM_HC_MAP_GPA_RANGE hypercall is used by the SEV guest to notify a
+change in the page encryption status to the hypervisor. The hypercall
+is invoked when the encryption attribute is changed from encrypted -> decrypted
+and vice versa. By default all guest pages are considered encrypted.
+
+This hypercall exits to qemu via KVM_EXIT_HYPERCALL to manage the guest
+shared regions and integrate with the qemu's migration code. The shared
+region list can be used to check if the given guest page is private or shared.
+
+Before initiating the migration, we need to know the targets machine's public
+Diffie-Hellman key (PDH) and certificate chain. It can be retrieved
+with the 'query-sev-capabilities' QMP command or using the sev-tool. The
+migrate-set-parameter can be used to pass the target machine's PDH and
+certificate chain.
+
+During the migration flow, the SEND_START is called on the source hypervisor
+to create an outgoing encryption context. The SEV guest policy dictates whether
+the certificate passed through the migrate-sev-set-info command will be
+validated. SEND_UPDATE_DATA is called to encrypt the guest private pages.
+After migration is completed, SEND_FINISH is called to destroy the encryption
+context and make the VM non-runnable to protect it against cloning.
+
+On the target machine, RECEIVE_START is called first to create an
+incoming encryption context. The RECEIVE_UPDATE_DATA is called to copy
+the received encrypted page into guest memory. After migration has
+completed, RECEIVE_FINISH is called to make the VM runnable.
+
+For more information about the migration see SEV API Appendix A
+Usage flow (Live migration section).
+
+NOTE:
+To protect against the memory clone SEV APIs are designed to make the VM
+unrunnable in case of the migration failure.
 
 References
 -----------------
-- 
2.17.1

[PATCH v4 03/14] migration.json: add AMD SEV specific migration parameters

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

AMD SEV migration flow requires that target machine's public Diffie-Hellman
key (PDH) and certificate chain must be passed before initiating the guest
migration. User can use QMP 'migrate-set-parameters' to pass the certificate
chain. The certificate chain will be used while creating the outgoing
encryption context.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 migration/migration.c | 61 +++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c    | 18 +++++++++++++
 qapi/migration.json   | 40 +++++++++++++++++++++++++---
 3 files changed, 116 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 041b8451a6..daea3ecd04 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -907,6 +907,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->announce_rounds = s->parameters.announce_rounds;
     params->has_announce_step = true;
     params->announce_step = s->parameters.announce_step;
+    params->has_sev_pdh = true;
+    params->sev_pdh = g_strdup(s->parameters.sev_pdh);
+    params->has_sev_plat_cert = true;
+    params->sev_plat_cert = g_strdup(s->parameters.sev_plat_cert);
+    params->has_sev_amd_cert = true;
+    params->sev_amd_cert = g_strdup(s->parameters.sev_amd_cert);
 
     if (s->parameters.has_block_bitmap_mapping) {
         params->has_block_bitmap_mapping = true;
@@ -1563,6 +1569,18 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->has_block_bitmap_mapping = true;
         dest->block_bitmap_mapping = params->block_bitmap_mapping;
     }
+    if (params->has_sev_pdh) {
+        assert(params->sev_pdh->type == QTYPE_QSTRING);
+        dest->sev_pdh = g_strdup(params->sev_pdh->u.s);
+    }
+    if (params->has_sev_plat_cert) {
+        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
+        dest->sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
+    }
+    if (params->has_sev_amd_cert) {
+        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
+        dest->sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1685,6 +1703,21 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
             QAPI_CLONE(BitmapMigrationNodeAliasList,
                        params->block_bitmap_mapping);
     }
+    if (params->has_sev_pdh) {
+        g_free(s->parameters.sev_pdh);
+        assert(params->sev_pdh->type == QTYPE_QSTRING);
+        s->parameters.sev_pdh = g_strdup(params->sev_pdh->u.s);
+    }
+    if (params->has_sev_plat_cert) {
+        g_free(s->parameters.sev_plat_cert);
+        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
+        s->parameters.sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
+    }
+    if (params->has_sev_amd_cert) {
+        g_free(s->parameters.sev_amd_cert);
+        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
+        s->parameters.sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
@@ -1705,6 +1738,27 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
         params->tls_hostname->type = QTYPE_QSTRING;
         params->tls_hostname->u.s = strdup("");
     }
+    /* TODO Rewrite "" to null instead */
+    if (params->has_sev_pdh
+        && params->sev_pdh->type == QTYPE_QNULL) {
+        qobject_unref(params->sev_pdh->u.n);
+        params->sev_pdh->type = QTYPE_QSTRING;
+        params->sev_pdh->u.s = strdup("");
+    }
+    /* TODO Rewrite "" to null instead */
+    if (params->has_sev_plat_cert
+        && params->sev_plat_cert->type == QTYPE_QNULL) {
+        qobject_unref(params->sev_plat_cert->u.n);
+        params->sev_plat_cert->type = QTYPE_QSTRING;
+        params->sev_plat_cert->u.s = strdup("");
+    }
+    /* TODO Rewrite "" to null instead */
+    if (params->has_sev_amd_cert
+        && params->sev_amd_cert->type == QTYPE_QNULL) {
+        qobject_unref(params->sev_amd_cert->u.n);
+        params->sev_amd_cert->type = QTYPE_QSTRING;
+        params->sev_amd_cert->u.s = strdup("");
+    }
 
     migrate_params_test_apply(params, &tmp);
 
@@ -4233,6 +4287,9 @@ static void migration_instance_finalize(Object *obj)
     qemu_mutex_destroy(&ms->qemu_file_lock);
     g_free(params->tls_hostname);
     g_free(params->tls_creds);
+    g_free(params->sev_pdh);
+    g_free(params->sev_plat_cert);
+    g_free(params->sev_amd_cert);
     qemu_sem_destroy(&ms->wait_unplug_sem);
     qemu_sem_destroy(&ms->rate_limit_sem);
     qemu_sem_destroy(&ms->pause_sem);
@@ -4280,6 +4337,10 @@ static void migration_instance_init(Object *obj)
     params->has_announce_rounds = true;
     params->has_announce_step = true;
 
+    params->sev_pdh = g_strdup("");
+    params->sev_plat_cert = g_strdup("");
+    params->sev_amd_cert = g_strdup("");
+
     qemu_sem_init(&ms->postcopy_pause_sem, 0);
     qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
     qemu_sem_init(&ms->rp_state.rp_sem, 0);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index e00255f7ee..27ca2024bb 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1399,6 +1399,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         error_setg(&err, "The block-bitmap-mapping parameter can only be set "
                    "through QMP");
         break;
+    case MIGRATION_PARAMETER_SEV_PDH:
+        p->has_sev_pdh = true;
+        p->sev_pdh = g_new0(StrOrNull, 1);
+        p->sev_pdh->type = QTYPE_QSTRING;
+        visit_type_str(v, param, &p->sev_pdh->u.s, &err);
+        break;
+    case MIGRATION_PARAMETER_SEV_PLAT_CERT:
+        p->has_sev_plat_cert = true;
+        p->sev_plat_cert = g_new0(StrOrNull, 1);
+        p->sev_plat_cert->type = QTYPE_QSTRING;
+        visit_type_str(v, param, &p->sev_plat_cert->u.s, &err);
+        break;
+    case MIGRATION_PARAMETER_SEV_AMD_CERT:
+        p->has_sev_amd_cert = true;
+        p->sev_amd_cert = g_new0(StrOrNull, 1);
+        p->sev_amd_cert->type = QTYPE_QSTRING;
+        visit_type_str(v, param, &p->sev_amd_cert->u.s, &err);
+        break;
     default:
         assert(0);
     }
diff --git a/qapi/migration.json b/qapi/migration.json
index 1124a2dda8..69c615ec4d 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -743,6 +743,15 @@
 #                        block device name if there is one, and to their node name
 #                        otherwise. (Since 5.2)
 #
+# @sev-pdh: The target host platform diffie-hellman key encoded in base64
+#           (Since 4.2)
+#
+# @sev-plat-cert: The target host platform certificate chain encoded in base64
+#                 (Since 4.2)
+#
+# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
+#                base64 (Since 4.2)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -758,7 +767,8 @@
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
            'max-cpu-throttle', 'multifd-compression',
            'multifd-zlib-level' ,'multifd-zstd-level',
-           'block-bitmap-mapping' ] }
+           'block-bitmap-mapping',
+           'sev-pdh', 'sev-plat-cert', 'sev-amd-cert' ] }
 
 ##
 # @MigrateSetParameters:
@@ -903,6 +913,15 @@
 #                        block device name if there is one, and to their node name
 #                        otherwise. (Since 5.2)
 #
+# @sev-pdh: The target host platform diffie-hellman key encoded in base64
+#           (Since 4.2)
+#
+# @sev-plat-cert: The target host platform certificate chain encoded in base64
+#                 (Since 4.2)
+#
+# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
+#                base64 (Since 4.2)
+#
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -934,7 +953,10 @@
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
             '*multifd-zstd-level': 'uint8',
-            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+            '*sev-pdh':'StrOrNull',
+            '*sev-plat-cert': 'StrOrNull',
+            '*sev-amd-cert' : 'StrOrNull' } }
 
 ##
 # @migrate-set-parameters:
@@ -1099,6 +1121,15 @@
 #                        block device name if there is one, and to their node name
 #                        otherwise. (Since 5.2)
 #
+# @sev-pdh: The target host platform diffie-hellman key encoded in base64
+#           (Since 4.2)
+#
+# @sev-plat-cert: The target host platform certificate chain encoded in base64
+#                 (Since 4.2)
+#
+# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
+#                base64 (Since 4.2)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -1128,7 +1159,10 @@
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
             '*multifd-zstd-level': 'uint8',
-            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+            '*sev-pdh':'str',
+            '*sev-plat-cert': 'str',
+            '*sev-amd-cert' : 'str'} }
 
 ##
 # @query-migrate-parameters:
-- 
2.17.1

[PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

When memory encryption is enabled in VM, the guest RAM will be encrypted
with the guest-specific key, to protect the confidentiality of data while
in transit we need to platform specific hooks to save or migrate the
guest RAM.

Introduce the new ConfidentialGuestMemoryEncryptionOps in this patch
which will be later used by the encrypted guest for migration.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/exec/confidential-guest-support.h | 27 +++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ba2dd4b5df..d8b4bd4c42 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -20,6 +20,7 @@
 
 #ifndef CONFIG_USER_ONLY
 
+#include <qapi/qapi-types-migration.h>
 #include "qom/object.h"
 
 #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
@@ -53,8 +54,34 @@ struct ConfidentialGuestSupport {
     bool ready;
 };
 
+/**
+ * The functions registers with ConfidentialGuestMemoryEncryptionOps will be
+ * used during the encrypted guest migration.
+ */
+struct ConfidentialGuestMemoryEncryptionOps {
+    /* Initialize the platform specific state before starting the migration */
+    int (*save_setup)(MigrationParameters *p);
+
+    /* Write the encrypted page and metadata associated with it */
+    int (*save_outgoing_page)(QEMUFile *f, uint8_t *ptr, uint32_t size,
+                              uint64_t *bytes_sent);
+
+    /* Load the incoming encrypted page into guest memory */
+    int (*load_incoming_page)(QEMUFile *f, uint8_t *ptr);
+
+    /* Check if gfn is in shared/unencrypted region */
+    bool (*is_gfn_in_unshared_region)(unsigned long gfn);
+
+    /* Write the shared regions list */
+    int (*save_outgoing_shared_regions_list)(QEMUFile *f);
+
+    /* Load the shared regions list */
+    int (*load_incoming_shared_regions_list)(QEMUFile *f);
+};
+
 typedef struct ConfidentialGuestSupportClass {
     ObjectClass parent;
+    struct ConfidentialGuestMemoryEncryptionOps *memory_encryption_ops;
 } ConfidentialGuestSupportClass;
 
 #endif /* !CONFIG_USER_ONLY */
-- 
2.17.1

[PATCH v4 05/14] target/i386: sev: provide callback to setup outgoing context

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

The user provides the target machine's Platform Diffie-Hellman key (PDH)
and certificate chain before starting the SEV guest migration. Cache the
certificate chain as we need them while creating the outgoing context.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h |  2 ++
 target/i386/sev.c    | 61 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 94d821d737..64fc88d3c5 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -14,11 +14,13 @@
 #ifndef QEMU_SEV_H
 #define QEMU_SEV_H
 
+#include <qapi/qapi-types-migration.h>
 #include "sysemu/kvm.h"
 
 bool sev_enabled(void);
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
+int sev_save_setup(MigrationParameters *p);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
                              uint64_t gpa, Error **errp);
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 83df8c09f6..5e7c87764c 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -24,6 +24,7 @@
 #include "qemu/module.h"
 #include "qemu/uuid.h"
 #include "sysemu/kvm.h"
+#include "sysemu/sev.h"
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
@@ -68,6 +69,12 @@ struct SevGuestState {
     int sev_fd;
     SevState state;
     gchar *measurement;
+    guchar *remote_pdh;
+    size_t remote_pdh_len;
+    guchar *remote_plat_cert;
+    size_t remote_plat_cert_len;
+    guchar *amd_cert;
+    size_t amd_cert_len;
 
     uint32_t reset_cs;
     uint32_t reset_ip;
@@ -116,6 +123,12 @@ static const char *const sev_fw_errlist[] = {
 
 #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
 
+#define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB */
+
+static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
+    .save_setup = sev_save_setup,
+};
+
 static int
 sev_ioctl(int fd, int cmd, void *data, int *error)
 {
@@ -772,6 +785,50 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
     }
 }
 
+static inline bool check_blob_length(size_t value)
+{
+    if (value > SEV_FW_BLOB_MAX_SIZE) {
+        error_report("invalid length max=%d got=%ld",
+                     SEV_FW_BLOB_MAX_SIZE, value);
+        return false;
+    }
+
+    return true;
+}
+
+int sev_save_setup(MigrationParameters *p)
+{
+    SevGuestState *s = sev_guest;
+    const char *pdh = p->sev_pdh;
+    const char *plat_cert = p->sev_plat_cert;
+    const char *amd_cert = p->sev_amd_cert;
+
+    s->remote_pdh = g_base64_decode(pdh, &s->remote_pdh_len);
+    if (!check_blob_length(s->remote_pdh_len)) {
+        goto error;
+    }
+
+    s->remote_plat_cert = g_base64_decode(plat_cert,
+                                          &s->remote_plat_cert_len);
+    if (!check_blob_length(s->remote_plat_cert_len)) {
+        goto error;
+    }
+
+    s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
+    if (!check_blob_length(s->amd_cert_len)) {
+        goto error;
+    }
+
+    return 0;
+
+error:
+    g_free(s->remote_pdh);
+    g_free(s->remote_plat_cert);
+    g_free(s->amd_cert);
+
+    return 1;
+}
+
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
     SevGuestState *sev
@@ -781,6 +838,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
+    ConfidentialGuestSupportClass *cgs_class =
+        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(cgs));
 
     if (!sev) {
         return 0;
@@ -870,6 +929,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
 
+    cgs_class->memory_encryption_ops = &sev_memory_encryption_ops;
+
     cgs->ready = true;
 
     return 0;
-- 
2.17.1

[PATCH v4 06/14] target/i386: sev: do not create launch context for an incoming guest

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

The LAUNCH_START is used for creating an encryption context to encrypt
newly created guest, for an incoming guest the RECEIVE_START should be
used.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 target/i386/sev.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 5e7c87764c..10038d3880 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -919,12 +919,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         goto err;
     }
 
-    ret = sev_launch_start(sev);
-    if (ret) {
-        error_setg(errp, "%s: failed to create encryption context", __func__);
-        goto err;
+    /*
+     * The LAUNCH context is used for new guest, if its an incoming guest
+     * then RECEIVE context will be created after the connection is established.
+     */
+    if (!runstate_check(RUN_STATE_INMIGRATE)) {
+        ret = sev_launch_start(sev);
+        if (ret) {
+            error_report("%s: failed to create encryption context", __func__);
+            goto err;
+        }
     }
-
     ram_block_notifier_add(&sev_ram_notifier);
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
-- 
2.17.1

[PATCH v4 07/14] target/i386: sev: add support to encrypt the outgoing page

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

The sev_save_outgoing_page() provide the implementation to encrypt the
guest private pages during the transit. The routines uses the SEND_START
command to create the outgoing encryption context on the first call then
uses the SEND_UPDATE_DATA command to encrypt the data before writing it
to the socket. While encrypting the data SEND_UPDATE_DATA produces some
metadata (e.g MAC, IV). The metadata is also sent to the target machine.
After migration is completed, we issue the SEND_FINISH command to transition
the SEV guest state from sending to unrunnable state.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h     |   2 +
 target/i386/sev.c        | 221 +++++++++++++++++++++++++++++++++++++++
 target/i386/trace-events |   3 +
 3 files changed, 226 insertions(+)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 64fc88d3c5..aa6b91a53e 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -21,6 +21,8 @@ bool sev_enabled(void);
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_save_setup(MigrationParameters *p);
+int sev_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
+                           uint32_t size, uint64_t *bytes_sent);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
                              uint64_t gpa, Error **errp);
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 10038d3880..411bd657e8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -30,6 +30,8 @@
 #include "sysemu/runstate.h"
 #include "trace.h"
 #include "migration/blocker.h"
+#include "migration/qemu-file.h"
+#include "migration/misc.h"
 #include "qom/object.h"
 #include "monitor/monitor.h"
 #include "exec/confidential-guest-support.h"
@@ -75,6 +77,8 @@ struct SevGuestState {
     size_t remote_plat_cert_len;
     guchar *amd_cert;
     size_t amd_cert_len;
+    gchar *send_packet_hdr;
+    size_t send_packet_hdr_len;
 
     uint32_t reset_cs;
     uint32_t reset_ip;
@@ -127,6 +131,7 @@ static const char *const sev_fw_errlist[] = {
 
 static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
     .save_setup = sev_save_setup,
+    .save_outgoing_page = sev_save_outgoing_page,
 };
 
 static int
@@ -829,6 +834,40 @@ error:
     return 1;
 }
 
+static void
+sev_send_finish(void)
+{
+    int ret, error;
+
+    trace_kvm_sev_send_finish();
+    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
+    if (ret) {
+        error_report("%s: SEND_FINISH ret=%d fw_error=%d '%s'",
+                     __func__, ret, error, fw_error_to_str(error));
+    }
+
+    g_free(sev_guest->send_packet_hdr);
+    sev_set_guest_state(sev_guest, SEV_STATE_RUNNING);
+}
+
+static void
+sev_migration_state_notifier(Notifier *notifier, void *data)
+{
+    MigrationState *s = data;
+
+    if (migration_has_finished(s) ||
+        migration_in_postcopy_after_devices(s) ||
+        migration_has_failed(s)) {
+        if (sev_check_state(sev_guest, SEV_STATE_SEND_UPDATE)) {
+            sev_send_finish();
+        }
+    }
+}
+
+static Notifier sev_migration_state_notify = {
+    .notify = sev_migration_state_notifier,
+};
+
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
     SevGuestState *sev
@@ -933,6 +972,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     ram_block_notifier_add(&sev_ram_notifier);
     qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
     qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
+    add_migration_state_change_notifier(&sev_migration_state_notify);
 
     cgs_class->memory_encryption_ops = &sev_memory_encryption_ops;
 
@@ -1143,6 +1183,187 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
     return 0;
 }
 
+static int
+sev_get_send_session_length(void)
+{
+    int ret, fw_err = 0;
+    struct kvm_sev_send_start start = {};
+
+    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_SEND_START, &start, &fw_err);
+    if (fw_err != SEV_RET_INVALID_LEN) {
+        ret = -1;
+        error_report("%s: failed to get session length ret=%d fw_error=%d '%s'",
+                     __func__, ret, fw_err, fw_error_to_str(fw_err));
+        goto err;
+    }
+
+    ret = start.session_len;
+err:
+    return ret;
+}
+
+static int
+sev_send_start(SevGuestState *s, QEMUFile *f, uint64_t *bytes_sent)
+{
+    gsize pdh_len = 0, plat_cert_len;
+    int session_len, ret, fw_error;
+    struct kvm_sev_send_start start = { };
+    guchar *pdh = NULL, *plat_cert = NULL, *session = NULL;
+    Error *local_err = NULL;
+
+    if (!s->remote_pdh || !s->remote_plat_cert || !s->amd_cert_len) {
+        error_report("%s: missing remote PDH or PLAT_CERT", __func__);
+        return 1;
+    }
+
+   start.pdh_cert_uaddr = (uintptr_t) s->remote_pdh;
+   start.pdh_cert_len = s->remote_pdh_len;
+
+   start.plat_certs_uaddr = (uintptr_t)s->remote_plat_cert;
+   start.plat_certs_len = s->remote_plat_cert_len;
+
+   start.amd_certs_uaddr = (uintptr_t)s->amd_cert;
+   start.amd_certs_len = s->amd_cert_len;
+
+    /* get the session length */
+   session_len = sev_get_send_session_length();
+   if (session_len < 0) {
+       ret = 1;
+       goto err;
+   }
+
+   session = g_new0(guchar, session_len);
+   start.session_uaddr = (unsigned long)session;
+   start.session_len = session_len;
+
+   /* Get our PDH certificate */
+   ret = sev_get_pdh_info(s->sev_fd, &pdh, &pdh_len,
+                          &plat_cert, &plat_cert_len, &local_err);
+   if (ret) {
+       error_report("Failed to get our PDH cert");
+       goto err;
+   }
+
+   trace_kvm_sev_send_start(start.pdh_cert_uaddr, start.pdh_cert_len,
+                            start.plat_certs_uaddr, start.plat_certs_len,
+                            start.amd_certs_uaddr, start.amd_certs_len);
+
+   ret = sev_ioctl(s->sev_fd, KVM_SEV_SEND_START, &start, &fw_error);
+   if (ret < 0) {
+       error_report("%s: SEND_START ret=%d fw_error=%d '%s'",
+               __func__, ret, fw_error, fw_error_to_str(fw_error));
+       goto err;
+   }
+
+   qemu_put_be32(f, start.policy);
+   qemu_put_be32(f, pdh_len);
+   qemu_put_buffer(f, (uint8_t *)pdh, pdh_len);
+   qemu_put_be32(f, start.session_len);
+   qemu_put_buffer(f, (uint8_t *)start.session_uaddr, start.session_len);
+   *bytes_sent = 12 + pdh_len + start.session_len;
+
+   sev_set_guest_state(s, SEV_STATE_SEND_UPDATE);
+
+err:
+   g_free(pdh);
+   g_free(plat_cert);
+   return ret;
+}
+
+static int
+sev_send_get_packet_len(int *fw_err)
+{
+    int ret;
+    struct kvm_sev_send_update_data update = {};
+
+    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_SEND_UPDATE_DATA,
+                    &update, fw_err);
+    if (*fw_err != SEV_RET_INVALID_LEN) {
+        ret = -1;
+        error_report("%s: failed to get session length ret=%d fw_error=%d '%s'",
+                    __func__, ret, *fw_err, fw_error_to_str(*fw_err));
+        goto err;
+    }
+
+    ret = update.hdr_len;
+
+err:
+    return ret;
+}
+
+static int
+sev_send_update_data(SevGuestState *s, QEMUFile *f, uint8_t *ptr, uint32_t size,
+                     uint64_t *bytes_sent)
+{
+    int ret, fw_error;
+    guchar *trans;
+    struct kvm_sev_send_update_data update = { };
+
+    /*
+     * If this is first call then query the packet header bytes and allocate
+     * the packet buffer.
+     */
+    if (!s->send_packet_hdr) {
+        s->send_packet_hdr_len = sev_send_get_packet_len(&fw_error);
+        if (s->send_packet_hdr_len < 1) {
+            error_report("%s: SEND_UPDATE fw_error=%d '%s'",
+                         __func__, fw_error, fw_error_to_str(fw_error));
+            return 1;
+        }
+
+        s->send_packet_hdr = g_new(gchar, s->send_packet_hdr_len);
+    }
+
+    /* allocate transport buffer */
+    trans = g_new(guchar, size);
+
+    update.hdr_uaddr = (uintptr_t)s->send_packet_hdr;
+    update.hdr_len = s->send_packet_hdr_len;
+    update.guest_uaddr = (uintptr_t)ptr;
+    update.guest_len = size;
+    update.trans_uaddr = (uintptr_t)trans;
+    update.trans_len = size;
+
+    trace_kvm_sev_send_update_data(ptr, trans, size);
+
+    ret = sev_ioctl(s->sev_fd, KVM_SEV_SEND_UPDATE_DATA, &update, &fw_error);
+    if (ret) {
+        error_report("%s: SEND_UPDATE_DATA ret=%d fw_error=%d '%s'",
+                     __func__, ret, fw_error, fw_error_to_str(fw_error));
+        goto err;
+    }
+
+    qemu_put_be32(f, update.hdr_len);
+    qemu_put_buffer(f, (uint8_t *)update.hdr_uaddr, update.hdr_len);
+    *bytes_sent = 4 + update.hdr_len;
+
+    qemu_put_be32(f, update.trans_len);
+    qemu_put_buffer(f, (uint8_t *)update.trans_uaddr, update.trans_len);
+    *bytes_sent += (4 + update.trans_len);
+
+err:
+    g_free(trans);
+    return ret;
+}
+
+int sev_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
+                           uint32_t sz, uint64_t *bytes_sent)
+{
+    SevGuestState *s = sev_guest;
+
+    /*
+     * If this is a first buffer then create outgoing encryption context
+     * and write our PDH, policy and session data.
+     */
+    if (!sev_check_state(s, SEV_STATE_SEND_UPDATE) &&
+        sev_send_start(s, f, bytes_sent)) {
+        error_report("Failed to create outgoing context");
+        return 1;
+    }
+
+    return sev_send_update_data(s, f, ptr, sz, bytes_sent);
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 2cd8726eeb..e8d4aec125 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -11,3 +11,6 @@ kvm_sev_launch_measurement(const char *value) "data %s"
 kvm_sev_launch_finish(void) ""
 kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
 kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
+kvm_sev_send_start(uint64_t pdh, int l1, uint64_t plat, int l2, uint64_t amd, int l3) "pdh 0x%" PRIx64 " len %d plat 0x%" PRIx64 " len %d amd 0x%" PRIx64 " len %d"
+kvm_sev_send_update_data(void *src, void *dst, int len) "guest %p trans %p len %d"
+kvm_sev_send_finish(void) ""
-- 
2.17.1

[PATCH v4 08/14] target/i386: sev: add support to load incoming encrypted page

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

The sev_load_incoming_page() provide the implementation to read the
incoming guest private pages from the socket and load it into the guest
memory. The routines uses the RECEIVE_START command to create the
incoming encryption context on the first call then uses the
RECEIEVE_UPDATE_DATA command to load the encrypted pages into the guest
memory. After migration is completed, we issue the RECEIVE_FINISH command
to transition the SEV guest to the runnable state so that it can be
executed.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h     |   1 +
 target/i386/sev.c        | 137 ++++++++++++++++++++++++++++++++++++++-
 target/i386/trace-events |   3 +
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index aa6b91a53e..faa02bdd3d 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -23,6 +23,7 @@ int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
 int sev_save_setup(MigrationParameters *p);
 int sev_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
                            uint32_t size, uint64_t *bytes_sent);
+int sev_load_incoming_page(QEMUFile *f, uint8_t *ptr);
 int sev_inject_launch_secret(const char *hdr, const char *secret,
                              uint64_t gpa, Error **errp);
 
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 411bd657e8..1901c9ade4 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -132,6 +132,7 @@ static const char *const sev_fw_errlist[] = {
 static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
     .save_setup = sev_save_setup,
     .save_outgoing_page = sev_save_outgoing_page,
+    .load_incoming_page = sev_load_incoming_page,
 };
 
 static int
@@ -778,13 +779,33 @@ sev_launch_finish(SevGuestState *sev)
     }
 }
 
+static int
+sev_receive_finish(SevGuestState *s)
+{
+    int error, ret = 1;
+
+    trace_kvm_sev_receive_finish();
+    ret = sev_ioctl(s->sev_fd, KVM_SEV_RECEIVE_FINISH, 0, &error);
+    if (ret) {
+        error_report("%s: RECEIVE_FINISH ret=%d fw_error=%d '%s'",
+                     __func__, ret, error, fw_error_to_str(error));
+        goto err;
+    }
+
+    sev_set_guest_state(s, SEV_STATE_RUNNING);
+err:
+    return ret;
+}
+
 static void
 sev_vm_state_change(void *opaque, bool running, RunState state)
 {
     SevGuestState *sev = opaque;
 
     if (running) {
-        if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
+        if (sev_check_state(sev, SEV_STATE_RECEIVE_UPDATE)) {
+            sev_receive_finish(sev);
+        } else if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
             sev_launch_finish(sev);
         }
     }
@@ -1364,6 +1385,120 @@ int sev_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
     return sev_send_update_data(s, f, ptr, sz, bytes_sent);
 }
 
+static int
+sev_receive_start(SevGuestState *sev, QEMUFile *f)
+{
+    int ret = 1;
+    int fw_error;
+    struct kvm_sev_receive_start start = { };
+    gchar *session = NULL, *pdh_cert = NULL;
+
+    /* get SEV guest handle */
+    start.handle = object_property_get_int(OBJECT(sev), "handle",
+                                           &error_abort);
+
+    /* get the source policy */
+    start.policy = qemu_get_be32(f);
+
+    /* get source PDH key */
+    start.pdh_len = qemu_get_be32(f);
+    if (!check_blob_length(start.pdh_len)) {
+        return 1;
+    }
+
+    pdh_cert = g_new(gchar, start.pdh_len);
+    qemu_get_buffer(f, (uint8_t *)pdh_cert, start.pdh_len);
+    start.pdh_uaddr = (uintptr_t)pdh_cert;
+
+    /* get source session data */
+    start.session_len = qemu_get_be32(f);
+    if (!check_blob_length(start.session_len)) {
+        return 1;
+    }
+    session = g_new(gchar, start.session_len);
+    qemu_get_buffer(f, (uint8_t *)session, start.session_len);
+    start.session_uaddr = (uintptr_t)session;
+
+    trace_kvm_sev_receive_start(start.policy, session, pdh_cert);
+
+    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_RECEIVE_START,
+                    &start, &fw_error);
+    if (ret < 0) {
+        error_report("Error RECEIVE_START ret=%d fw_error=%d '%s'",
+                      ret, fw_error, fw_error_to_str(fw_error));
+        goto err;
+    }
+
+    object_property_set_int(OBJECT(sev), "handle", start.handle, &error_abort);
+    sev_set_guest_state(sev, SEV_STATE_RECEIVE_UPDATE);
+err:
+    g_free(session);
+    g_free(pdh_cert);
+
+    return ret;
+}
+
+static int sev_receive_update_data(QEMUFile *f, uint8_t *ptr)
+{
+    int ret = 1, fw_error = 0;
+    gchar *hdr = NULL, *trans = NULL;
+    struct kvm_sev_receive_update_data update = {};
+
+    /* get packet header */
+    update.hdr_len = qemu_get_be32(f);
+    if (!check_blob_length(update.hdr_len)) {
+        return 1;
+    }
+
+    hdr = g_new(gchar, update.hdr_len);
+    qemu_get_buffer(f, (uint8_t *)hdr, update.hdr_len);
+    update.hdr_uaddr = (uintptr_t)hdr;
+
+    /* get transport buffer */
+    update.trans_len = qemu_get_be32(f);
+    if (!check_blob_length(update.trans_len)) {
+        goto err;
+    }
+
+    trans = g_new(gchar, update.trans_len);
+    update.trans_uaddr = (uintptr_t)trans;
+    qemu_get_buffer(f, (uint8_t *)update.trans_uaddr, update.trans_len);
+
+    update.guest_uaddr = (uintptr_t) ptr;
+    update.guest_len = update.trans_len;
+
+    trace_kvm_sev_receive_update_data(trans, ptr, update.guest_len,
+            hdr, update.hdr_len);
+
+    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_RECEIVE_UPDATE_DATA,
+                    &update, &fw_error);
+    if (ret) {
+        error_report("Error RECEIVE_UPDATE_DATA ret=%d fw_error=%d '%s'",
+                     ret, fw_error, fw_error_to_str(fw_error));
+        goto err;
+    }
+err:
+    g_free(trans);
+    g_free(hdr);
+    return ret;
+}
+
+int sev_load_incoming_page(QEMUFile *f, uint8_t *ptr)
+{
+    SevGuestState *s = sev_guest;
+
+    /*
+     * If this is first buffer and SEV is not in recieiving state then
+     * use RECEIVE_START command to create a encryption context.
+     */
+    if (!sev_check_state(s, SEV_STATE_RECEIVE_UPDATE) &&
+        sev_receive_start(s, f)) {
+        return 1;
+    }
+
+    return sev_receive_update_data(f, ptr);
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/trace-events b/target/i386/trace-events
index e8d4aec125..475de65ad4 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -14,3 +14,6 @@ kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data
 kvm_sev_send_start(uint64_t pdh, int l1, uint64_t plat, int l2, uint64_t amd, int l3) "pdh 0x%" PRIx64 " len %d plat 0x%" PRIx64 " len %d amd 0x%" PRIx64 " len %d"
 kvm_sev_send_update_data(void *src, void *dst, int len) "guest %p trans %p len %d"
 kvm_sev_send_finish(void) ""
+kvm_sev_receive_start(int policy, void *session, void *pdh) "policy 0x%x session %p pdh %p"
+kvm_sev_receive_update_data(void *src, void *dst, int len, void *hdr, int hdr_len) "guest %p trans %p len %d hdr %p hdr_len %d"
+kvm_sev_receive_finish(void) ""
-- 
2.17.1

[PATCH v4 09/14] kvm: Add support for SEV shared regions list and KVM_EXIT_HYPERCALL.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

KVM_HC_MAP_GPA_RANGE hypercall is used by the SEV guest to notify a
change in the page encryption status to the hypervisor. The hypercall
should be invoked only when the encryption attribute is changed from
encrypted -> decrypted and vice versa. By default all guest pages are
considered encrypted.

The hypercall exits to userspace with KVM_EXIT_HYPERCALL exit code,
currently this is used only by SEV guests for guest page encryptiion
status tracking. Add support to handle this exit and invoke SEV
shared regions list handlers.

Add support for SEV guest shared regions and implementation of the
SEV shared regions list.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h      |   3 ++
 linux-headers/linux/kvm.h |   3 ++
 target/i386/kvm/kvm.c     |  46 +++++++++++++++++
 target/i386/sev.c         | 105 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 157 insertions(+)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index faa02bdd3d..3b913518c0 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -29,5 +29,8 @@ int sev_inject_launch_secret(const char *hdr, const char *secret,
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size);
 void sev_es_set_reset_vector(CPUState *cpu);
+int sev_remove_shared_regions_list(unsigned long gfn_start,
+                                   unsigned long gfn_end);
+int sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
 
 #endif
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index bcaf66cc4d..78874f4d43 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -343,6 +343,7 @@ struct kvm_run {
 		} mmio;
 		/* KVM_EXIT_HYPERCALL */
 		struct {
+#define KVM_HC_MAP_GPA_RANGE 12
 			__u64 nr;
 			__u64 args[6];
 			__u64 ret;
@@ -1113,6 +1114,8 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
 
+#define KVM_EXIT_HYPERCALL_VALID_MASK (1 << KVM_HC_MAP_GPA_RANGE)
+
 #ifdef KVM_CAP_IRQ_ROUTING
 
 struct kvm_irq_routing_irqchip {
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index e69abe48e3..303722e06f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -125,6 +125,7 @@ static int has_xsave;
 static int has_xcrs;
 static int has_pit_state2;
 static int has_exception_payload;
+static int has_map_gpa_range;
 
 static bool has_msr_mcg_ext_ctl;
 
@@ -1916,6 +1917,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
         c->eax = MAX(c->eax, KVM_CPUID_SIGNATURE | 0x10);
     }
 
+    if (sev_enabled()) {
+        c = cpuid_find_entry(&cpuid_data.cpuid,
+                             KVM_CPUID_FEATURES | kvm_base, 0);
+        c->eax |= (1 << KVM_FEATURE_MIGRATION_CONTROL);
+        if (has_map_gpa_range) {
+            c->eax |= (1 << KVM_FEATURE_HC_MAP_GPA_RANGE);
+        }
+    }
+
     cpuid_data.cpuid.nent = cpuid_i;
 
     cpuid_data.cpuid.padding = 0;
@@ -2277,6 +2287,17 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    has_map_gpa_range = kvm_check_extension(s, KVM_CAP_EXIT_HYPERCALL);
+    if (has_map_gpa_range) {
+        ret = kvm_vm_enable_cap(s, KVM_CAP_EXIT_HYPERCALL, 0,
+                                KVM_EXIT_HYPERCALL_VALID_MASK);
+        if (ret < 0) {
+            error_report("kvm: Failed to enable MAP_GPA_RANGE cap: %s",
+                         strerror(-ret));
+            return ret;
+        }
+    }
+
     ret = kvm_get_supported_msrs(s);
     if (ret < 0) {
         return ret;
@@ -4429,6 +4450,28 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
     return 1;
 }
 
+static int kvm_handle_exit_hypercall(X86CPU *cpu, struct kvm_run *run)
+{
+    /*
+     * Currently this exit is only used by SEV guests for
+     * guest page encryption status tracking.
+     */
+    if (run->hypercall.nr == KVM_HC_MAP_GPA_RANGE) {
+        unsigned long enc = run->hypercall.args[2];
+        unsigned long gpa = run->hypercall.args[0];
+        unsigned long npages = run->hypercall.args[1];
+        unsigned long gfn_start = gpa >> TARGET_PAGE_BITS;
+        unsigned long gfn_end = gfn_start + npages;
+
+        if (enc) {
+            sev_remove_shared_regions_list(gfn_start, gfn_end);
+         } else {
+            sev_add_shared_regions_list(gfn_start, gfn_end);
+         }
+    }
+    return 0;
+}
+
 int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
 {
     static const uint8_t int3 = 0xcc;
@@ -4690,6 +4733,9 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
         /* already handled in kvm_arch_post_run */
         ret = 0;
         break;
+    case KVM_EXIT_HYPERCALL:
+        ret = kvm_handle_exit_hypercall(cpu, run);
+        break;
     default:
         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
         ret = -1;
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 1901c9ade4..6d44b7ad21 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -40,6 +40,10 @@
 #define TYPE_SEV_GUEST "sev-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
 
+struct shared_region {
+    unsigned long gfn_start, gfn_end;
+    QTAILQ_ENTRY(shared_region) list;
+};
 
 /**
  * SevGuestState:
@@ -83,6 +87,8 @@ struct SevGuestState {
     uint32_t reset_cs;
     uint32_t reset_ip;
     bool reset_data_valid;
+
+    QTAILQ_HEAD(, shared_region) shared_regions_list;
 };
 
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
@@ -996,6 +1002,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     add_migration_state_change_notifier(&sev_migration_state_notify);
 
     cgs_class->memory_encryption_ops = &sev_memory_encryption_ops;
+    QTAILQ_INIT(&sev->shared_regions_list);
 
     cgs->ready = true;
 
@@ -1499,6 +1506,104 @@ int sev_load_incoming_page(QEMUFile *f, uint8_t *ptr)
     return sev_receive_update_data(f, ptr);
 }
 
+int sev_remove_shared_regions_list(unsigned long start, unsigned long end)
+{
+    SevGuestState *s = sev_guest;
+    struct shared_region *pos;
+
+    QTAILQ_FOREACH(pos, &s->shared_regions_list, list) {
+        unsigned long l, r;
+        unsigned long curr_gfn_end = pos->gfn_end;
+
+        /*
+         * Find if any intersection exists ?
+         * left bound for intersecting segment
+         */
+        l = MAX(start, pos->gfn_start);
+        /* right bound for intersecting segment */
+        r = MIN(end, pos->gfn_end);
+        if (l <= r) {
+            if (pos->gfn_start == l && pos->gfn_end == r) {
+                QTAILQ_REMOVE(&s->shared_regions_list, pos, list);
+            } else if (l == pos->gfn_start) {
+                pos->gfn_start = r;
+            } else if (r == pos->gfn_end) {
+                pos->gfn_end = l;
+            } else {
+                /* Do a de-merge -- split linked list nodes */
+                struct shared_region *shrd_region;
+
+                pos->gfn_end = l;
+                shrd_region = g_malloc0(sizeof(*shrd_region));
+                if (!shrd_region) {
+                    return 0;
+                }
+                shrd_region->gfn_start = r;
+                shrd_region->gfn_end = curr_gfn_end;
+                QTAILQ_INSERT_AFTER(&s->shared_regions_list, pos,
+                                    shrd_region, list);
+            }
+        }
+        if (end <= curr_gfn_end) {
+            break;
+        }
+    }
+    return 0;
+}
+
+int sev_add_shared_regions_list(unsigned long start, unsigned long end)
+{
+    struct shared_region *shrd_region;
+    struct shared_region *pos;
+    SevGuestState *s = sev_guest;
+
+    if (QTAILQ_EMPTY(&s->shared_regions_list)) {
+        shrd_region = g_malloc0(sizeof(*shrd_region));
+        if (!shrd_region) {
+            return -1;
+        }
+        shrd_region->gfn_start = start;
+        shrd_region->gfn_end = end;
+        QTAILQ_INSERT_TAIL(&s->shared_regions_list, shrd_region, list);
+        return 0;
+    }
+
+    /*
+     * shared regions list is a sorted list in ascending order
+     * of guest PA's and also merges consecutive range of guest PA's
+     */
+    QTAILQ_FOREACH(pos, &s->shared_regions_list, list) {
+        /* handle duplicate overlapping regions */
+        if (start >= pos->gfn_start && end <= pos->gfn_end) {
+            return 0;
+        }
+        if (pos->gfn_end < start) {
+            continue;
+        }
+        /* merge consecutive guest PA(s) -- forward merge */
+        if (pos->gfn_start <= start && pos->gfn_end >= start) {
+            pos->gfn_end = end;
+            return 0;
+        }
+        break;
+    }
+    /*
+     * Add a new node
+     */
+    shrd_region = g_malloc0(sizeof(*shrd_region));
+    if (!shrd_region) {
+        return -1;
+    }
+    shrd_region->gfn_start = start;
+    shrd_region->gfn_end = end;
+    if (pos) {
+        QTAILQ_INSERT_BEFORE(pos, shrd_region, list);
+    } else {
+        QTAILQ_INSERT_TAIL(&s->shared_regions_list, shrd_region, list);
+    }
+    return 1;
+}
+
 static void
 sev_register_types(void)
 {
-- 
2.17.1

[PATCH v4 10/14] migration: add support to migrate shared regions list

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

When memory encryption is enabled, the hypervisor maintains a shared
regions list which is referred by hypervisor during migration to check
if page is private or shared. This list is built during the VM bootup and
must be migrated to the target host so that hypervisor on target host can
use it for future migration.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h |  2 ++
 target/i386/sev.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 3b913518c0..118ee66406 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -32,5 +32,7 @@ void sev_es_set_reset_vector(CPUState *cpu);
 int sev_remove_shared_regions_list(unsigned long gfn_start,
                                    unsigned long gfn_end);
 int sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
+int sev_save_outgoing_shared_regions_list(QEMUFile *f);
+int sev_load_incoming_shared_regions_list(QEMUFile *f);
 
 #endif
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6d44b7ad21..789051f7b4 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -135,10 +135,15 @@ static const char *const sev_fw_errlist[] = {
 
 #define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB */
 
+#define SHARED_REGION_LIST_CONT     0x1
+#define SHARED_REGION_LIST_END      0x2
+
 static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
     .save_setup = sev_save_setup,
     .save_outgoing_page = sev_save_outgoing_page,
     .load_incoming_page = sev_load_incoming_page,
+    .save_outgoing_shared_regions_list = sev_save_outgoing_shared_regions_list,
+    .load_incoming_shared_regions_list = sev_load_incoming_shared_regions_list,
 };
 
 static int
@@ -1604,6 +1609,44 @@ int sev_add_shared_regions_list(unsigned long start, unsigned long end)
     return 1;
 }
 
+int sev_save_outgoing_shared_regions_list(QEMUFile *f)
+{
+    SevGuestState *s = sev_guest;
+    struct shared_region *pos;
+
+    QTAILQ_FOREACH(pos, &s->shared_regions_list, list) {
+        qemu_put_be32(f, SHARED_REGION_LIST_CONT);
+        qemu_put_be32(f, pos->gfn_start);
+        qemu_put_be32(f, pos->gfn_end);
+    }
+
+    qemu_put_be32(f, SHARED_REGION_LIST_END);
+    return 0;
+}
+
+int sev_load_incoming_shared_regions_list(QEMUFile *f)
+{
+    SevGuestState *s = sev_guest;
+    struct shared_region *shrd_region;
+    int status;
+
+    status = qemu_get_be32(f);
+    while (status == SHARED_REGION_LIST_CONT) {
+
+        shrd_region = g_malloc0(sizeof(*shrd_region));
+        if (!shrd_region) {
+            return 0;
+        }
+        shrd_region->gfn_start = qemu_get_be32(f);
+        shrd_region->gfn_end = qemu_get_be32(f);
+
+        QTAILQ_INSERT_TAIL(&s->shared_regions_list, shrd_region, list);
+
+        status = qemu_get_be32(f);
+    }
+    return 0;
+}
+
 static void
 sev_register_types(void)
 {
-- 
2.17.1

[PATCH v4 11/14] migration/ram: add support to send encrypted pages

[Ashish Kalra]

From: Brijesh Singh <brijesh.singh@amd.com>

When memory encryption is enabled, the guest memory will be encrypted with
the guest specific key. The patch introduces RAM_SAVE_FLAG_ENCRYPTED_PAGE
flag to distinguish the encrypted data from plaintext. Encrypted pages
may need special handling. The sev_save_outgoing_page() is used
by the sender to write the encrypted pages onto the socket, similarly the
sev_load_incoming_page() is used by the target to read the
encrypted pages from the socket and load into the guest memory.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h  |   4 ++
 migration/migration.h |   1 +
 migration/ram.c       | 162 +++++++++++++++++++++++++++++++++++++++++-
 target/i386/sev.c     |  14 ++++
 4 files changed, 180 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 118ee66406..023e694ac4 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -17,6 +17,9 @@
 #include <qapi/qapi-types-migration.h>
 #include "sysemu/kvm.h"
 
+#define RAM_SAVE_ENCRYPTED_PAGE           0x1
+#define RAM_SAVE_SHARED_REGIONS_LIST      0x2
+
 bool sev_enabled(void);
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
@@ -34,5 +37,6 @@ int sev_remove_shared_regions_list(unsigned long gfn_start,
 int sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
 int sev_save_outgoing_shared_regions_list(QEMUFile *f);
 int sev_load_incoming_shared_regions_list(QEMUFile *f);
+bool sev_is_gfn_in_unshared_region(unsigned long gfn);
 
 #endif
diff --git a/migration/migration.h b/migration/migration.h
index 7a5aa8c2fd..eda07e214d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -391,5 +391,6 @@ bool migration_rate_limit(void);
 void migration_cancel(void);
 
 void populate_vfio_info(MigrationInfo *info);
+bool memcrypt_enabled(void);
 
 #endif
diff --git a/migration/ram.c b/migration/ram.c
index 7a43bfd7af..1cb8d57a89 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -55,6 +55,11 @@
 #include "qemu/iov.h"
 #include "multifd.h"
 #include "sysemu/runstate.h"
+#include "hw/boards.h"
+#include "exec/confidential-guest-support.h"
+
+/* Defines RAM_SAVE_ENCRYPTED_PAGE and RAM_SAVE_SHARED_REGION_LIST */
+#include "sysemu/sev.h"
 
 #if defined(__linux__)
 #include "qemu/userfaultfd.h"
@@ -78,12 +83,20 @@
 #define RAM_SAVE_FLAG_XBZRLE   0x40
 /* 0x80 is reserved in migration.h start with 0x100 next */
 #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
+#define RAM_SAVE_FLAG_ENCRYPTED_DATA   0x200
 
 static inline bool is_zero_range(uint8_t *p, uint64_t size)
 {
     return buffer_is_zero(p, size);
 }
 
+bool memcrypt_enabled(void)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+
+    return ms->cgs->ready;
+}
+
 XBZRLECacheStats xbzrle_counters;
 
 /* struct contains XBZRLE cache and a static page
@@ -449,6 +462,8 @@ static QemuCond decomp_done_cond;
 
 static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                  ram_addr_t offset, uint8_t *source_buf);
+static int ram_save_encrypted_page(RAMState *rs, PageSearchStatus *pss,
+                                   bool last_stage);
 
 static void *do_data_compress(void *opaque)
 {
@@ -1165,6 +1180,80 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
     return 1;
 }
 
+/**
+ * ram_save_encrypted_page - send the given encrypted page to the stream
+ */
+static int ram_save_encrypted_page(RAMState *rs, PageSearchStatus *pss,
+                                   bool last_stage)
+{
+    int ret;
+    uint8_t *p;
+    RAMBlock *block = pss->block;
+    ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
+    uint64_t bytes_xmit;
+    MachineState *ms = MACHINE(qdev_get_machine());
+    ConfidentialGuestSupportClass *cgs_class =
+        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(ms->cgs));
+    struct ConfidentialGuestMemoryEncryptionOps *ops =
+        cgs_class->memory_encryption_ops;
+
+    p = block->host + offset;
+
+    ram_counters.transferred +=
+        save_page_header(rs, rs->f, block,
+                    offset | RAM_SAVE_FLAG_ENCRYPTED_DATA);
+
+    qemu_put_be32(rs->f, RAM_SAVE_ENCRYPTED_PAGE);
+    ret = ops->save_outgoing_page(rs->f, p, TARGET_PAGE_SIZE, &bytes_xmit);
+    if (ret) {
+        return -1;
+    }
+
+    ram_counters.transferred += bytes_xmit;
+    ram_counters.normal++;
+
+    return 1;
+}
+
+/**
+ * ram_save_shared_region_list: send the shared region list
+ */
+static int ram_save_shared_region_list(RAMState *rs, QEMUFile *f)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    ConfidentialGuestSupportClass *cgs_class =
+        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(ms->cgs));
+    struct ConfidentialGuestMemoryEncryptionOps *ops =
+        cgs_class->memory_encryption_ops;
+
+    save_page_header(rs, rs->f, rs->last_seen_block,
+                     RAM_SAVE_FLAG_ENCRYPTED_DATA);
+    qemu_put_be32(rs->f, RAM_SAVE_SHARED_REGIONS_LIST);
+    return ops->save_outgoing_shared_regions_list(rs->f);
+}
+
+static int load_encrypted_data(QEMUFile *f, uint8_t *ptr)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    ConfidentialGuestSupportClass *cgs_class =
+        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(ms->cgs));
+    struct ConfidentialGuestMemoryEncryptionOps *ops =
+        cgs_class->memory_encryption_ops;
+
+    int flag;
+
+    flag = qemu_get_be32(f);
+
+    if (flag == RAM_SAVE_ENCRYPTED_PAGE) {
+        return ops->load_incoming_page(f, ptr);
+    } else if (flag == RAM_SAVE_SHARED_REGIONS_LIST) {
+        return ops->load_incoming_shared_regions_list(f);
+    } else {
+        error_report("unknown encrypted flag %x", flag);
+        return 1;
+    }
+}
+
 /**
  * ram_save_page: send the given page to the stream
  *
@@ -1965,6 +2054,35 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
     return false;
 }
 
+/**
+ * encrypted_test_list: check if the page is encrypted
+ *
+ * Returns a bool indicating whether the page is encrypted.
+ */
+static bool encrypted_test_list(RAMState *rs, RAMBlock *block,
+                                  unsigned long page)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    ConfidentialGuestSupportClass *cgs_class =
+        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(ms->cgs));
+    struct ConfidentialGuestMemoryEncryptionOps *ops =
+        cgs_class->memory_encryption_ops;
+    unsigned long gfn;
+
+    /* ROM devices contains the unencrypted data */
+    if (memory_region_is_rom(block->mr)) {
+        return false;
+    }
+
+    /*
+     * Translate page in ram_addr_t address space to GPA address
+     * space using memory region.
+     */
+    gfn = page + (block->mr->addr >> TARGET_PAGE_BITS);
+
+    return ops->is_gfn_in_unshared_region(gfn);
+}
+
 /**
  * ram_save_target_page: save one target page
  *
@@ -1985,6 +2103,17 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
         return res;
     }
 
+    /*
+     * If memory encryption is enabled then use memory encryption APIs
+     * to write the outgoing buffer to the wire. The encryption APIs
+     * will take care of accessing the guest memory and re-encrypt it
+     * for the transport purposes.
+     */
+    if (memcrypt_enabled() &&
+        encrypted_test_list(rs, pss->block, pss->page)) {
+        return ram_save_encrypted_page(rs, pss, last_stage);
+    }
+
     if (save_compress_page(rs, block, offset)) {
         return 1;
     }
@@ -2786,6 +2915,18 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
     }
 }
 
+static int ram_encrypted_save_setup(void)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    ConfidentialGuestSupportClass *cgs_class =
+        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(ms->cgs));
+    struct ConfidentialGuestMemoryEncryptionOps *ops =
+        cgs_class->memory_encryption_ops;
+    MigrationParameters *p = &migrate_get_current()->parameters;
+
+    return ops->save_setup(p);
+}
+
 /*
  * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
  * long-running RCU critical section.  When rcu-reclaims in the code
@@ -2820,6 +2961,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     (*rsp)->f = f;
 
     WITH_RCU_READ_LOCK_GUARD() {
+
+        if (memcrypt_enabled()) {
+            if (ram_encrypted_save_setup()) {
+                return -1;
+            }
+        }
+
         qemu_put_be64(f, ram_bytes_total_common(true) | RAM_SAVE_FLAG_MEM_SIZE);
 
         RAMBLOCK_FOREACH_MIGRATABLE(block) {
@@ -3004,6 +3152,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 
         flush_compressed_data(rs);
         ram_control_after_iterate(f, RAM_CONTROL_FINISH);
+
+        /* send the shared regions list */
+        if (memcrypt_enabled()) {
+            ret = ram_save_shared_region_list(rs, f);
+        }
     }
 
     if (ret >= 0) {
@@ -3808,7 +3961,8 @@ static int ram_load_precopy(QEMUFile *f)
         }
 
         if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
-                     RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
+                     RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE |
+                     RAM_SAVE_FLAG_ENCRYPTED_DATA)) {
             RAMBlock *block = ram_block_from_stream(f, flags);
 
             host = host_from_ram_block_offset(block, addr);
@@ -3937,6 +4091,12 @@ static int ram_load_precopy(QEMUFile *f)
                 break;
             }
             break;
+        case RAM_SAVE_FLAG_ENCRYPTED_DATA:
+            if (load_encrypted_data(f, host)) {
+                    error_report("Failed to load encrypted data");
+                    ret = -EINVAL;
+            }
+            break;
         case RAM_SAVE_FLAG_EOS:
             /* normal exit */
             multifd_recv_sync_main();
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 789051f7b4..d22f2ef6dc 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -142,6 +142,7 @@ static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
     .save_setup = sev_save_setup,
     .save_outgoing_page = sev_save_outgoing_page,
     .load_incoming_page = sev_load_incoming_page,
+    .is_gfn_in_unshared_region = sev_is_gfn_in_unshared_region,
     .save_outgoing_shared_regions_list = sev_save_outgoing_shared_regions_list,
     .load_incoming_shared_regions_list = sev_load_incoming_shared_regions_list,
 };
@@ -1647,6 +1648,19 @@ int sev_load_incoming_shared_regions_list(QEMUFile *f)
     return 0;
 }
 
+bool sev_is_gfn_in_unshared_region(unsigned long gfn)
+{
+    SevGuestState *s = sev_guest;
+    struct shared_region *pos;
+
+    QTAILQ_FOREACH(pos, &s->shared_regions_list, list) {
+        if (gfn >= pos->gfn_start && gfn < pos->gfn_end) {
+            return false;
+        }
+    }
+    return true;
+}
+
 static void
 sev_register_types(void)
 {
-- 
2.17.1

[PATCH v4 12/14] migration/ram: Force encrypted status for flash0 & flash1 devices.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Currently OVMF clears the C-bit and marks NonExistent memory space
as decrypted in the page encryption bitmap. By marking the
NonExistent memory space as decrypted it gurantees any future MMIO adds
will work correctly, but this marks flash0 device space as decrypted.
At reset the SEV core will be in forced encrypted state, so this
decrypted marking of flash0 device space will cause VCPU reset to fail
as flash0 device pages will be migrated incorrectly.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 migration/ram.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 1cb8d57a89..4eca90cceb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2074,6 +2074,14 @@ static bool encrypted_test_list(RAMState *rs, RAMBlock *block,
         return false;
     }
 
+    if (!strcmp(memory_region_name(block->mr), "system.flash0")) {
+        return true;
+    }
+
+    if (!strcmp(memory_region_name(block->mr), "system.flash1")) {
+        return false;
+    }
+
     /*
      * Translate page in ram_addr_t address space to GPA address
      * space using memory region.
-- 
2.17.1

[PATCH v4 13/14] migration: for SEV live migration bump downtime limit to 1s.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Now, qemu has a default expected downtime of 300 ms and
SEV Live migration has a page-per-second bandwidth of 350-450 pages
( SEV Live migration being generally slow due to guest RAM pages
being migrated after encryption using the security processor ).
With this expected downtime of 300ms and 350-450 pps bandwith,
the threshold size = <1/3 of the PPS bandwidth = ~100 pages.

Now, this threshold size is the maximum pages/bytes that can be
sent in the final completion phase of Live migration
(where the source VM is stopped) with the expected downtime.
Therefore, with the threshold size computed above,
the migration completion phase which halts the source VM
and then transfers the leftover dirty pages,
is only reached in SEV live migration case when # of dirty pages are ~100.

The dirty-pages-rate with larger guest RAM configuration like 4G, 8G, etc.
is much higher, typically in the range of 300-400+ pages, hence,
we always remain in the "dirty-sync" phase of migration and never
reach the migration completion phase with above guest RAM configs.

To summarize, with larger guest RAM configs,
the dirty-pages-rate > threshold_size (with the default qemu expected downtime of 300ms).

So, the fix is to increase qemu's expected downtime.

This is a tweakable parameter which can be set using "migrate_set_downtime".

With a downtime of 1 second, we get a threshold size of ~350-450 pages,
which will handle the "dirty-pages-rate" of 300+ pages and complete
the migration process, so we bump the default downtime to 1s in case
of SEV live migration being active.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index daea3ecd04..c9bc33fb10 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3568,6 +3568,10 @@ static void migration_update_counters(MigrationState *s,
     transferred = current_bytes - s->iteration_initial_bytes;
     time_spent = current_time - s->iteration_start_time;
     bandwidth = (double)transferred / time_spent;
+    if (memcrypt_enabled() &&
+        s->parameters.downtime_limit < 1000) {
+        s->parameters.downtime_limit = 1000;
+    }
     s->threshold_size = bandwidth * s->parameters.downtime_limit;
 
     s->mbps = (((double) transferred * 8.0) /
-- 
2.17.1

[PATCH v4 14/14] kvm: Add support for userspace MSR filtering and handling of MSR_KVM_MIGRATION_CONTROL.

[Ashish Kalra]

From: Ashish Kalra <ashish.kalra@amd.com>

Add support for userspace MSR filtering using KVM_X86_SET_MSR_FILTER
ioctl and handling of MSRs in userspace. Currently this is only used
for SEV guests which use MSR_KVM_MIGRATION_CONTROL to indicate if the
guest is enabled and ready for migration.

KVM arch code calls into SEV guest specific code to delete the
SEV migrate blocker which has been setup at SEV_LAUNCH_FINISH.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 include/sysemu/sev.h  |  1 +
 target/i386/kvm/kvm.c | 61 +++++++++++++++++++++++++++++++++++++++++++
 target/i386/sev.c     |  6 +++++
 3 files changed, 68 insertions(+)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 023e694ac4..d04890113c 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -38,5 +38,6 @@ int sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
 int sev_save_outgoing_shared_regions_list(QEMUFile *f);
 int sev_load_incoming_shared_regions_list(QEMUFile *f);
 bool sev_is_gfn_in_unshared_region(unsigned long gfn);
+void sev_del_migrate_blocker(void);
 
 #endif
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 303722e06f..785b8fae6b 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2240,6 +2240,19 @@ static void register_smram_listener(Notifier *n, void *unused)
                                  &smram_address_space, 1);
 }
 
+static __u64 bitmap;
+struct kvm_msr_filter msr_filter_allow = {
+    .flags = KVM_MSR_FILTER_DEFAULT_ALLOW,
+    .ranges = {
+        {
+            .flags = KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE,
+            .nmsrs = 1,
+            .base = MSR_KVM_MIGRATION_CONTROL,
+            .bitmap = (uint8_t *)&bitmap,
+        }
+    }
+};
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     uint64_t identity_base = 0xfffbc000;
@@ -2298,6 +2311,21 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    ret = kvm_check_extension(s, KVM_CAP_X86_USER_SPACE_MSR) ?
+          kvm_check_extension(s, KVM_CAP_X86_MSR_FILTER) :
+          -ENOTSUP;
+    if (ret > 0) {
+        ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR,
+                                0, KVM_MSR_EXIT_REASON_FILTER);
+        if (ret == 0) {
+            ret = kvm_vm_ioctl(s, KVM_X86_SET_MSR_FILTER, &msr_filter_allow);
+            if (ret < 0) {
+                error_report("kvm: KVM_X86_SET_MSR_FILTER failed : %s",
+                             strerror(-ret));
+            }
+        }
+    }
+
     ret = kvm_get_supported_msrs(s);
     if (ret < 0) {
         return ret;
@@ -4472,6 +4500,35 @@ static int kvm_handle_exit_hypercall(X86CPU *cpu, struct kvm_run *run)
     return 0;
 }
 
+/*
+ * Currently this exit is only used by SEV guests for
+ * MSR_KVM_MIGRATION_CONTROL to indicate if the guest
+ * is ready for migration.
+ */
+static int kvm_handle_x86_msr(X86CPU *cpu, struct kvm_run *run)
+{
+    static uint64_t msr_kvm_migration_control;
+
+    if (run->msr.index != MSR_KVM_MIGRATION_CONTROL) {
+        run->msr.error = -EINVAL;
+        return -1;
+    }
+
+    switch (run->exit_reason) {
+    case KVM_EXIT_X86_RDMSR:
+        run->msr.error = 0;
+        run->msr.data = msr_kvm_migration_control;
+        break;
+    case KVM_EXIT_X86_WRMSR:
+        msr_kvm_migration_control = run->msr.data;
+        if (run->msr.data == KVM_MIGRATION_READY) {
+            sev_del_migrate_blocker();
+        }
+        run->msr.error = 0;
+    }
+    return 0;
+}
+
 int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
 {
     static const uint8_t int3 = 0xcc;
@@ -4736,6 +4793,10 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
     case KVM_EXIT_HYPERCALL:
         ret = kvm_handle_exit_hypercall(cpu, run);
         break;
+    case KVM_EXIT_X86_RDMSR:
+    case KVM_EXIT_X86_WRMSR:
+        ret = kvm_handle_x86_msr(cpu, run);
+        break;
     default:
         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
         ret = -1;
diff --git a/target/i386/sev.c b/target/i386/sev.c
index d22f2ef6dc..58f74db0e3 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -791,6 +791,12 @@ sev_launch_finish(SevGuestState *sev)
     }
 }
 
+void
+sev_del_migrate_blocker(void)
+{
+    migrate_del_blocker(sev_mig_blocker);
+}
+
 static int
 sev_receive_finish(SevGuestState *s)
 {
-- 
2.17.1

Re: [PATCH v4 02/14] doc: update AMD SEV to include Live migration flow

[Dov Murik]

On 04/08/2021 14:53, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  docs/amd-memory-encryption.txt | 46 +++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index 12ca25180e..0d9184532a 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -126,7 +126,51 @@ TODO
> 
>  Live Migration
>  ----------------
> -TODO
> +AMD SEV encrypts the memory of VMs and because a different key is used
> +in each VM, the hypervisor will be unable to simply copy the
> +ciphertext from one VM to another to migrate the VM. Instead the AMD SEV Key
> +Management API provides sets of function which the hypervisor can use
> +to package a guest page for migration, while maintaining the confidentiality
> +provided by AMD SEV.
> +
> +SEV guest VMs have the concept of private and shared memory. The private
> +memory is encrypted with the guest-specific key, while shared memory may
> +be encrypted with the hypervisor key. The migration APIs provided by the
> +SEV API spec should be used for migrating the private pages.
> +
> +The KVM_HC_MAP_GPA_RANGE hypercall is used by the SEV guest to notify a
> +change in the page encryption status to the hypervisor. The hypercall
> +is invoked when the encryption attribute is changed from encrypted -> decrypted
> +and vice versa. By default all guest pages are considered encrypted.
> +
> +This hypercall exits to qemu via KVM_EXIT_HYPERCALL to manage the guest
> +shared regions and integrate with the qemu's migration code. The shared
> +region list can be used to check if the given guest page is private or shared.
> +
> +Before initiating the migration, we need to know the targets machine's public

s/targets/target/

> +Diffie-Hellman key (PDH) and certificate chain. It can be retrieved
> +with the 'query-sev-capabilities' QMP command or using the sev-tool. The
> +migrate-set-parameter can be used to pass the target machine's PDH and
> +certificate chain.

It's better to clarify that you use query-sev-capabilities QMP command
on the *target* VM (to get its PDF and cert) when it's ready, and then
use migrate-set-parameter QMP command on the *source* so it can prepare
the migration for that specific target.


> +
> +During the migration flow, the SEND_START is called on the source hypervisor
> +to create an outgoing encryption context. The SEV guest policy dictates whether
> +the certificate passed through the migrate-sev-set-info command will be

Here you say migrate-sev-set-info but above you said
migrate-set-parameter.  Which one is it?


-Dov

> +validated. SEND_UPDATE_DATA is called to encrypt the guest private pages.
> +After migration is completed, SEND_FINISH is called to destroy the encryption
> +context and make the VM non-runnable to protect it against cloning.
> +
> +On the target machine, RECEIVE_START is called first to create an
> +incoming encryption context. The RECEIVE_UPDATE_DATA is called to copy
> +the received encrypted page into guest memory. After migration has
> +completed, RECEIVE_FINISH is called to make the VM runnable.
> +
> +For more information about the migration see SEV API Appendix A
> +Usage flow (Live migration section).
> +
> +NOTE:
> +To protect against the memory clone SEV APIs are designed to make the VM
> +unrunnable in case of the migration failure.
> 
>  References
>  -----------------
> 

Re: [PATCH v4 02/14] doc: update AMD SEV to include Live migration flow

[Ashish Kalra]

Hello Dov,

On Thu, Aug 05, 2021 at 09:34:42AM +0300, Dov Murik wrote:
> 
> 
> On 04/08/2021 14:53, Ashish Kalra wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  docs/amd-memory-encryption.txt | 46 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> > index 12ca25180e..0d9184532a 100644
> > --- a/docs/amd-memory-encryption.txt
> > +++ b/docs/amd-memory-encryption.txt
> > @@ -126,7 +126,51 @@ TODO
> > 
> >  Live Migration
> >  ----------------
> > -TODO
> > +AMD SEV encrypts the memory of VMs and because a different key is used
> > +in each VM, the hypervisor will be unable to simply copy the
> > +ciphertext from one VM to another to migrate the VM. Instead the AMD SEV Key
> > +Management API provides sets of function which the hypervisor can use
> > +to package a guest page for migration, while maintaining the confidentiality
> > +provided by AMD SEV.
> > +
> > +SEV guest VMs have the concept of private and shared memory. The private
> > +memory is encrypted with the guest-specific key, while shared memory may
> > +be encrypted with the hypervisor key. The migration APIs provided by the
> > +SEV API spec should be used for migrating the private pages.
> > +
> > +The KVM_HC_MAP_GPA_RANGE hypercall is used by the SEV guest to notify a
> > +change in the page encryption status to the hypervisor. The hypercall
> > +is invoked when the encryption attribute is changed from encrypted -> decrypted
> > +and vice versa. By default all guest pages are considered encrypted.
> > +
> > +This hypercall exits to qemu via KVM_EXIT_HYPERCALL to manage the guest
> > +shared regions and integrate with the qemu's migration code. The shared
> > +region list can be used to check if the given guest page is private or shared.
> > +
> > +Before initiating the migration, we need to know the targets machine's public
> 
> s/targets/target/
> 
> > +Diffie-Hellman key (PDH) and certificate chain. It can be retrieved
> > +with the 'query-sev-capabilities' QMP command or using the sev-tool. The
> > +migrate-set-parameter can be used to pass the target machine's PDH and
> > +certificate chain.
> 
> It's better to clarify that you use query-sev-capabilities QMP command
> on the *target* VM (to get its PDF and cert) when it's ready, and then
> use migrate-set-parameter QMP command on the *source* so it can prepare
> the migration for that specific target.
> 
> 
Ok.

> > +
> > +During the migration flow, the SEND_START is called on the source hypervisor
> > +to create an outgoing encryption context. The SEV guest policy dictates whether
> > +the certificate passed through the migrate-sev-set-info command will be
> 
> Here you say migrate-sev-set-info but above you said
> migrate-set-parameter.  Which one is it?
> 
> 
Actually earlier it used to be migrate-sev-set-info, so this is a typo,
it should be migrate-set-parameter.

Thanks,
Ashish

> 
> > +validated. SEND_UPDATE_DATA is called to encrypt the guest private pages.
> > +After migration is completed, SEND_FINISH is called to destroy the encryption
> > +context and make the VM non-runnable to protect it against cloning.
> > +
> > +On the target machine, RECEIVE_START is called first to create an
> > +incoming encryption context. The RECEIVE_UPDATE_DATA is called to copy
> > +the received encrypted page into guest memory. After migration has
> > +completed, RECEIVE_FINISH is called to make the VM runnable.
> > +
> > +For more information about the migration see SEV API Appendix A
> > +Usage flow (Live migration section).
> > +
> > +NOTE:
> > +To protect against the memory clone SEV APIs are designed to make the VM
> > +unrunnable in case of the migration failure.
> > 
> >  References
> >  -----------------
> > 

Re: [PATCH v4 03/14] migration.json: add AMD SEV specific migration parameters

[Dov Murik]

On 04/08/2021 14:54, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> AMD SEV migration flow requires that target machine's public Diffie-Hellman
> key (PDH) and certificate chain must be passed before initiating the guest
> migration. User can use QMP 'migrate-set-parameters' to pass the certificate
> chain. The certificate chain will be used while creating the outgoing
> encryption context.

Just making sure: The source QEMU must *not* accept the sev_amd_cert
from the target, because that will allow a malicious target to give its
own root cert instead of the official AMD one.  Instead it should use
its own trusted AMD root certificate.


> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  migration/migration.c | 61 +++++++++++++++++++++++++++++++++++++++++++
>  monitor/hmp-cmds.c    | 18 +++++++++++++
>  qapi/migration.json   | 40 +++++++++++++++++++++++++---
>  3 files changed, 116 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 041b8451a6..daea3ecd04 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -907,6 +907,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->announce_rounds = s->parameters.announce_rounds;
>      params->has_announce_step = true;
>      params->announce_step = s->parameters.announce_step;
> +    params->has_sev_pdh = true;
> +    params->sev_pdh = g_strdup(s->parameters.sev_pdh);
> +    params->has_sev_plat_cert = true;
> +    params->sev_plat_cert = g_strdup(s->parameters.sev_plat_cert);
> +    params->has_sev_amd_cert = true;
> +    params->sev_amd_cert = g_strdup(s->parameters.sev_amd_cert);
> 
>      if (s->parameters.has_block_bitmap_mapping) {
>          params->has_block_bitmap_mapping = true;
> @@ -1563,6 +1569,18 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>          dest->has_block_bitmap_mapping = true;
>          dest->block_bitmap_mapping = params->block_bitmap_mapping;
>      }
> +    if (params->has_sev_pdh) {
> +        assert(params->sev_pdh->type == QTYPE_QSTRING);
> +        dest->sev_pdh = g_strdup(params->sev_pdh->u.s);
> +    }
> +    if (params->has_sev_plat_cert) {
> +        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
> +        dest->sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
> +    }
> +    if (params->has_sev_amd_cert) {
> +        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
> +        dest->sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
> +    }
>  }
> 
>  static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1685,6 +1703,21 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>              QAPI_CLONE(BitmapMigrationNodeAliasList,
>                         params->block_bitmap_mapping);
>      }
> +    if (params->has_sev_pdh) {
> +        g_free(s->parameters.sev_pdh);
> +        assert(params->sev_pdh->type == QTYPE_QSTRING);
> +        s->parameters.sev_pdh = g_strdup(params->sev_pdh->u.s);
> +    }
> +    if (params->has_sev_plat_cert) {
> +        g_free(s->parameters.sev_plat_cert);
> +        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
> +        s->parameters.sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
> +    }
> +    if (params->has_sev_amd_cert) {
> +        g_free(s->parameters.sev_amd_cert);
> +        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
> +        s->parameters.sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
> +    }
>  }
> 
>  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> @@ -1705,6 +1738,27 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>          params->tls_hostname->type = QTYPE_QSTRING;
>          params->tls_hostname->u.s = strdup("");
>      }
> +    /* TODO Rewrite "" to null instead */
> +    if (params->has_sev_pdh
> +        && params->sev_pdh->type == QTYPE_QNULL) {
> +        qobject_unref(params->sev_pdh->u.n);
> +        params->sev_pdh->type = QTYPE_QSTRING;
> +        params->sev_pdh->u.s = strdup("");
> +    }
> +    /* TODO Rewrite "" to null instead */
> +    if (params->has_sev_plat_cert
> +        && params->sev_plat_cert->type == QTYPE_QNULL) {
> +        qobject_unref(params->sev_plat_cert->u.n);
> +        params->sev_plat_cert->type = QTYPE_QSTRING;
> +        params->sev_plat_cert->u.s = strdup("");
> +    }
> +    /* TODO Rewrite "" to null instead */
> +    if (params->has_sev_amd_cert
> +        && params->sev_amd_cert->type == QTYPE_QNULL) {
> +        qobject_unref(params->sev_amd_cert->u.n);
> +        params->sev_amd_cert->type = QTYPE_QSTRING;
> +        params->sev_amd_cert->u.s = strdup("");
> +    }
> 
>      migrate_params_test_apply(params, &tmp);
> 
> @@ -4233,6 +4287,9 @@ static void migration_instance_finalize(Object *obj)
>      qemu_mutex_destroy(&ms->qemu_file_lock);
>      g_free(params->tls_hostname);
>      g_free(params->tls_creds);
> +    g_free(params->sev_pdh);
> +    g_free(params->sev_plat_cert);
> +    g_free(params->sev_amd_cert);
>      qemu_sem_destroy(&ms->wait_unplug_sem);
>      qemu_sem_destroy(&ms->rate_limit_sem);
>      qemu_sem_destroy(&ms->pause_sem);
> @@ -4280,6 +4337,10 @@ static void migration_instance_init(Object *obj)
>      params->has_announce_rounds = true;
>      params->has_announce_step = true;
> 
> +    params->sev_pdh = g_strdup("");
> +    params->sev_plat_cert = g_strdup("");
> +    params->sev_amd_cert = g_strdup("");
> +

TODO: init to NULL instead.

>      qemu_sem_init(&ms->postcopy_pause_sem, 0);
>      qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
>      qemu_sem_init(&ms->rp_state.rp_sem, 0);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index e00255f7ee..27ca2024bb 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1399,6 +1399,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          error_setg(&err, "The block-bitmap-mapping parameter can only be set "
>                     "through QMP");
>          break;
> +    case MIGRATION_PARAMETER_SEV_PDH:
> +        p->has_sev_pdh = true;
> +        p->sev_pdh = g_new0(StrOrNull, 1);
> +        p->sev_pdh->type = QTYPE_QSTRING;
> +        visit_type_str(v, param, &p->sev_pdh->u.s, &err);
> +        break;
> +    case MIGRATION_PARAMETER_SEV_PLAT_CERT:
> +        p->has_sev_plat_cert = true;
> +        p->sev_plat_cert = g_new0(StrOrNull, 1);
> +        p->sev_plat_cert->type = QTYPE_QSTRING;
> +        visit_type_str(v, param, &p->sev_plat_cert->u.s, &err);
> +        break;
> +    case MIGRATION_PARAMETER_SEV_AMD_CERT:
> +        p->has_sev_amd_cert = true;
> +        p->sev_amd_cert = g_new0(StrOrNull, 1);
> +        p->sev_amd_cert->type = QTYPE_QSTRING;
> +        visit_type_str(v, param, &p->sev_amd_cert->u.s, &err);
> +        break;
>      default:
>          assert(0);
>      }
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 1124a2dda8..69c615ec4d 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -743,6 +743,15 @@
>  #                        block device name if there is one, and to their node name
>  #                        otherwise. (Since 5.2)
>  #
> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> +#           (Since 4.2)

Since 6.2, I assume. (same for all the changes in this file)


> +#
> +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> +#                 (Since 4.2)
> +#
> +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> +#                base64 (Since 4.2)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -758,7 +767,8 @@
>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>             'max-cpu-throttle', 'multifd-compression',
>             'multifd-zlib-level' ,'multifd-zstd-level',
> -           'block-bitmap-mapping' ] }
> +           'block-bitmap-mapping',
> +           'sev-pdh', 'sev-plat-cert', 'sev-amd-cert' ] }
> 
>  ##
>  # @MigrateSetParameters:
> @@ -903,6 +913,15 @@
>  #                        block device name if there is one, and to their node name
>  #                        otherwise. (Since 5.2)
>  #
> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> +#           (Since 4.2)
> +#
> +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> +#                 (Since 4.2)
> +#
> +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> +#                base64 (Since 4.2)
> +#
>  # Since: 2.4
>  ##
>  # TODO either fuse back into MigrationParameters, or make
> @@ -934,7 +953,10 @@
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*sev-pdh':'StrOrNull',
> +            '*sev-plat-cert': 'StrOrNull',
> +            '*sev-amd-cert' : 'StrOrNull' } }
> 
>  ##
>  # @migrate-set-parameters:
> @@ -1099,6 +1121,15 @@
>  #                        block device name if there is one, and to their node name
>  #                        otherwise. (Since 5.2)
>  #
> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> +#           (Since 4.2)
> +#
> +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> +#                 (Since 4.2)
> +#
> +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> +#                base64 (Since 4.2)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -1128,7 +1159,10 @@
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*sev-pdh':'str',
> +            '*sev-plat-cert': 'str',
> +            '*sev-amd-cert' : 'str'} }
> 
>  ##
>  # @query-migrate-parameters:
> 

Re: [PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs

[Dov Murik]

On 04/08/2021 14:55, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> When memory encryption is enabled in VM, the guest RAM will be encrypted
> with the guest-specific key, to protect the confidentiality of data while
> in transit we need to platform specific hooks to save or migrate the
> guest RAM.
> 
> Introduce the new ConfidentialGuestMemoryEncryptionOps in this patch
> which will be later used by the encrypted guest for migration.

Do we already have SEV / ConfidentialGuest debug operations? (for
reading SEV guest memory from gdb if debug is allowed in policy)

Are they supposed to be in the same Ops struct? Another?


> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  include/exec/confidential-guest-support.h | 27 +++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> index ba2dd4b5df..d8b4bd4c42 100644
> --- a/include/exec/confidential-guest-support.h
> +++ b/include/exec/confidential-guest-support.h
> @@ -20,6 +20,7 @@
> 
>  #ifndef CONFIG_USER_ONLY
> 
> +#include <qapi/qapi-types-migration.h>
>  #include "qom/object.h"
> 
>  #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> @@ -53,8 +54,34 @@ struct ConfidentialGuestSupport {
>      bool ready;
>  };
> 
> +/**
> + * The functions registers with ConfidentialGuestMemoryEncryptionOps will be
> + * used during the encrypted guest migration.
> + */
> +struct ConfidentialGuestMemoryEncryptionOps {

[style] in QEMU you should add a 'typedef' at the beginning and call the
type ConfidentialGuestMemoryEncryptionOps, and then you don't use the
keyword 'struct' when you refer to it.  See for example the definition
of ConfidentialGuestSupportClass below.


> +    /* Initialize the platform specific state before starting the migration */
> +    int (*save_setup)(MigrationParameters *p);
> +
> +    /* Write the encrypted page and metadata associated with it */
> +    int (*save_outgoing_page)(QEMUFile *f, uint8_t *ptr, uint32_t size,
> +                              uint64_t *bytes_sent);
> +
> +    /* Load the incoming encrypted page into guest memory */
> +    int (*load_incoming_page)(QEMUFile *f, uint8_t *ptr);
> +
> +    /* Check if gfn is in shared/unencrypted region */
> +    bool (*is_gfn_in_unshared_region)(unsigned long gfn);

The comment says "shared/unencrypted", but the function name talks about
"unshared".  I prefer:

    /* Check if gfn is in encrypted region */
    bool (*is_gfn_in_encrypted_region)(unsigned long gfn);

(and then maybe the comment is useless?)


> +
> +    /* Write the shared regions list */
> +    int (*save_outgoing_shared_regions_list)(QEMUFile *f);
> +
> +    /* Load the shared regions list */
> +    int (*load_incoming_shared_regions_list)(QEMUFile *f);
> +};
> +
>  typedef struct ConfidentialGuestSupportClass {
>      ObjectClass parent;
> +    struct ConfidentialGuestMemoryEncryptionOps *memory_encryption_ops;

per above, remove 'struct'.


>  } ConfidentialGuestSupportClass;
> 
>  #endif /* !CONFIG_USER_ONLY */
> 

Re: [PATCH v4 05/14] target/i386: sev: provide callback to setup outgoing context

[Dov Murik]

On 04/08/2021 14:56, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> The user provides the target machine's Platform Diffie-Hellman key (PDH)
> and certificate chain before starting the SEV guest migration. Cache the
> certificate chain as we need them while creating the outgoing context.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  include/sysemu/sev.h |  2 ++
>  target/i386/sev.c    | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index 94d821d737..64fc88d3c5 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -14,11 +14,13 @@
>  #ifndef QEMU_SEV_H
>  #define QEMU_SEV_H
> 
> +#include <qapi/qapi-types-migration.h>
>  #include "sysemu/kvm.h"
> 
>  bool sev_enabled(void);
>  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>  int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
> +int sev_save_setup(MigrationParameters *p);
>  int sev_inject_launch_secret(const char *hdr, const char *secret,
>                               uint64_t gpa, Error **errp);
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 83df8c09f6..5e7c87764c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -24,6 +24,7 @@
>  #include "qemu/module.h"
>  #include "qemu/uuid.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/sev.h"
>  #include "sev_i386.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/runstate.h"
> @@ -68,6 +69,12 @@ struct SevGuestState {
>      int sev_fd;
>      SevState state;
>      gchar *measurement;
> +    guchar *remote_pdh;
> +    size_t remote_pdh_len;
> +    guchar *remote_plat_cert;
> +    size_t remote_plat_cert_len;
> +    guchar *amd_cert;
> +    size_t amd_cert_len;
> 
>      uint32_t reset_cs;
>      uint32_t reset_ip;
> @@ -116,6 +123,12 @@ static const char *const sev_fw_errlist[] = {
> 
>  #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
> 
> +#define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB */
> +
> +static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
> +    .save_setup = sev_save_setup,
> +};
> +
>  static int
>  sev_ioctl(int fd, int cmd, void *data, int *error)
>  {
> @@ -772,6 +785,50 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>      }
>  }
> 
> +static inline bool check_blob_length(size_t value)
> +{
> +    if (value > SEV_FW_BLOB_MAX_SIZE) {
> +        error_report("invalid length max=%d got=%ld",
> +                     SEV_FW_BLOB_MAX_SIZE, value);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +int sev_save_setup(MigrationParameters *p)
> +{
> +    SevGuestState *s = sev_guest;
> +    const char *pdh = p->sev_pdh;
> +    const char *plat_cert = p->sev_plat_cert;
> +    const char *amd_cert = p->sev_amd_cert;
> +
> +    s->remote_pdh = g_base64_decode(pdh, &s->remote_pdh_len);

You should check    if (!s->remote_pdh)   to detect decoding failure
(for all g_base64_decode calls here).

Though I must say, it would be better to check validity of the
user-supplied base64 earlier (when migrate-set-parameters QMP call
occurs), and not later when migration starts.


> +    if (!check_blob_length(s->remote_pdh_len)) {
> +        goto error;
> +    }
> +
> +    s->remote_plat_cert = g_base64_decode(plat_cert,
> +                                          &s->remote_plat_cert_len);
> +    if (!check_blob_length(s->remote_plat_cert_len)) {
> +        goto error;
> +    }
> +
> +    s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
> +    if (!check_blob_length(s->amd_cert_len)) {
> +        goto error;
> +    }
> +
> +    return 0;
> +
> +error:
> +    g_free(s->remote_pdh);
> +    g_free(s->remote_plat_cert);
> +    g_free(s->amd_cert);
> +
> +    return 1;
> +}
> +
>  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>  {
>      SevGuestState *sev
> @@ -781,6 +838,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      uint32_t ebx;
>      uint32_t host_cbitpos;
>      struct sev_user_data_status status = {};
> +    ConfidentialGuestSupportClass *cgs_class =
> +        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(cgs));
> 
>      if (!sev) {
>          return 0;
> @@ -870,6 +929,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
>      qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
> 
> +    cgs_class->memory_encryption_ops = &sev_memory_encryption_ops;
> +
>      cgs->ready = true;
> 
>      return 0;
> 

Re: [PATCH v4 07/14] target/i386: sev: add support to encrypt the outgoing page

[Dov Murik]

General comment: In this series you add a lot of migration-related
functionality to sev.c.   Consider moving these functions to
sev-migration.c . (cc: Phil, who attempted some reorg around SEV code.)



On 04/08/2021 14:56, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> The sev_save_outgoing_page() provide the implementation to encrypt the
> guest private pages during the transit. The routines uses the SEND_START
> command to create the outgoing encryption context on the first call then
> uses the SEND_UPDATE_DATA command to encrypt the data before writing it
> to the socket. While encrypting the data SEND_UPDATE_DATA produces some
> metadata (e.g MAC, IV). The metadata is also sent to the target machine.
> After migration is completed, we issue the SEND_FINISH command to transition
> the SEV guest state from sending to unrunnable state.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  include/sysemu/sev.h     |   2 +
>  target/i386/sev.c        | 221 +++++++++++++++++++++++++++++++++++++++
>  target/i386/trace-events |   3 +
>  3 files changed, 226 insertions(+)
> 
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index 64fc88d3c5..aa6b91a53e 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -21,6 +21,8 @@ bool sev_enabled(void);
>  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>  int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
>  int sev_save_setup(MigrationParameters *p);
> +int sev_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
> +                           uint32_t size, uint64_t *bytes_sent);

This function is added to the sev_memory_encryption_ops struct; why do
you need to export it here?


>  int sev_inject_launch_secret(const char *hdr, const char *secret,
>                               uint64_t gpa, Error **errp);
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 10038d3880..411bd657e8 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -30,6 +30,8 @@
>  #include "sysemu/runstate.h"
>  #include "trace.h"
>  #include "migration/blocker.h"
> +#include "migration/qemu-file.h"
> +#include "migration/misc.h"
>  #include "qom/object.h"
>  #include "monitor/monitor.h"
>  #include "exec/confidential-guest-support.h"
> @@ -75,6 +77,8 @@ struct SevGuestState {
>      size_t remote_plat_cert_len;
>      guchar *amd_cert;
>      size_t amd_cert_len;
> +    gchar *send_packet_hdr;
> +    size_t send_packet_hdr_len;
> 
>      uint32_t reset_cs;
>      uint32_t reset_ip;
> @@ -127,6 +131,7 @@ static const char *const sev_fw_errlist[] = {
> 
>  static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
>      .save_setup = sev_save_setup,
> +    .save_outgoing_page = sev_save_outgoing_page,
>  };
> 
>  static int
> @@ -829,6 +834,40 @@ error:
>      return 1;
>  }
> 
> +static void
> +sev_send_finish(void)
> +{
> +    int ret, error;
> +
> +    trace_kvm_sev_send_finish();
> +    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
> +    if (ret) {
> +        error_report("%s: SEND_FINISH ret=%d fw_error=%d '%s'",
> +                     __func__, ret, error, fw_error_to_str(error));
> +    }
> +
> +    g_free(sev_guest->send_packet_hdr);
> +    sev_set_guest_state(sev_guest, SEV_STATE_RUNNING);
> +}
> +
> +static void
> +sev_migration_state_notifier(Notifier *notifier, void *data)
> +{
> +    MigrationState *s = data;
> +
> +    if (migration_has_finished(s) ||
> +        migration_in_postcopy_after_devices(s) ||
> +        migration_has_failed(s)) {
> +        if (sev_check_state(sev_guest, SEV_STATE_SEND_UPDATE)) {
> +            sev_send_finish();
> +        }
> +    }
> +}
> +
> +static Notifier sev_migration_state_notify = {
> +    .notify = sev_migration_state_notifier,
> +};
> +
>  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>  {
>      SevGuestState *sev
> @@ -933,6 +972,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      ram_block_notifier_add(&sev_ram_notifier);
>      qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
>      qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
> +    add_migration_state_change_notifier(&sev_migration_state_notify);
> 
>      cgs_class->memory_encryption_ops = &sev_memory_encryption_ops;
> 
> @@ -1143,6 +1183,187 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
>      return 0;
>  }
> 
> +static int
> +sev_get_send_session_length(void)
> +{
> +    int ret, fw_err = 0;
> +    struct kvm_sev_send_start start = {};
> +
> +    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_SEND_START, &start, &fw_err);
> +    if (fw_err != SEV_RET_INVALID_LEN) {
> +        ret = -1;
> +        error_report("%s: failed to get session length ret=%d fw_error=%d '%s'",
> +                     __func__, ret, fw_err, fw_error_to_str(fw_err));
> +        goto err;

Simply return -1 and eliminate the ret variable?

> +    }
> +
> +    ret = start.session_len;
> +err:
> +    return ret;
> +}
> +
> +static int
> +sev_send_start(SevGuestState *s, QEMUFile *f, uint64_t *bytes_sent)
> +{
> +    gsize pdh_len = 0, plat_cert_len;
> +    int session_len, ret, fw_error;
> +    struct kvm_sev_send_start start = { };
> +    guchar *pdh = NULL, *plat_cert = NULL, *session = NULL;
> +    Error *local_err = NULL;
> +
> +    if (!s->remote_pdh || !s->remote_plat_cert || !s->amd_cert_len) {
> +        error_report("%s: missing remote PDH or PLAT_CERT", __func__);
> +        return 1;
> +    }
> +
> +   start.pdh_cert_uaddr = (uintptr_t) s->remote_pdh;
> +   start.pdh_cert_len = s->remote_pdh_len;
> +
> +   start.plat_certs_uaddr = (uintptr_t)s->remote_plat_cert;
> +   start.plat_certs_len = s->remote_plat_cert_len;
> +
> +   start.amd_certs_uaddr = (uintptr_t)s->amd_cert;
> +   start.amd_certs_len = s->amd_cert_len;
> +
> +    /* get the session length */
> +   session_len = sev_get_send_session_length();
> +   if (session_len < 0) {
> +       ret = 1;
> +       goto err;
> +   }
> +
> +   session = g_new0(guchar, session_len);
> +   start.session_uaddr = (unsigned long)session;
> +   start.session_len = session_len;
> +
> +   /* Get our PDH certificate */
> +   ret = sev_get_pdh_info(s->sev_fd, &pdh, &pdh_len,
> +                          &plat_cert, &plat_cert_len, &local_err);
> +   if (ret) {
> +       error_report("Failed to get our PDH cert");

Add __func__ like other error_report calls here.

> +       goto err;
> +   }
> +
> +   trace_kvm_sev_send_start(start.pdh_cert_uaddr, start.pdh_cert_len,
> +                            start.plat_certs_uaddr, start.plat_certs_len,
> +                            start.amd_certs_uaddr, start.amd_certs_len);
> +
> +   ret = sev_ioctl(s->sev_fd, KVM_SEV_SEND_START, &start, &fw_error);
> +   if (ret < 0) {
> +       error_report("%s: SEND_START ret=%d fw_error=%d '%s'",
> +               __func__, ret, fw_error, fw_error_to_str(fw_error));
> +       goto err;
> +   }
> +
> +   qemu_put_be32(f, start.policy);
> +   qemu_put_be32(f, pdh_len);
> +   qemu_put_buffer(f, (uint8_t *)pdh, pdh_len);
> +   qemu_put_be32(f, start.session_len);
> +   qemu_put_buffer(f, (uint8_t *)start.session_uaddr, start.session_len);
> +   *bytes_sent = 12 + pdh_len + start.session_len;

I prefer updating *bytes_sent in steps, like you do below in
sev_send_update_data.

> +
> +   sev_set_guest_state(s, SEV_STATE_SEND_UPDATE);
> +
> +err:
> +   g_free(pdh);
> +   g_free(plat_cert);
> +   return ret;
> +}
> +
> +static int
> +sev_send_get_packet_len(int *fw_err)
> +{
> +    int ret;
> +    struct kvm_sev_send_update_data update = {};
> +
> +    ret = sev_ioctl(sev_guest->sev_fd, KVM_SEV_SEND_UPDATE_DATA,
> +                    &update, fw_err);
> +    if (*fw_err != SEV_RET_INVALID_LEN) {
> +        ret = -1;
> +        error_report("%s: failed to get session length ret=%d fw_error=%d '%s'",
> +                    __func__, ret, *fw_err, fw_error_to_str(*fw_err));
> +        goto err;

Simply return -1, remove ret variable?

> +    }
> +
> +    ret = update.hdr_len;
> +
> +err:
> +    return ret;
> +}
> +
> +static int
> +sev_send_update_data(SevGuestState *s, QEMUFile *f, uint8_t *ptr, uint32_t size,
> +                     uint64_t *bytes_sent)
> +{
> +    int ret, fw_error;
> +    guchar *trans;
> +    struct kvm_sev_send_update_data update = { };
> +
> +    /*
> +     * If this is first call then query the packet header bytes and allocate
> +     * the packet buffer.
> +     */
> +    if (!s->send_packet_hdr) {
> +        s->send_packet_hdr_len = sev_send_get_packet_len(&fw_error);
> +        if (s->send_packet_hdr_len < 1) {
> +            error_report("%s: SEND_UPDATE fw_error=%d '%s'",
> +                         __func__, fw_error, fw_error_to_str(fw_error));
> +            return 1;
> +        }
> +
> +        s->send_packet_hdr = g_new(gchar, s->send_packet_hdr_len);
> +    }
> +
> +    /* allocate transport buffer */
> +    trans = g_new(guchar, size);

Is it OK to allocate and free this buffer (4 KB, I assume) for every page?

If that's the purpose, use g_autofree, and replace "goto err" with return.


-Dov

> +
> +    update.hdr_uaddr = (uintptr_t)s->send_packet_hdr;
> +    update.hdr_len = s->send_packet_hdr_len;
> +    update.guest_uaddr = (uintptr_t)ptr;
> +    update.guest_len = size;
> +    update.trans_uaddr = (uintptr_t)trans;
> +    update.trans_len = size;
> +
> +    trace_kvm_sev_send_update_data(ptr, trans, size);
> +
> +    ret = sev_ioctl(s->sev_fd, KVM_SEV_SEND_UPDATE_DATA, &update, &fw_error);
> +    if (ret) {
> +        error_report("%s: SEND_UPDATE_DATA ret=%d fw_error=%d '%s'",
> +                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> +        goto err;
> +    }
> +
> +    qemu_put_be32(f, update.hdr_len);
> +    qemu_put_buffer(f, (uint8_t *)update.hdr_uaddr, update.hdr_len);
> +    *bytes_sent = 4 + update.hdr_len;
> +
> +    qemu_put_be32(f, update.trans_len);
> +    qemu_put_buffer(f, (uint8_t *)update.trans_uaddr, update.trans_len);
> +    *bytes_sent += (4 + update.trans_len);
> +
> +err:
> +    g_free(trans);
> +    return ret;
> +}
> +
> +int sev_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
> +                           uint32_t sz, uint64_t *bytes_sent)
> +{
> +    SevGuestState *s = sev_guest;
> +
> +    /*
> +     * If this is a first buffer then create outgoing encryption context
> +     * and write our PDH, policy and session data.
> +     */
> +    if (!sev_check_state(s, SEV_STATE_SEND_UPDATE) &&
> +        sev_send_start(s, f, bytes_sent)) {
> +        error_report("Failed to create outgoing context");
> +        return 1;
> +    }
> +
> +    return sev_send_update_data(s, f, ptr, sz, bytes_sent);
> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 2cd8726eeb..e8d4aec125 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -11,3 +11,6 @@ kvm_sev_launch_measurement(const char *value) "data %s"
>  kvm_sev_launch_finish(void) ""
>  kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s"
> +kvm_sev_send_start(uint64_t pdh, int l1, uint64_t plat, int l2, uint64_t amd, int l3) "pdh 0x%" PRIx64 " len %d plat 0x%" PRIx64 " len %d amd 0x%" PRIx64 " len %d"
> +kvm_sev_send_update_data(void *src, void *dst, int len) "guest %p trans %p len %d"
> +kvm_sev_send_finish(void) ""
> 

Re: [PATCH v4 03/14] migration.json: add AMD SEV specific migration parameters

[Ashish Kalra]

Hello Dov,

On Thu, Aug 05, 2021 at 12:42:50PM +0300, Dov Murik wrote:
> 
> 
> On 04/08/2021 14:54, Ashish Kalra wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > AMD SEV migration flow requires that target machine's public Diffie-Hellman
> > key (PDH) and certificate chain must be passed before initiating the guest
> > migration. User can use QMP 'migrate-set-parameters' to pass the certificate
> > chain. The certificate chain will be used while creating the outgoing
> > encryption context.
> 
> Just making sure: The source QEMU must *not* accept the sev_amd_cert
> from the target, because that will allow a malicious target to give its
> own root cert instead of the official AMD one.  Instead it should use
> its own trusted AMD root certificate.
> 

Actually, it is the responsibility of the VMM management stack to manage
all the certificates, before starting migration the VMM management stack
will retrieve the certs, from either the target or other ways.

KVM/qemu are just facilitating in providing these certs to the PSP via
the SEND_START command, it is the responsibility of the PSP to
validate/authenticate these certs and establish an encryption context
with the target.

Thanks,
Ashish

> 
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  migration/migration.c | 61 +++++++++++++++++++++++++++++++++++++++++++
> >  monitor/hmp-cmds.c    | 18 +++++++++++++
> >  qapi/migration.json   | 40 +++++++++++++++++++++++++---
> >  3 files changed, 116 insertions(+), 3 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 041b8451a6..daea3ecd04 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -907,6 +907,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> >      params->announce_rounds = s->parameters.announce_rounds;
> >      params->has_announce_step = true;
> >      params->announce_step = s->parameters.announce_step;
> > +    params->has_sev_pdh = true;
> > +    params->sev_pdh = g_strdup(s->parameters.sev_pdh);
> > +    params->has_sev_plat_cert = true;
> > +    params->sev_plat_cert = g_strdup(s->parameters.sev_plat_cert);
> > +    params->has_sev_amd_cert = true;
> > +    params->sev_amd_cert = g_strdup(s->parameters.sev_amd_cert);
> > 
> >      if (s->parameters.has_block_bitmap_mapping) {
> >          params->has_block_bitmap_mapping = true;
> > @@ -1563,6 +1569,18 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
> >          dest->has_block_bitmap_mapping = true;
> >          dest->block_bitmap_mapping = params->block_bitmap_mapping;
> >      }
> > +    if (params->has_sev_pdh) {
> > +        assert(params->sev_pdh->type == QTYPE_QSTRING);
> > +        dest->sev_pdh = g_strdup(params->sev_pdh->u.s);
> > +    }
> > +    if (params->has_sev_plat_cert) {
> > +        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
> > +        dest->sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
> > +    }
> > +    if (params->has_sev_amd_cert) {
> > +        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
> > +        dest->sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
> > +    }
> >  }
> > 
> >  static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> > @@ -1685,6 +1703,21 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> >              QAPI_CLONE(BitmapMigrationNodeAliasList,
> >                         params->block_bitmap_mapping);
> >      }
> > +    if (params->has_sev_pdh) {
> > +        g_free(s->parameters.sev_pdh);
> > +        assert(params->sev_pdh->type == QTYPE_QSTRING);
> > +        s->parameters.sev_pdh = g_strdup(params->sev_pdh->u.s);
> > +    }
> > +    if (params->has_sev_plat_cert) {
> > +        g_free(s->parameters.sev_plat_cert);
> > +        assert(params->sev_plat_cert->type == QTYPE_QSTRING);
> > +        s->parameters.sev_plat_cert = g_strdup(params->sev_plat_cert->u.s);
> > +    }
> > +    if (params->has_sev_amd_cert) {
> > +        g_free(s->parameters.sev_amd_cert);
> > +        assert(params->sev_amd_cert->type == QTYPE_QSTRING);
> > +        s->parameters.sev_amd_cert = g_strdup(params->sev_amd_cert->u.s);
> > +    }
> >  }
> > 
> >  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> > @@ -1705,6 +1738,27 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> >          params->tls_hostname->type = QTYPE_QSTRING;
> >          params->tls_hostname->u.s = strdup("");
> >      }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_sev_pdh
> > +        && params->sev_pdh->type == QTYPE_QNULL) {
> > +        qobject_unref(params->sev_pdh->u.n);
> > +        params->sev_pdh->type = QTYPE_QSTRING;
> > +        params->sev_pdh->u.s = strdup("");
> > +    }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_sev_plat_cert
> > +        && params->sev_plat_cert->type == QTYPE_QNULL) {
> > +        qobject_unref(params->sev_plat_cert->u.n);
> > +        params->sev_plat_cert->type = QTYPE_QSTRING;
> > +        params->sev_plat_cert->u.s = strdup("");
> > +    }
> > +    /* TODO Rewrite "" to null instead */
> > +    if (params->has_sev_amd_cert
> > +        && params->sev_amd_cert->type == QTYPE_QNULL) {
> > +        qobject_unref(params->sev_amd_cert->u.n);
> > +        params->sev_amd_cert->type = QTYPE_QSTRING;
> > +        params->sev_amd_cert->u.s = strdup("");
> > +    }
> > 
> >      migrate_params_test_apply(params, &tmp);
> > 
> > @@ -4233,6 +4287,9 @@ static void migration_instance_finalize(Object *obj)
> >      qemu_mutex_destroy(&ms->qemu_file_lock);
> >      g_free(params->tls_hostname);
> >      g_free(params->tls_creds);
> > +    g_free(params->sev_pdh);
> > +    g_free(params->sev_plat_cert);
> > +    g_free(params->sev_amd_cert);
> >      qemu_sem_destroy(&ms->wait_unplug_sem);
> >      qemu_sem_destroy(&ms->rate_limit_sem);
> >      qemu_sem_destroy(&ms->pause_sem);
> > @@ -4280,6 +4337,10 @@ static void migration_instance_init(Object *obj)
> >      params->has_announce_rounds = true;
> >      params->has_announce_step = true;
> > 
> > +    params->sev_pdh = g_strdup("");
> > +    params->sev_plat_cert = g_strdup("");
> > +    params->sev_amd_cert = g_strdup("");
> > +
> 
> TODO: init to NULL instead.
> 
> >      qemu_sem_init(&ms->postcopy_pause_sem, 0);
> >      qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
> >      qemu_sem_init(&ms->rp_state.rp_sem, 0);
> > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > index e00255f7ee..27ca2024bb 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -1399,6 +1399,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> >          error_setg(&err, "The block-bitmap-mapping parameter can only be set "
> >                     "through QMP");
> >          break;
> > +    case MIGRATION_PARAMETER_SEV_PDH:
> > +        p->has_sev_pdh = true;
> > +        p->sev_pdh = g_new0(StrOrNull, 1);
> > +        p->sev_pdh->type = QTYPE_QSTRING;
> > +        visit_type_str(v, param, &p->sev_pdh->u.s, &err);
> > +        break;
> > +    case MIGRATION_PARAMETER_SEV_PLAT_CERT:
> > +        p->has_sev_plat_cert = true;
> > +        p->sev_plat_cert = g_new0(StrOrNull, 1);
> > +        p->sev_plat_cert->type = QTYPE_QSTRING;
> > +        visit_type_str(v, param, &p->sev_plat_cert->u.s, &err);
> > +        break;
> > +    case MIGRATION_PARAMETER_SEV_AMD_CERT:
> > +        p->has_sev_amd_cert = true;
> > +        p->sev_amd_cert = g_new0(StrOrNull, 1);
> > +        p->sev_amd_cert->type = QTYPE_QSTRING;
> > +        visit_type_str(v, param, &p->sev_amd_cert->u.s, &err);
> > +        break;
> >      default:
> >          assert(0);
> >      }
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 1124a2dda8..69c615ec4d 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -743,6 +743,15 @@
> >  #                        block device name if there is one, and to their node name
> >  #                        otherwise. (Since 5.2)
> >  #
> > +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> > +#           (Since 4.2)
> 
> Since 6.2, I assume. (same for all the changes in this file)
> 
> 
> > +#
> > +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> > +#                 (Since 4.2)
> > +#
> > +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> > +#                base64 (Since 4.2)
> > +#
> >  # Since: 2.4
> >  ##
> >  { 'enum': 'MigrationParameter',
> > @@ -758,7 +767,8 @@
> >             'xbzrle-cache-size', 'max-postcopy-bandwidth',
> >             'max-cpu-throttle', 'multifd-compression',
> >             'multifd-zlib-level' ,'multifd-zstd-level',
> > -           'block-bitmap-mapping' ] }
> > +           'block-bitmap-mapping',
> > +           'sev-pdh', 'sev-plat-cert', 'sev-amd-cert' ] }
> > 
> >  ##
> >  # @MigrateSetParameters:
> > @@ -903,6 +913,15 @@
> >  #                        block device name if there is one, and to their node name
> >  #                        otherwise. (Since 5.2)
> >  #
> > +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> > +#           (Since 4.2)
> > +#
> > +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> > +#                 (Since 4.2)
> > +#
> > +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> > +#                base64 (Since 4.2)
> > +#
> >  # Since: 2.4
> >  ##
> >  # TODO either fuse back into MigrationParameters, or make
> > @@ -934,7 +953,10 @@
> >              '*multifd-compression': 'MultiFDCompression',
> >              '*multifd-zlib-level': 'uint8',
> >              '*multifd-zstd-level': 'uint8',
> > -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> > +            '*sev-pdh':'StrOrNull',
> > +            '*sev-plat-cert': 'StrOrNull',
> > +            '*sev-amd-cert' : 'StrOrNull' } }
> > 
> >  ##
> >  # @migrate-set-parameters:
> > @@ -1099,6 +1121,15 @@
> >  #                        block device name if there is one, and to their node name
> >  #                        otherwise. (Since 5.2)
> >  #
> > +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> > +#           (Since 4.2)
> > +#
> > +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> > +#                 (Since 4.2)
> > +#
> > +# @sev-amd-cert: AMD certificate chain which include ASK and OCA encoded in
> > +#                base64 (Since 4.2)
> > +#
> >  # Since: 2.4
> >  ##
> >  { 'struct': 'MigrationParameters',
> > @@ -1128,7 +1159,10 @@
> >              '*multifd-compression': 'MultiFDCompression',
> >              '*multifd-zlib-level': 'uint8',
> >              '*multifd-zstd-level': 'uint8',
> > -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> > +            '*sev-pdh':'str',
> > +            '*sev-plat-cert': 'str',
> > +            '*sev-amd-cert' : 'str'} }
> > 
> >  ##
> >  # @query-migrate-parameters:
> > 

Re: [PATCH v4 04/14] confidential guest support: introduce ConfidentialGuestMemoryEncryptionOps for encrypted VMs

[Ashish Kalra]

Hello Dov,

On Thu, Aug 05, 2021 at 03:20:50PM +0300, Dov Murik wrote:
> 
> 
> On 04/08/2021 14:55, Ashish Kalra wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > When memory encryption is enabled in VM, the guest RAM will be encrypted
> > with the guest-specific key, to protect the confidentiality of data while
> > in transit we need to platform specific hooks to save or migrate the
> > guest RAM.
> > 
> > Introduce the new ConfidentialGuestMemoryEncryptionOps in this patch
> > which will be later used by the encrypted guest for migration.
> 
> Do we already have SEV / ConfidentialGuest debug operations? (for
> reading SEV guest memory from gdb if debug is allowed in policy)
> 
> Are they supposed to be in the same Ops struct? Another?
> 

Not currently, the SEV debug patches were submitted as an independent
patch-set way before the ConfidentialGuestSupport was introduced, in the
future they may need to be rebased on this support.

Thanks,
Ashish

> 
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  include/exec/confidential-guest-support.h | 27 +++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> > index ba2dd4b5df..d8b4bd4c42 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -20,6 +20,7 @@
> > 
> >  #ifndef CONFIG_USER_ONLY
> > 
> > +#include <qapi/qapi-types-migration.h>
> >  #include "qom/object.h"
> > 
> >  #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > @@ -53,8 +54,34 @@ struct ConfidentialGuestSupport {
> >      bool ready;
> >  };
> > 
> > +/**
> > + * The functions registers with ConfidentialGuestMemoryEncryptionOps will be
> > + * used during the encrypted guest migration.
> > + */
> > +struct ConfidentialGuestMemoryEncryptionOps {
> 
> [style] in QEMU you should add a 'typedef' at the beginning and call the
> type ConfidentialGuestMemoryEncryptionOps, and then you don't use the
> keyword 'struct' when you refer to it.  See for example the definition
> of ConfidentialGuestSupportClass below.
> 
> 
> > +    /* Initialize the platform specific state before starting the migration */
> > +    int (*save_setup)(MigrationParameters *p);
> > +
> > +    /* Write the encrypted page and metadata associated with it */
> > +    int (*save_outgoing_page)(QEMUFile *f, uint8_t *ptr, uint32_t size,
> > +                              uint64_t *bytes_sent);
> > +
> > +    /* Load the incoming encrypted page into guest memory */
> > +    int (*load_incoming_page)(QEMUFile *f, uint8_t *ptr);
> > +
> > +    /* Check if gfn is in shared/unencrypted region */
> > +    bool (*is_gfn_in_unshared_region)(unsigned long gfn);
> 
> The comment says "shared/unencrypted", but the function name talks about
> "unshared".  I prefer:
> 
>     /* Check if gfn is in encrypted region */
>     bool (*is_gfn_in_encrypted_region)(unsigned long gfn);
> 
> (and then maybe the comment is useless?)
> 
> 
> > +
> > +    /* Write the shared regions list */
> > +    int (*save_outgoing_shared_regions_list)(QEMUFile *f);
> > +
> > +    /* Load the shared regions list */
> > +    int (*load_incoming_shared_regions_list)(QEMUFile *f);
> > +};
> > +
> >  typedef struct ConfidentialGuestSupportClass {
> >      ObjectClass parent;
> > +    struct ConfidentialGuestMemoryEncryptionOps *memory_encryption_ops;
> 
> per above, remove 'struct'.
> 
> 
> >  } ConfidentialGuestSupportClass;
> > 
> >  #endif /* !CONFIG_USER_ONLY */
> > 

Re: [PATCH v4 05/14] target/i386: sev: provide callback to setup outgoing context

[Ashish Kalra]

On Thu, Aug 05, 2021 at 04:06:27PM +0300, Dov Murik wrote:
> 
> 
> On 04/08/2021 14:56, Ashish Kalra wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > The user provides the target machine's Platform Diffie-Hellman key (PDH)
> > and certificate chain before starting the SEV guest migration. Cache the
> > certificate chain as we need them while creating the outgoing context.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  include/sysemu/sev.h |  2 ++
> >  target/i386/sev.c    | 61 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> > index 94d821d737..64fc88d3c5 100644
> > --- a/include/sysemu/sev.h
> > +++ b/include/sysemu/sev.h
> > @@ -14,11 +14,13 @@
> >  #ifndef QEMU_SEV_H
> >  #define QEMU_SEV_H
> > 
> > +#include <qapi/qapi-types-migration.h>
> >  #include "sysemu/kvm.h"
> > 
> >  bool sev_enabled(void);
> >  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
> >  int sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp);
> > +int sev_save_setup(MigrationParameters *p);
> >  int sev_inject_launch_secret(const char *hdr, const char *secret,
> >                               uint64_t gpa, Error **errp);
> > 
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 83df8c09f6..5e7c87764c 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -24,6 +24,7 @@
> >  #include "qemu/module.h"
> >  #include "qemu/uuid.h"
> >  #include "sysemu/kvm.h"
> > +#include "sysemu/sev.h"
> >  #include "sev_i386.h"
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/runstate.h"
> > @@ -68,6 +69,12 @@ struct SevGuestState {
> >      int sev_fd;
> >      SevState state;
> >      gchar *measurement;
> > +    guchar *remote_pdh;
> > +    size_t remote_pdh_len;
> > +    guchar *remote_plat_cert;
> > +    size_t remote_plat_cert_len;
> > +    guchar *amd_cert;
> > +    size_t amd_cert_len;
> > 
> >      uint32_t reset_cs;
> >      uint32_t reset_ip;
> > @@ -116,6 +123,12 @@ static const char *const sev_fw_errlist[] = {
> > 
> >  #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
> > 
> > +#define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB */
> > +
> > +static struct ConfidentialGuestMemoryEncryptionOps sev_memory_encryption_ops = {
> > +    .save_setup = sev_save_setup,
> > +};
> > +
> >  static int
> >  sev_ioctl(int fd, int cmd, void *data, int *error)
> >  {
> > @@ -772,6 +785,50 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
> >      }
> >  }
> > 
> > +static inline bool check_blob_length(size_t value)
> > +{
> > +    if (value > SEV_FW_BLOB_MAX_SIZE) {
> > +        error_report("invalid length max=%d got=%ld",
> > +                     SEV_FW_BLOB_MAX_SIZE, value);
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +int sev_save_setup(MigrationParameters *p)
> > +{
> > +    SevGuestState *s = sev_guest;
> > +    const char *pdh = p->sev_pdh;
> > +    const char *plat_cert = p->sev_plat_cert;
> > +    const char *amd_cert = p->sev_amd_cert;
> > +
> > +    s->remote_pdh = g_base64_decode(pdh, &s->remote_pdh_len);
> 
> You should check    if (!s->remote_pdh)   to detect decoding failure
> (for all g_base64_decode calls here).
> 
Ok.

Thanks,
Ashish

> Though I must say, it would be better to check validity of the
> user-supplied base64 earlier (when migrate-set-parameters QMP call
> occurs), and not later when migration starts.
> 
> 
> > +    if (!check_blob_length(s->remote_pdh_len)) {
> > +        goto error;
> > +    }
> > +
> > +    s->remote_plat_cert = g_base64_decode(plat_cert,
> > +                                          &s->remote_plat_cert_len);
> > +    if (!check_blob_length(s->remote_plat_cert_len)) {
> > +        goto error;
> > +    }
> > +
> > +    s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
> > +    if (!check_blob_length(s->amd_cert_len)) {
> > +        goto error;
> > +    }
> > +
> > +    return 0;
> > +
> > +error:
> > +    g_free(s->remote_pdh);
> > +    g_free(s->remote_plat_cert);
> > +    g_free(s->amd_cert);
> > +
> > +    return 1;
> > +}
> > +
> >  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >  {
> >      SevGuestState *sev
> > @@ -781,6 +838,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >      uint32_t ebx;
> >      uint32_t host_cbitpos;
> >      struct sev_user_data_status status = {};
> > +    ConfidentialGuestSupportClass *cgs_class =
> > +        (ConfidentialGuestSupportClass *) object_get_class(OBJECT(cgs));
> > 
> >      if (!sev) {
> >          return 0;
> > @@ -870,6 +929,8 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >      qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> >      qemu_add_vm_change_state_handler(sev_vm_state_change, sev);
> > 
> > +    cgs_class->memory_encryption_ops = &sev_memory_encryption_ops;
> > +
> >      cgs->ready = true;
> > 
> >      return 0;
> > 

Re: [PATCH v4 03/14] migration.json: add AMD SEV specific migration parameters

[Eric Blake]

On Wed, Aug 04, 2021 at 11:54:43AM +0000, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> AMD SEV migration flow requires that target machine's public Diffie-Hellman
> key (PDH) and certificate chain must be passed before initiating the guest
> migration. User can use QMP 'migrate-set-parameters' to pass the certificate
> chain. The certificate chain will be used while creating the outgoing
> encryption context.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> +++ b/qapi/migration.json
> @@ -743,6 +743,15 @@
>  #                        block device name if there is one, and to their node name
>  #                        otherwise. (Since 5.2)
>  #
> +# @sev-pdh: The target host platform diffie-hellman key encoded in base64
> +#           (Since 4.2)

6.2, not 4.2.

> +#
> +# @sev-plat-cert: The target host platform certificate chain encoded in base64
> +#                 (Since 4.2)

And again; I'll quit pointing it out.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 01/14] doc: update AMD SEV API spec web link

[Dr. David Alan Gilbert]

* Ashish Kalra (Ashish.Kalra@amd.com) wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>

Lets get this little change out the way; please resend just this patch to
qemu-trivial@nongnu.org  and we can have one less patch on this series.

Dave

> ---
>  docs/amd-memory-encryption.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index ffca382b5f..12ca25180e 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -88,8 +88,8 @@ expects.
>  LAUNCH_FINISH finalizes the guest launch and destroys the cryptographic
>  context.
>  
> -See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> -complete flow chart.
> +See Secure Encrypted Virtualization Key Management API spec section
> +'Launching a guest' usage flow  (Appendix A) for the complete flow chart.
>  
>  To launch a SEV guest
>  
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH v4 10/14] migration: add support to migrate shared regions list

[Wang, Wei W]

> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> When memory encryption is enabled, the hypervisor maintains a shared
> regions list which is referred by hypervisor during migration to check if page is
> private or shared. This list is built during the VM bootup and must be migrated
> to the target host so that hypervisor on target host can use it for future
> migration.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  include/sysemu/sev.h |  2 ++
>  target/i386/sev.c    | 43
> +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h index
> 3b913518c0..118ee66406 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -32,5 +32,7 @@ void sev_es_set_reset_vector(CPUState *cpu);  int
> sev_remove_shared_regions_list(unsigned long gfn_start,
>                                     unsigned long gfn_end);  int
> sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
> +int sev_save_outgoing_shared_regions_list(QEMUFile *f); int
> +sev_load_incoming_shared_regions_list(QEMUFile *f);
> 
>  #endif
> diff --git a/target/i386/sev.c b/target/i386/sev.c index
> 6d44b7ad21..789051f7b4 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -135,10 +135,15 @@ static const char *const sev_fw_errlist[] = {
> 
>  #define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB
> */
> 
> +#define SHARED_REGION_LIST_CONT     0x1
> +#define SHARED_REGION_LIST_END      0x2
> +
>  static struct ConfidentialGuestMemoryEncryptionOps
> sev_memory_encryption_ops = {
>      .save_setup = sev_save_setup,
>      .save_outgoing_page = sev_save_outgoing_page,
>      .load_incoming_page = sev_load_incoming_page,
> +    .save_outgoing_shared_regions_list =
> sev_save_outgoing_shared_regions_list,
> +    .load_incoming_shared_regions_list =
> + sev_load_incoming_shared_regions_list,

Hi Ashish,
I have some questions about the callbacks:

1) why using a list of shared regions, instead of bitmaps to record private/shared state?
I saw that the KVM side implementation used bitmaps in the first place and changed to
shared regions since v10, but don't find the reason.

2) why is the save/load of shared region list (or bitmap) made vendor specific?
I think it can be a common interface and data structure, e.g. KVM maintains a per memory slot
bitmap to be obtained by QEMU.

Best,
Wei

RE: [PATCH v4 14/14] kvm: Add support for userspace MSR filtering and handling of MSR_KVM_MIGRATION_CONTROL.

[Wang, Wei W]

On Wednesday, August 4, 2021 8:00 PM, Ashish Kalra wrote:
> +/*
> + * Currently this exit is only used by SEV guests for
> + * MSR_KVM_MIGRATION_CONTROL to indicate if the guest
> + * is ready for migration.
> + */
> +static int kvm_handle_x86_msr(X86CPU *cpu, struct kvm_run *run) {
> +    static uint64_t msr_kvm_migration_control;
> +
> +    if (run->msr.index != MSR_KVM_MIGRATION_CONTROL) {
> +        run->msr.error = -EINVAL;
> +        return -1;
> +    }
> +
> +    switch (run->exit_reason) {
> +    case KVM_EXIT_X86_RDMSR:
> +        run->msr.error = 0;
> +        run->msr.data = msr_kvm_migration_control;
> +        break;
> +    case KVM_EXIT_X86_WRMSR:
> +        msr_kvm_migration_control = run->msr.data;
> +        if (run->msr.data == KVM_MIGRATION_READY) {
> +            sev_del_migrate_blocker();

It seems this is enabled/disabled by the guest, which means that the guest can always refuse to be migrated?

Best,
Wei

Re: [PATCH v4 10/14] migration: add support to migrate shared regions list

[Ashish Kalra]

Hello Wang, 

On Fri, Sep 10, 2021 at 07:54:10AM +0000, Wang, Wei W wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > When memory encryption is enabled, the hypervisor maintains a shared
> > regions list which is referred by hypervisor during migration to check if page is
> > private or shared. This list is built during the VM bootup and must be migrated
> > to the target host so that hypervisor on target host can use it for future
> > migration.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  include/sysemu/sev.h |  2 ++
> >  target/i386/sev.c    | 43
> > +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h index
> > 3b913518c0..118ee66406 100644
> > --- a/include/sysemu/sev.h
> > +++ b/include/sysemu/sev.h
> > @@ -32,5 +32,7 @@ void sev_es_set_reset_vector(CPUState *cpu);  int
> > sev_remove_shared_regions_list(unsigned long gfn_start,
> >                                     unsigned long gfn_end);  int
> > sev_add_shared_regions_list(unsigned long gfn_start, unsigned long gfn_end);
> > +int sev_save_outgoing_shared_regions_list(QEMUFile *f); int
> > +sev_load_incoming_shared_regions_list(QEMUFile *f);
> > 
> >  #endif
> > diff --git a/target/i386/sev.c b/target/i386/sev.c index
> > 6d44b7ad21..789051f7b4 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -135,10 +135,15 @@ static const char *const sev_fw_errlist[] = {
> > 
> >  #define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB
> > */
> > 
> > +#define SHARED_REGION_LIST_CONT     0x1
> > +#define SHARED_REGION_LIST_END      0x2
> > +
> >  static struct ConfidentialGuestMemoryEncryptionOps
> > sev_memory_encryption_ops = {
> >      .save_setup = sev_save_setup,
> >      .save_outgoing_page = sev_save_outgoing_page,
> >      .load_incoming_page = sev_load_incoming_page,
> > +    .save_outgoing_shared_regions_list =
> > sev_save_outgoing_shared_regions_list,
> > +    .load_incoming_shared_regions_list =
> > + sev_load_incoming_shared_regions_list,
> 
> Hi Ashish,
> I have some questions about the callbacks:
> 
> 1) why using a list of shared regions, instead of bitmaps to record private/shared state?
> I saw that the KVM side implementation used bitmaps in the first place and changed to
> shared regions since v10, but don't find the reason.
> 

There has been a long discussion on this implementation on KVM mailing
list. Tracking shared memory via a list of ranges instead of using bitmap
is more optimal. Most of the guest memory will be private and the
unencrypted/shared regions are basically ranges/intervals, so easy to
implement and maintain using lists.

A list will consume much less memory than a bitmap. 

The bitmap will consume more memory as it will need to be sized as per
guest RAM size and will remain sparsely populated due to limited amount
of shared/unencrypted guest memory regions.

> 2) why is the save/load of shared region list (or bitmap) made vendor specific?
> I think it can be a common interface and data structure, e.g. KVM maintains a per memory slot
> bitmap to be obtained by QEMU.
> 

As the shared regions list current implementation is vendor specific,
it's migration is also vendor specific.

But you are right, this can be implemented as a common interface and
data stucture in qemu, but it is a separate data structure and not
maintained as the per memory slot bitmap in KVM and obtained as such by
qemu.

Thanks,
Ashish

RE: [PATCH v4 10/14] migration: add support to migrate shared regions list

[Wang, Wei W]

On Friday, September 10, 2021 4:48 PM, Ashish Kalra wrote:
> On Fri, Sep 10, 2021 at 07:54:10AM +0000, Wang, Wei W wrote:
> There has been a long discussion on this implementation on KVM mailing list.
> Tracking shared memory via a list of ranges instead of using bitmap is more
> optimal. Most of the guest memory will be private and the unencrypted/shared
> regions are basically ranges/intervals, so easy to implement and maintain using
> lists.

OK. At which version did you discuss this or do you have a link? (I didn't find it in v9 KVM patches)

> A list will consume much less memory than a bitmap.
> 
> The bitmap will consume more memory as it will need to be sized as per guest
> RAM size and will remain sparsely populated due to limited amount of
> shared/unencrypted guest memory regions.

I also thought about this. It depends on the guest.
I think "A list will consume much less memory" is true when we assume most of guest pages are private pages.
From design perspective, what if guest chooses to have most of its pages being shared?
Lists might consume much more memory than bitmaps in some cases, I think.
(Probably I can check your previous discussions first)

Best,
Wei

Re: [PATCH v4 14/14] kvm: Add support for userspace MSR filtering and handling of MSR_KVM_MIGRATION_CONTROL.

[Ashish Kalra]

On Fri, Sep 10, 2021 at 07:56:36AM +0000, Wang, Wei W wrote:
> On Wednesday, August 4, 2021 8:00 PM, Ashish Kalra wrote:
> > +/*
> > + * Currently this exit is only used by SEV guests for
> > + * MSR_KVM_MIGRATION_CONTROL to indicate if the guest
> > + * is ready for migration.
> > + */
> > +static int kvm_handle_x86_msr(X86CPU *cpu, struct kvm_run *run) {
> > +    static uint64_t msr_kvm_migration_control;
> > +
> > +    if (run->msr.index != MSR_KVM_MIGRATION_CONTROL) {
> > +        run->msr.error = -EINVAL;
> > +        return -1;
> > +    }
> > +
> > +    switch (run->exit_reason) {
> > +    case KVM_EXIT_X86_RDMSR:
> > +        run->msr.error = 0;
> > +        run->msr.data = msr_kvm_migration_control;
> > +        break;
> > +    case KVM_EXIT_X86_WRMSR:
> > +        msr_kvm_migration_control = run->msr.data;
> > +        if (run->msr.data == KVM_MIGRATION_READY) {
> > +            sev_del_migrate_blocker();
> 
> It seems this is enabled/disabled by the guest, which means that the guest can always refuse to be migrated?
> 

Yes.

Are there any specific concerns/issues with that ?

Thanks,
Ashish

RE: [PATCH v4 14/14] kvm: Add support for userspace MSR filtering and handling of MSR_KVM_MIGRATION_CONTROL.

[Wang, Wei W]

On Friday, September 10, 2021 5:14 PM, Ashish Kalra wrote:
> > It seems this is enabled/disabled by the guest, which means that the guest
> can always refuse to be migrated?
> >
> 
> Yes.
> 
> Are there any specific concerns/issues with that ?

It's kind of wired if everybody rejects to migrate from the guest
but they ticked the option "agree to be migrated" when they buy the guest
(e.g. at a discounted price).

In general, I think the migration decision should be made by the management s/w, not the migratable guest.

Best,
Wei

Re: [PATCH v4 10/14] migration: add support to migrate shared regions list

[Ashish Kalra]

On Fri, Sep 10, 2021 at 09:11:09AM +0000, Wang, Wei W wrote:
> On Friday, September 10, 2021 4:48 PM, Ashish Kalra wrote:
> > On Fri, Sep 10, 2021 at 07:54:10AM +0000, Wang, Wei W wrote:
> > There has been a long discussion on this implementation on KVM mailing list.
> > Tracking shared memory via a list of ranges instead of using bitmap is more
> > optimal. Most of the guest memory will be private and the unencrypted/shared
> > regions are basically ranges/intervals, so easy to implement and maintain using
> > lists.
> 
> OK. At which version did you discuss this or do you have a link? (I didn't find it in v9 KVM patches)
> 

You can follow this email thread:
https://lore.kernel.org/kvm/20201211225542.GA30409@ashkalra_ubuntu_server/

> > A list will consume much less memory than a bitmap.
> > 
> > The bitmap will consume more memory as it will need to be sized as per guest
> > RAM size and will remain sparsely populated due to limited amount of
> > shared/unencrypted guest memory regions.
> 
> I also thought about this. It depends on the guest.
> I think "A list will consume much less memory" is true when we assume most of guest pages are private pages.
> From design perspective, what if guest chooses to have most of its pages being shared?
> Lists might consume much more memory than bitmaps in some cases, I think.
> (Probably I can check your previous discussions first)
> 

This will probably depend on the most common use case and scenario, i
think that most common use case will be a mostly encrypted guest.

Thanks,
Ashish

Re: [PATCH v4 13/14] migration: for SEV live migration bump downtime limit to 1s.

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 11:59:47AM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Now, qemu has a default expected downtime of 300 ms and
> SEV Live migration has a page-per-second bandwidth of 350-450 pages
> ( SEV Live migration being generally slow due to guest RAM pages
> being migrated after encryption using the security processor ).
> With this expected downtime of 300ms and 350-450 pps bandwith,
> the threshold size = <1/3 of the PPS bandwidth = ~100 pages.
> 
> Now, this threshold size is the maximum pages/bytes that can be
> sent in the final completion phase of Live migration
> (where the source VM is stopped) with the expected downtime.
> Therefore, with the threshold size computed above,
> the migration completion phase which halts the source VM
> and then transfers the leftover dirty pages,
> is only reached in SEV live migration case when # of dirty pages are ~100.
> 
> The dirty-pages-rate with larger guest RAM configuration like 4G, 8G, etc.
> is much higher, typically in the range of 300-400+ pages, hence,
> we always remain in the "dirty-sync" phase of migration and never
> reach the migration completion phase with above guest RAM configs.
> 
> To summarize, with larger guest RAM configs,
> the dirty-pages-rate > threshold_size (with the default qemu expected downtime of 300ms).
> 
> So, the fix is to increase qemu's expected downtime.
> 
> This is a tweakable parameter which can be set using "migrate_set_downtime".
> 
> With a downtime of 1 second, we get a threshold size of ~350-450 pages,
> which will handle the "dirty-pages-rate" of 300+ pages and complete
> the migration process, so we bump the default downtime to 1s in case
> of SEV live migration being active.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  migration/migration.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index daea3ecd04..c9bc33fb10 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3568,6 +3568,10 @@ static void migration_update_counters(MigrationState *s,
>      transferred = current_bytes - s->iteration_initial_bytes;
>      time_spent = current_time - s->iteration_start_time;
>      bandwidth = (double)transferred / time_spent;
> +    if (memcrypt_enabled() &&
> +        s->parameters.downtime_limit < 1000) {
> +        s->parameters.downtime_limit = 1000;
> +    }

I don't think we can be silently changing a value set by the mgmt
app. If the app requests 300 ms downtime, then we *must* honour
that, because it is driven by the SLA they need to privide to the
guest user's workload. If it means the migration won't complete,
it is up to the app to deal with that in some manner.

At most I think this is a documentation task to give guidance to
mgmt apps about what special SEV-only things to consider whe tuning
live migration.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 02/14] doc: update AMD SEV to include Live migration flow

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 11:53:47AM +0000, Ashish Kalra wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  docs/amd-memory-encryption.txt | 46 +++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index 12ca25180e..0d9184532a 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -126,7 +126,51 @@ TODO
>  
>  Live Migration
>  ----------------

> +NOTE:
> +To protect against the memory clone SEV APIs are designed to make the VM
> +unrunnable in case of the migration failure.

Can you expand on this, as the limited explanation does not make a
whole lot of sense. What is the threat model here, what actions
are being taken to ensure unrunnability and who or what enforces
that ?  Which VM is this referring to - the src VM or dst VM ?

I comes across like you're trying to protect against the case where
the same VM is executing on both hosts concurrently, but I'm not
clear how that ties into migration failure.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 13/14] migration: for SEV live migration bump downtime limit to 1s.

[Ashish Kalra via]

On Fri, Sep 10, 2021 at 10:43:50AM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 04, 2021 at 11:59:47AM +0000, Ashish Kalra wrote:
> > From: Ashish Kalra <ashish.kalra@amd.com>
> > 
> > Now, qemu has a default expected downtime of 300 ms and
> > SEV Live migration has a page-per-second bandwidth of 350-450 pages
> > ( SEV Live migration being generally slow due to guest RAM pages
> > being migrated after encryption using the security processor ).
> > With this expected downtime of 300ms and 350-450 pps bandwith,
> > the threshold size = <1/3 of the PPS bandwidth = ~100 pages.
> > 
> > Now, this threshold size is the maximum pages/bytes that can be
> > sent in the final completion phase of Live migration
> > (where the source VM is stopped) with the expected downtime.
> > Therefore, with the threshold size computed above,
> > the migration completion phase which halts the source VM
> > and then transfers the leftover dirty pages,
> > is only reached in SEV live migration case when # of dirty pages are ~100.
> > 
> > The dirty-pages-rate with larger guest RAM configuration like 4G, 8G, etc.
> > is much higher, typically in the range of 300-400+ pages, hence,
> > we always remain in the "dirty-sync" phase of migration and never
> > reach the migration completion phase with above guest RAM configs.
> > 
> > To summarize, with larger guest RAM configs,
> > the dirty-pages-rate > threshold_size (with the default qemu expected downtime of 300ms).
> > 
> > So, the fix is to increase qemu's expected downtime.
> > 
> > This is a tweakable parameter which can be set using "migrate_set_downtime".
> > 
> > With a downtime of 1 second, we get a threshold size of ~350-450 pages,
> > which will handle the "dirty-pages-rate" of 300+ pages and complete
> > the migration process, so we bump the default downtime to 1s in case
> > of SEV live migration being active.
> > 
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > ---
> >  migration/migration.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index daea3ecd04..c9bc33fb10 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -3568,6 +3568,10 @@ static void migration_update_counters(MigrationState *s,
> >      transferred = current_bytes - s->iteration_initial_bytes;
> >      time_spent = current_time - s->iteration_start_time;
> >      bandwidth = (double)transferred / time_spent;
> > +    if (memcrypt_enabled() &&
> > +        s->parameters.downtime_limit < 1000) {
> > +        s->parameters.downtime_limit = 1000;
> > +    }
> 
> I don't think we can be silently changing a value set by the mgmt
> app. If the app requests 300 ms downtime, then we *must* honour
> that, because it is driven by the SLA they need to privide to the
> guest user's workload. If it means the migration won't complete,
> it is up to the app to deal with that in some manner.
> 
> At most I think this is a documentation task to give guidance to
> mgmt apps about what special SEV-only things to consider whe tuning
> live migration.
> 

Yes, this makes sense to add this as part of SEV live migration
documentation. Also this is a tuneable parameter with
migrate-set-parameters, so this can be configured or adjusted
accordingly. 

Thanks,
Ashish

> -- 
> |: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fberrange.com%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C475027a3a7554e0f139f08d9743f8a2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637668638521013586%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tUfEk6CEPwP79VRX%2BAQ4FAm7JgwKDfOv9cgGmETgsQk%3D&amp;reserved=0      -o-    https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.flickr.com%2Fphotos%2Fdberrange&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C475027a3a7554e0f139f08d9743f8a2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637668638521013586%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WHR%2FMZo4c3MZ0IWkZalvwP3V53fV9Grc6VqA%2FGxCo4s%3D&amp;reserved=0 :|
> |: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flibvirt.org%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C475027a3a7554e0f139f08d9743f8a2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637668638521013586%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=K7c0HWjylWIf9joMj7IqGUEoFA%2BWpOq6EeYgsCgP5XA%3D&amp;reserved=0         -o-            https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ffstop138.berrange.com%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C475027a3a7554e0f139f08d9743f8a2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637668638521013586%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=n%2BTiLz2WHORv07TEjTGDA2JIKjfmJ0WuNR7CbXzdA8c%3D&amp;reserved=0 :|
> |: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fentangle-photo.org%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C475027a3a7554e0f139f08d9743f8a2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637668638521013586%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=v5yyZXY6XH4TBle7oXCPSVy85PX1INJgrwIlW40Gydg%3D&amp;reserved=0    -o-    https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.instagram.com%2Fdberrange&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C475027a3a7554e0f139f08d9743f8a2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637668638521013586%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=biI3OcZyTZe7Gfn8RrWM1tGImANHmJeaYeSNel0rzg4%3D&amp;reserved=0 :|
>