[Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support

[Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support

[Singh, Brijesh]

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 patch series depends on kernel patches available here:
https://marc.info/?l=kvm&m=156278967226011&w=2

The complete tree with patch is available at:
https://github.com/codomania/qemu/tree/sev-migration-v2

Changes since v1:
 - use the dirty log sync APIs to also sync the page encryption bitmap
   when SEV is active.

Brijesh Singh (13):
  linux-headers: update kernel header to include SEV migration commands
  kvm: introduce high-level API to support encrypted page migration
  migration/ram: add support to send encrypted pages
  kvm: add support to sync the page encryption state bitmap
  doc: update AMD SEV API spec web link
  doc: update AMD SEV to include Live migration flow
  target/i386: sev: do not create launch context for an incoming guest
  misc.json: add migrate-set-sev-info command
  target/i386: sev: add support to encrypt the outgoing page
  target/i386: sev: add support to load incoming encrypted page
  kvm: introduce high-level API to migrate the page encryption bitmap
  migration: add support to migrate page encryption bitmap
  target/i386: sev: remove migration blocker

 accel/kvm/kvm-all.c            | 108 ++++++++
 accel/kvm/sev-stub.c           |  22 ++
 accel/stubs/kvm-stub.c         |  22 ++
 docs/amd-memory-encryption.txt |  44 +++-
 include/exec/ram_addr.h        | 161 +++++++++++-
 include/exec/ramlist.h         |   3 +-
 include/sysemu/kvm.h           |  25 ++
 include/sysemu/sev.h           |   6 +
 linux-headers/linux/kvm.h      |  53 ++++
 migration/ram.c                |  91 ++++++-
 qapi/misc-target.json          |  18 ++
 target/i386/monitor.c          |  10 +
 target/i386/sev-stub.c         |   5 +
 target/i386/sev.c              | 455 +++++++++++++++++++++++++++++++--
 target/i386/sev_i386.h         |  11 +-
 target/i386/trace-events       |   8 +
 16 files changed, 1016 insertions(+), 26 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 01/13] linux-headers: update kernel header to include SEV migration commands

[Singh, Brijesh]

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 linux-headers/linux/kvm.h | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index c8423e760c..2b0a2a97b8 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -492,6 +492,16 @@ struct kvm_dirty_log {
 	};
 };
 
+/* for KVM_GET_PAGE_ENC_BITMAP */
+struct kvm_page_enc_bitmap {
+        __u64 start_gfn;
+        __u64 num_pages;
+	union {
+		void *enc_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
 /* for KVM_CLEAR_DIRTY_LOG */
 struct kvm_clear_dirty_log {
 	__u32 slot;
@@ -1451,6 +1461,9 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+#define KVM_GET_PAGE_ENC_BITMAP  	 _IOW(KVMIO, 0xc2, struct kvm_page_enc_bitmap)
+#define KVM_SET_PAGE_ENC_BITMAP  	 _IOW(KVMIO, 0xc3, struct kvm_page_enc_bitmap)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */
@@ -1531,6 +1544,46 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {
+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_cert_uaddr;
+	__u32 plat_cert_len;
+	__u64 amd_cert_uaddr;
+	__u32 amd_cert_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};
+
+struct kvm_sev_send_update_data {
+	__u64 hdr_uaddr;
+	__u32 hdr_len;
+	__u64 guest_uaddr;
+	__u32 guest_len;
+	__u64 trans_uaddr;
+	__u32 trans_len;
+};
+
+struct kvm_sev_receive_start {
+	__u32 handle;
+	__u32 policy;
+	__u64 pdh_uaddr;
+	__u32 pdh_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};
+
+struct kvm_sev_receive_update_data {
+	__u64 hdr_uaddr;
+	__u32 hdr_len;
+	__u64 guest_uaddr;
+	__u32 guest_len;
+	__u64 trans_uaddr;
+	__u32 trans_len;
+};
+
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.17.1

[Qemu-devel] [PATCH v2 02/13] kvm: introduce high-level API to support encrypted page migration

[Singh, Brijesh]

When memory encryption is enabled in VM, the guest pages will be
encrypted with the guest-specific key, to protect the confidentiality
of data in transit. To support the live migration we need to use
platform specific hooks to access the guest memory.

The kvm_memcrypt_save_outgoing_page() can be used by the sender to write
the encrypted pages and metadata associated with it on the socket.

The kvm_memcrypt_load_incoming_page() can be used by receiver to read the
incoming encrypted pages from the socket and load into the guest memory.

Signed-off-by: Brijesh Singh <<brijesh.singh@amd.com>>
---
 accel/kvm/kvm-all.c    | 27 +++++++++++++++++++++++++++
 accel/kvm/sev-stub.c   | 11 +++++++++++
 accel/stubs/kvm-stub.c | 12 ++++++++++++
 include/sysemu/kvm.h   | 12 ++++++++++++
 include/sysemu/sev.h   |  3 +++
 5 files changed, 65 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 3d86ae5052..162a2d5085 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -110,6 +110,10 @@ struct KVMState
     /* memory encryption */
     void *memcrypt_handle;
     int (*memcrypt_encrypt_data)(void *handle, uint8_t *ptr, uint64_t len);
+    int (*memcrypt_save_outgoing_page)(void *ehandle, QEMUFile *f,
+            uint8_t *ptr, uint32_t sz, uint64_t *bytes_sent);
+    int (*memcrypt_load_incoming_page)(void *ehandle, QEMUFile *f,
+            uint8_t *ptr);
 };
 
 KVMState *kvm_state;
@@ -165,6 +169,29 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
     return 1;
 }
 
+int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
+                                    uint32_t size, uint64_t *bytes_sent)
+{
+    if (kvm_state->memcrypt_handle &&
+        kvm_state->memcrypt_save_outgoing_page) {
+        return kvm_state->memcrypt_save_outgoing_page(kvm_state->memcrypt_handle,
+                    f, ptr, size, bytes_sent);
+    }
+
+    return 1;
+}
+
+int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
+{
+    if (kvm_state->memcrypt_handle &&
+        kvm_state->memcrypt_load_incoming_page) {
+        return kvm_state->memcrypt_load_incoming_page(kvm_state->memcrypt_handle,
+                    f, ptr);
+    }
+
+    return 1;
+}
+
 static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
 {
     KVMState *s = kvm_state;
diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
index 4f97452585..c12a8e005e 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -24,3 +24,14 @@ void *sev_guest_init(const char *id)
 {
     return NULL;
 }
+
+int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
+                           uint32_t size, uint64_t *bytes_sent)
+{
+    return 1;
+}
+
+int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
+{
+    return 1;
+}
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 6feb66ed80..e14b879531 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -114,6 +114,18 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
   return 1;
 }
 
+int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
+                                    uint32_t size, uint64_t *bytes_sent)
+{
+    return 1;
+}
+
+int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
+{
+    return 1;
+}
+
+
 #ifndef CONFIG_USER_ONLY
 int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
 {
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index acd90aebb6..bb6bcc143c 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -247,6 +247,18 @@ bool kvm_memcrypt_enabled(void);
  */
 int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
 
+/**
+ * kvm_memcrypt_save_outgoing_buffer - encrypt the outgoing buffer
+ * and write to the wire.
+ */
+int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr, uint32_t size,
+                                    uint64_t *bytes_sent);
+
+/**
+ * kvm_memcrypt_load_incoming_buffer - read the encrypt incoming buffer and copy
+ * the buffer into the guest memory space.
+ */
+int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr);
 
 #ifdef NEED_CPU_H
 #include "cpu.h"
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 98c1ec8d38..752a71b1c0 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -18,4 +18,7 @@
 
 void *sev_guest_init(const char *id);
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
+int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
+                           uint32_t size, uint64_t *bytes_sent);
+int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr);
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v2 03/13] migration/ram: add support to send encrypted pages

[Singh, Brijesh]

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 kvm_memcrypt_save_outgoing_page() is used
by the sender to write the encrypted pages onto the socket, similarly the
kvm_memcrypt_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>
---
 migration/ram.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 908517fc2b..3c8977d508 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -57,6 +57,7 @@
 #include "qemu/uuid.h"
 #include "savevm.h"
 #include "qemu/iov.h"
+#include "sysemu/kvm.h"
 
 /***********************************************************/
 /* ram save/restore */
@@ -76,6 +77,7 @@
 #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_PAGE   0x200
 
 static inline bool is_zero_range(uint8_t *p, uint64_t size)
 {
@@ -460,6 +462,9 @@ 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)
 {
     CompressParam *param = opaque;
@@ -2006,6 +2011,36 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
     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;
+
+    p = block->host + offset;
+
+    ram_counters.transferred +=
+        save_page_header(rs, rs->f, block,
+                    offset | RAM_SAVE_FLAG_ENCRYPTED_PAGE);
+
+    ret = kvm_memcrypt_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;
+}
+
 static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                  ram_addr_t offset, uint8_t *source_buf)
 {
@@ -2450,6 +2485,16 @@ 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 (kvm_memcrypt_enabled()) {
+        return ram_save_encrypted_page(rs, pss, last_stage);
+     }
+
     if (save_compress_page(rs, block, offset)) {
         return 1;
     }
@@ -4271,7 +4316,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
         }
 
         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_PAGE)) {
             RAMBlock *block = ram_block_from_stream(f, flags);
 
             /*
@@ -4391,6 +4437,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
                 break;
             }
             break;
+        case RAM_SAVE_FLAG_ENCRYPTED_PAGE:
+            if (kvm_memcrypt_load_incoming_page(f, host)) {
+                    error_report("Failed to encrypted incoming data");
+                    ret = -EINVAL;
+            }
+            break;
         case RAM_SAVE_FLAG_EOS:
             /* normal exit */
             multifd_recv_sync_main();
-- 
2.17.1

[Qemu-devel] [PATCH v2 05/13] doc: update AMD SEV API spec web link

[Singh, Brijesh]

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 docs/amd-memory-encryption.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
index 43bf3ee6a5..abb9a976f5 100644
--- a/docs/amd-memory-encryption.txt
+++ b/docs/amd-memory-encryption.txt
@@ -98,7 +98,7 @@ AMD Memory Encryption whitepaper:
 http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf
 
 Secure Encrypted Virtualization Key Management:
-[1] http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
+[1] https://developer.amd.com/sev/ (Secure Encrypted Virtualization API)
 
 KVM Forum slides:
 http://www.linux-kvm.org/images/7/74/02x08A-Thomas_Lendacky-AMDs_Virtualizatoin_Memory_Encryption_Technology.pdf
-- 
2.17.1

[Qemu-devel] [PATCH v2 04/13] kvm: add support to sync the page encryption state bitmap

[Singh, Brijesh]

The SEV VMs have concept of private and shared memory. The private memory
is encrypted with guest-specific key, while shared memory may be encrypted
with hyperivosr key. The KVM_GET_PAGE_ENC_BITMAP can be used to get a
bitmap indicating whether the guest page is private or shared. A private
page must be transmitted using the SEV migration commands.

Add a cpu_physical_memory_sync_encrypted_bitmap() which can be used to sync
the page encryption bitmap for a given memory region.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 accel/kvm/kvm-all.c     |  38 ++++++++++
 include/exec/ram_addr.h | 161 ++++++++++++++++++++++++++++++++++++++--
 include/exec/ramlist.h  |   3 +-
 migration/ram.c         |  28 ++++++-
 4 files changed, 222 insertions(+), 8 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 162a2d5085..c935e9366c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -504,6 +504,37 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
 
 #define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
 
+/* sync page_enc bitmap */
+static int kvm_sync_page_enc_bitmap(KVMMemoryListener *kml,
+                                    MemoryRegionSection *section,
+                                    KVMSlot *mem)
+{
+    unsigned long size;
+    KVMState *s = kvm_state;
+    struct kvm_page_enc_bitmap e = {};
+    ram_addr_t pages = int128_get64(section->size) / getpagesize();
+    ram_addr_t start = section->offset_within_region +
+                       memory_region_get_ram_addr(section->mr);
+
+    size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
+                 /*HOST_LONG_BITS*/ 64) / 8;
+    e.enc_bitmap = g_malloc0(size);
+    e.start_gfn = mem->start_addr >> TARGET_PAGE_BITS;
+    e.num_pages = pages;
+    if (kvm_vm_ioctl(s, KVM_GET_PAGE_ENC_BITMAP, &e) == -1) {
+        DPRINTF("KVM_GET_PAGE_ENC_BITMAP ioctl failed %d\n", errno);
+        g_free(e.enc_bitmap);
+        return 1;
+    }
+
+    cpu_physical_memory_set_encrypted_lebitmap(e.enc_bitmap,
+                                               start, pages);
+
+    g_free(e.enc_bitmap);
+
+    return 0;
+}
+
 /**
  * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
  * This function updates qemu's dirty bitmap using
@@ -553,6 +584,13 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
         }
 
         kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
+
+        if (kvm_memcrypt_enabled() &&
+            kvm_sync_page_enc_bitmap(kml, section, mem)) {
+            g_free(d.dirty_bitmap);
+            return -1;
+        }
+
         g_free(d.dirty_bitmap);
     }
 
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index f96777bb99..6fc6864194 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -51,6 +51,8 @@ struct RAMBlock {
     unsigned long *unsentmap;
     /* bitmap of already received pages in postcopy */
     unsigned long *receivedmap;
+    /* bitmap of page encryption state for an encrypted guest */
+    unsigned long *encbmap;
 };
 
 static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
@@ -314,9 +316,41 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
 }
 
 #if !defined(_WIN32)
-static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
+
+static inline void cpu_physical_memory_set_encrypted_range(ram_addr_t start,
+                                                           ram_addr_t length,
+                                                           unsigned long val)
+{
+    unsigned long end, page;
+    unsigned long * const *src;
+
+    if (length == 0) {
+        return;
+    }
+
+    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
+    page = start >> TARGET_PAGE_BITS;
+
+    rcu_read_lock();
+
+    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
+
+    while (page < end) {
+        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
+
+        atomic_xchg(&src[idx][BIT_WORD(offset)], val);
+        page += num;
+    }
+
+    rcu_read_unlock();
+}
+
+static inline void cpu_physical_memory_set_dirty_enc_lebitmap(unsigned long *bitmap,
                                                           ram_addr_t start,
-                                                          ram_addr_t pages)
+                                                          ram_addr_t pages,
+                                                          bool enc_map)
 {
     unsigned long i, j;
     unsigned long page_number, c;
@@ -349,10 +383,14 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
             if (bitmap[k]) {
                 unsigned long temp = leul_to_cpu(bitmap[k]);
 
-                atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
-                atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
-                if (tcg_enabled()) {
-                    atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
+                if (enc_map) {
+                    atomic_xchg(&blocks[DIRTY_MEMORY_ENCRYPTED][idx][offset], temp);
+                } else {
+                    atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
+                    atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
+                    if (tcg_enabled()) {
+                        atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
+                    }
                 }
             }
 
@@ -372,6 +410,17 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
          * especially when most of the memory is not dirty.
          */
         for (i = 0; i < len; i++) {
+
+            /* If its encrypted bitmap update, then we need to copy the bitmap
+             * value as-is to the destination.
+             */
+            if (enc_map) {
+                cpu_physical_memory_set_encrypted_range(start + i * TARGET_PAGE_SIZE,
+                                                        TARGET_PAGE_SIZE * hpratio,
+                                                        leul_to_cpu(bitmap[i]));
+                continue;
+            }
+
             if (bitmap[i] != 0) {
                 c = leul_to_cpu(bitmap[i]);
                 do {
@@ -387,6 +436,21 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
         }
     }
 }
+
+static inline void cpu_physical_memory_set_encrypted_lebitmap(unsigned long *bitmap,
+                                                              ram_addr_t start,
+                                                              ram_addr_t pages)
+{
+    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, true);
+}
+
+static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
+                                                          ram_addr_t start,
+                                                          ram_addr_t pages)
+{
+    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, false);
+}
+
 #endif /* not _WIN32 */
 
 bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
@@ -406,6 +470,7 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
     cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
     cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
     cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
+    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_ENCRYPTED);
 }
 
 
@@ -474,5 +539,89 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
 
     return num_dirty;
 }
+
+static inline bool cpu_physical_memory_test_encrypted(ram_addr_t start,
+                                                      ram_addr_t length)
+{
+    unsigned long end, page;
+    bool enc = false;
+    unsigned long * const *src;
+
+    if (length == 0) {
+        return enc;
+    }
+
+    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
+    page = start >> TARGET_PAGE_BITS;
+
+    rcu_read_lock();
+
+    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
+
+    while (page < end) {
+        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
+
+        enc |= atomic_read(&src[idx][BIT_WORD(offset)]);
+        page += num;
+    }
+
+    rcu_read_unlock();
+
+    return enc;
+}
+
+static inline
+void cpu_physical_memory_sync_encrypted_bitmap(RAMBlock *rb,
+                                               ram_addr_t start,
+                                               ram_addr_t length)
+{
+    ram_addr_t addr;
+    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
+    unsigned long *dest = rb->encbmap;
+
+    /* start address and length is aligned at the start of a word? */
+    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
+         (start + rb->offset) &&
+        !(length & ((BITS_PER_LONG << TARGET_PAGE_BITS) - 1))) {
+        int k;
+        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
+        unsigned long * const *src;
+        unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
+        unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
+                                        DIRTY_MEMORY_BLOCK_SIZE);
+        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
+
+        rcu_read_lock();
+
+        src = atomic_rcu_read(
+                &ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
+
+        for (k = page; k < page + nr; k++) {
+            unsigned long bits = atomic_read(&src[idx][offset]);
+            dest[k] = bits;
+
+            if (++offset >= BITS_TO_LONGS(DIRTY_MEMORY_BLOCK_SIZE)) {
+                offset = 0;
+                idx++;
+            }
+        }
+
+        rcu_read_unlock();
+    } else {
+        ram_addr_t offset = rb->offset;
+
+        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
+            long k = (start + addr) >> TARGET_PAGE_BITS;
+            if (cpu_physical_memory_test_encrypted(start + addr + offset,
+                                                   TARGET_PAGE_SIZE)) {
+                set_bit(k, dest);
+            } else {
+                clear_bit(k, dest);
+            }
+        }
+    }
+}
 #endif
 #endif
diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
index bc4faa1b00..2a5eab8b11 100644
--- a/include/exec/ramlist.h
+++ b/include/exec/ramlist.h
@@ -11,7 +11,8 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
 #define DIRTY_MEMORY_VGA       0
 #define DIRTY_MEMORY_CODE      1
 #define DIRTY_MEMORY_MIGRATION 2
-#define DIRTY_MEMORY_NUM       3        /* num of dirty bits */
+#define DIRTY_MEMORY_ENCRYPTED 3
+#define DIRTY_MEMORY_NUM       4        /* num of dirty bits */
 
 /* The dirty memory bitmap is split into fixed-size blocks to allow growth
  * under RCU.  The bitmap for a block can be accessed as follows:
diff --git a/migration/ram.c b/migration/ram.c
index 3c8977d508..d179867e1b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1680,6 +1680,9 @@ static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
     rs->migration_dirty_pages +=
         cpu_physical_memory_sync_dirty_bitmap(rb, 0, length,
                                               &rs->num_dirty_pages_period);
+    if (kvm_memcrypt_enabled()) {
+        cpu_physical_memory_sync_encrypted_bitmap(rb, 0, length);
+    }
 }
 
 /**
@@ -2465,6 +2468,22 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
     return false;
 }
 
+/**
+ * encrypted_test_bitmap: check if the page is encrypted
+ *
+ * Returns a bool indicating whether the page is encrypted.
+ */
+static bool encrypted_test_bitmap(RAMState *rs, RAMBlock *block,
+                                  unsigned long page)
+{
+    /* ROM devices contains the unencrypted data */
+    if (memory_region_is_rom(block->mr)) {
+        return false;
+    }
+
+    return test_bit(page, block->encbmap);
+}
+
 /**
  * ram_save_target_page: save one target page
  *
@@ -2491,7 +2510,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
      * will take care of accessing the guest memory and re-encrypt it
      * for the transport purposes.
      */
-     if (kvm_memcrypt_enabled()) {
+     if (kvm_memcrypt_enabled() &&
+         encrypted_test_bitmap(rs, pss->block, pss->page)) {
         return ram_save_encrypted_page(rs, pss, last_stage);
      }
 
@@ -2724,6 +2744,8 @@ static void ram_save_cleanup(void *opaque)
         block->bmap = NULL;
         g_free(block->unsentmap);
         block->unsentmap = NULL;
+        g_free(block->encbmap);
+        block->encbmap = NULL;
     }
 
     xbzrle_cleanup();
@@ -3251,6 +3273,10 @@ static void ram_list_init_bitmaps(void)
                 block->unsentmap = bitmap_new(pages);
                 bitmap_set(block->unsentmap, 0, pages);
             }
+            if (kvm_memcrypt_enabled()) {
+                block->encbmap = bitmap_new(pages);
+                bitmap_set(block->encbmap, 0, pages);
+            }
         }
     }
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 06/13] doc: update AMD SEV to include Live migration flow

[Singh, Brijesh]

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 docs/amd-memory-encryption.txt | 42 +++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
index abb9a976f5..374f4b0a94 100644
--- a/docs/amd-memory-encryption.txt
+++ b/docs/amd-memory-encryption.txt
@@ -89,7 +89,47 @@ 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_GET_PAGE_ENC_BITMAP ioctl can be used to get the guest page encryption
+bitmap. The bitmap 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-sev-info object can be used to pass the target machine's PDH and
+certificate chain.
+
+e.g
+(QMP) migrate-sev-set-info pdh=<target_pdh> plat-cert=<target_cert_chain> \
+       amd-cert=<amd_cert>
+(QMP) migrate tcp:0:4444
+
+
+During the migration flow, the SEND_START is called on the source hypervisor
+to create outgoing encryption context. The SEV guest policy dectates whether
+the certificate passed through the migrate-sev-set-info command will be
+validate. 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 the 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 receieved 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).
 
 References
 -----------------
-- 
2.17.1

[Qemu-devel] [PATCH v2 07/13] target/i386: sev: do not create launch context for an incoming guest

[Singh, Brijesh]

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.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6dbdc3cdf1..49baf8fef0 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -789,10 +789,16 @@ sev_guest_init(const char *id)
         goto err;
     }
 
-    ret = sev_launch_start(s);
-    if (ret) {
-        error_report("%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(s);
+        if (ret) {
+            error_report("%s: failed to create encryption context", __func__);
+            goto err;
+        }
     }
 
     ram_block_notifier_add(&sev_ram_notifier);
-- 
2.17.1

[Qemu-devel] [PATCH v2 08/13] misc.json: add migrate-set-sev-info command

[Singh, Brijesh]

The command can be used by the hypervisor to specify the target Platform
Diffie-Hellman key (PDH) and certificate chain before starting the SEV
guest migration. The values passed through the command will be used while
creating the outgoing encryption context.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 qapi/misc-target.json  | 18 ++++++++++++++++++
 target/i386/monitor.c  | 10 ++++++++++
 target/i386/sev-stub.c |  5 +++++
 target/i386/sev.c      | 11 +++++++++++
 target/i386/sev_i386.h |  9 ++++++++-
 5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index a00fd821eb..938dcaea14 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -266,3 +266,21 @@
 ##
 { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
   'if': 'defined(TARGET_ARM)' }
+
+##
+# @migrate-set-sev-info:
+#
+# The command is used to provide the target host information used during the
+# SEV guest.
+#
+# @pdh the target host platform diffie-hellman key encoded in base64
+#
+# @plat-cert the target host platform certificate chain encoded in base64
+#
+# @amd-cert AMD certificate chain which include ASK and OCA encoded in base64
+#
+# Since 4.2
+#
+##
+{ 'command': 'migrate-set-sev-info',
+  'data': { 'pdh': 'str', 'plat-cert': 'str', 'amd-cert' : 'str' }}
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 1f3b532fc2..4a5f50fb45 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -736,3 +736,13 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
 
     return data;
 }
+
+void qmp_migrate_set_sev_info(const char *pdh, const char *plat_cert,
+                              const char *amd_cert, Error **errp)
+{
+    if (sev_enabled()) {
+        sev_set_migrate_info(pdh, plat_cert, amd_cert);
+    } else {
+        error_setg(errp, "SEV is not enabled");
+    }
+}
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index e5ee13309c..173bfa6374 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
 {
     return NULL;
 }
+
+void sev_set_migrate_info(const char *pdh, const char *plat_cert,
+                          const char *amd_cert)
+{
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 49baf8fef0..6c902d0be8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -825,6 +825,17 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
     return 0;
 }
 
+void sev_set_migrate_info(const char *pdh, const char *plat_cert,
+                          const char *amd_cert)
+{
+    SEVState *s = sev_state;
+
+    s->remote_pdh = g_base64_decode(pdh, &s->remote_pdh_len);
+    s->remote_plat_cert = g_base64_decode(plat_cert,
+                                          &s->remote_plat_cert_len);
+    s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 55313441ae..3f3449b346 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -39,7 +39,8 @@ extern uint32_t sev_get_cbit_position(void);
 extern uint32_t sev_get_reduced_phys_bits(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(void);
-
+extern void sev_set_migrate_info(const char *pdh, const char *plat_cert,
+                                 const char *amd_cert);
 typedef struct QSevGuestInfo QSevGuestInfo;
 typedef struct QSevGuestInfoClass QSevGuestInfoClass;
 
@@ -81,6 +82,12 @@ struct SEVState {
     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;
 };
 
 typedef struct SEVState SEVState;
-- 
2.17.1

[Qemu-devel] [PATCH v2 09/13] target/i386: sev: add support to encrypt the outgoing page

[Singh, Brijesh]

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>
---
 accel/kvm/kvm-all.c      |   1 +
 target/i386/sev.c        | 229 +++++++++++++++++++++++++++++++++++++++
 target/i386/sev_i386.h   |   2 +
 target/i386/trace-events |   3 +
 4 files changed, 235 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c935e9366c..a9fb447248 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1792,6 +1792,7 @@ static int kvm_init(MachineState *ms)
         }
 
         kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
+        kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6c902d0be8..28b36c8035 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -27,6 +27,8 @@
 #include "sysemu/sysemu.h"
 #include "trace.h"
 #include "migration/blocker.h"
+#include "migration/qemu-file.h"
+#include "migration/misc.h"
 
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
@@ -718,6 +720,39 @@ sev_vm_state_change(void *opaque, int running, RunState state)
     }
 }
 
+static void
+sev_send_finish(void)
+{
+    int ret, error;
+
+    trace_kvm_sev_send_finish();
+    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
+    if (ret) {
+        error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",
+                     __func__, ret, error, fw_error_to_str(error));
+    }
+
+    sev_set_guest_state(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_STATE_SEND_UPDATE)) {
+            sev_send_finish();
+        }
+    }
+}
+
+static Notifier sev_migration_state_notify = {
+    .notify = sev_migration_state_notifier,
+};
+
 void *
 sev_guest_init(const char *id)
 {
@@ -804,6 +839,7 @@ sev_guest_init(const char *id)
     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, s);
+    add_migration_state_change_notifier(&sev_migration_state_notify);
 
     return s;
 err:
@@ -836,6 +872,199 @@ void sev_set_migrate_info(const char *pdh, const char *plat_cert,
     s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
 }
 
+static int
+sev_get_send_session_length(void)
+{
+    int ret, fw_err = 0;
+    struct kvm_sev_send_start *start;
+
+    start = g_new0(struct kvm_sev_send_start, 1);
+
+    ret = sev_ioctl(sev_state->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:
+    g_free(start);
+    return ret;
+}
+
+static int
+sev_send_start(SEVState *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;
+
+    if (!s->remote_pdh || !s->remote_plat_cert) {
+        error_report("%s: missing remote PDH or PLAT_CERT", __func__);
+        return 1;
+    }
+
+    start = g_new0(struct kvm_sev_send_start, 1);
+
+    start->pdh_cert_uaddr = (unsigned long) s->remote_pdh;
+    start->pdh_cert_len = s->remote_pdh_len;
+
+    start->plat_cert_uaddr = (unsigned long)s->remote_plat_cert;
+    start->plat_cert_len = s->remote_plat_cert_len;
+
+    start->amd_cert_uaddr = (unsigned long)s->amd_cert;
+    start->amd_cert_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);
+    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_cert_uaddr, start->plat_cert_len,
+                             start->amd_cert_uaddr, start->amd_cert_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(SEV_STATE_SEND_UPDATE);
+
+err:
+    g_free(start);
+    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;
+
+    update = g_malloc0(sizeof(*update));
+    if (!update) {
+        return -1;
+    }
+
+    ret = sev_ioctl(sev_state->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:
+    g_free(update);
+    return ret;
+}
+
+static int
+sev_send_update_data(SEVState *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);
+    }
+
+    update = g_new0(struct kvm_sev_send_update_data, 1);
+
+    /* allocate transport buffer */
+    trans = g_new(guchar, size);
+
+    update->hdr_uaddr = (unsigned long)s->send_packet_hdr;
+    update->hdr_len = s->send_packet_hdr_len;
+    update->guest_uaddr = (unsigned long)ptr;
+    update->guest_len = size;
+    update->trans_uaddr = (unsigned long)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);
+    g_free(update);
+    return ret;
+}
+
+int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
+                           uint32_t sz, uint64_t *bytes_sent)
+{
+    SEVState *s = sev_state;
+
+    /*
+     * If this is a first buffer then create outgoing encryption context
+     * and write our PDH, policy and session data.
+     */
+    if (!sev_check_state(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/sev_i386.h b/target/i386/sev_i386.h
index 3f3449b346..2fdca5190d 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -88,6 +88,8 @@ struct SEVState {
     size_t remote_plat_cert_len;
     guchar *amd_cert;
     size_t amd_cert_len;
+    gchar *send_packet_hdr;
+    size_t send_packet_hdr_len;
 };
 
 typedef struct SEVState SEVState;
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 789c700d4a..b41516cf9f 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -15,3 +15,6 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
 kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
 kvm_sev_launch_measurement(const char *value) "data %s"
 kvm_sev_launch_finish(void) ""
+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

[Qemu-devel] [PATCH v2 10/13] target/i386: sev: add support to load incoming encrypted page

[Singh, Brijesh]

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>
---
 accel/kvm/kvm-all.c      |   1 +
 target/i386/sev.c        | 126 ++++++++++++++++++++++++++++++++++++++-
 target/i386/trace-events |   3 +
 3 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a9fb447248..7f94dba6f9 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1793,6 +1793,7 @@ static int kvm_init(MachineState *ms)
 
         kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
         kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
+        kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 28b36c8035..09a62d6f88 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -708,13 +708,34 @@ sev_launch_finish(SEVState *s)
     }
 }
 
+static int
+sev_receive_finish(SEVState *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(SEV_STATE_RUNNING);
+err:
+    return ret;
+}
+
+
 static void
 sev_vm_state_change(void *opaque, int running, RunState state)
 {
     SEVState *s = opaque;
 
     if (running) {
-        if (!sev_check_state(SEV_STATE_RUNNING)) {
+        if (sev_check_state(SEV_STATE_RECEIVE_UPDATE)) {
+            sev_receive_finish(s);
+        } else if (!sev_check_state(SEV_STATE_RUNNING)) {
             sev_launch_finish(s);
         }
     }
@@ -1065,6 +1086,109 @@ int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
     return sev_send_update_data(s, f, ptr, sz, bytes_sent);
 }
 
+static int
+sev_receive_start(QSevGuestInfo *sev, QEMUFile *f)
+{
+    int ret = 1;
+    int fw_error;
+    struct kvm_sev_receive_start *start;
+    gchar *session = NULL, *pdh_cert = NULL;
+
+    start = g_new0(struct kvm_sev_receive_start, 1);
+
+    /* 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);
+    pdh_cert = g_new(gchar, start->pdh_len);
+    qemu_get_buffer(f, (uint8_t *)pdh_cert, start->pdh_len);
+    start->pdh_uaddr = (unsigned long)pdh_cert;
+
+    /* get source session data */
+    start->session_len = qemu_get_be32(f);
+    session = g_new(gchar, start->session_len);
+    qemu_get_buffer(f, (uint8_t *)session, start->session_len);
+    start->session_uaddr = (unsigned long)session;
+
+    trace_kvm_sev_receive_start(start->policy, session, pdh_cert);
+
+    ret = sev_ioctl(sev_state->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), start->handle, "handle", &error_abort);
+    sev_set_guest_state(SEV_STATE_RECEIVE_UPDATE);
+err:
+    g_free(start);
+    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;
+
+    update = g_new0(struct kvm_sev_receive_update_data, 1);
+
+    /* get packet header */
+    update->hdr_len = qemu_get_be32(f);
+    hdr = g_new(gchar, update->hdr_len);
+    qemu_get_buffer(f, (uint8_t *)hdr, update->hdr_len);
+    update->hdr_uaddr = (unsigned long)hdr;
+
+    /* get transport buffer */
+    update->trans_len = qemu_get_be32(f);
+    trans = g_new(gchar, update->trans_len);
+    update->trans_uaddr = (unsigned long)trans;
+    qemu_get_buffer(f, (uint8_t *)update->trans_uaddr, update->trans_len);
+
+    update->guest_uaddr = (unsigned long) 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_state->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(update);
+    g_free(hdr);
+    return ret;
+}
+
+int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
+{
+    SEVState *s = (SEVState *)handle;
+
+    /* 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(SEV_STATE_RECEIVE_UPDATE) &&
+        sev_receive_start(s->sev_info, 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 b41516cf9f..609752cca7 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -18,3 +18,6 @@ kvm_sev_launch_finish(void) ""
 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

[Qemu-devel] [PATCH v2 11/13] kvm: introduce high-level API to migrate the page encryption bitmap

[Singh, Brijesh]

Encrypted VMs have concept of private and shared memory. The private
memory is encrypted with the guest-specific key, while shared memory
may be encrypted with hyperivosr key. The guest OS uses a hypercall
to notify the page encryption state to the hypervisor. The hypervisor
maintain a bitmap of page encryption state. This bitmap should be
migrated to ensure that target machine can function correctly.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 accel/kvm/kvm-all.c    | 37 +++++++++++++++++++++++++++++++++++++
 accel/kvm/sev-stub.c   | 11 +++++++++++
 accel/stubs/kvm-stub.c | 10 ++++++++++
 include/sysemu/kvm.h   | 13 +++++++++++++
 include/sysemu/sev.h   |  3 +++
 5 files changed, 74 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 7f94dba6f9..442b1af36e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -114,6 +114,9 @@ struct KVMState
             uint8_t *ptr, uint32_t sz, uint64_t *bytes_sent);
     int (*memcrypt_load_incoming_page)(void *ehandle, QEMUFile *f,
             uint8_t *ptr);
+    int (*memcrypt_load_incoming_page_enc_bitmap)(void *ehandle, QEMUFile *f);
+    int (*memcrypt_save_outgoing_page_enc_bitmap)(void *ehandle, QEMUFile *f,
+            uint64_t start, uint64_t length);
 };
 
 KVMState *kvm_state;
@@ -192,6 +195,40 @@ int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
     return 1;
 }
 
+int kvm_memcrypt_load_incoming_page_enc_bitmap(QEMUFile *f)
+{
+    if (kvm_state->memcrypt_handle &&
+        kvm_state->memcrypt_load_incoming_page_enc_bitmap) {
+        return kvm_state->memcrypt_load_incoming_page_enc_bitmap(
+                kvm_state->memcrypt_handle, f);
+    }
+
+    return 1;
+}
+
+int kvm_memcrypt_save_outgoing_page_enc_bitmap(QEMUFile *f)
+{
+    KVMMemoryListener *kml = &kvm_state->memory_listener;
+    KVMState *s = kvm_state;
+    int ret = 1, i;
+
+    if (s->memcrypt_handle &&
+        s->memcrypt_save_outgoing_page_enc_bitmap) {
+
+        /* iterate through all the registered slots and send the bitmap */
+        for (i = 0; i < s->nr_slots; i++) {
+            KVMSlot *mem = &kml->slots[i];
+            ret = s->memcrypt_save_outgoing_page_enc_bitmap(s->memcrypt_handle,
+                        f, mem->start_addr, mem->memory_size);
+            if (ret) {
+                return 1;
+            }
+        }
+    }
+
+    return ret;
+}
+
 static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
 {
     KVMState *s = kvm_state;
diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
index c12a8e005e..7acd7211e6 100644
--- a/accel/kvm/sev-stub.c
+++ b/accel/kvm/sev-stub.c
@@ -35,3 +35,14 @@ int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
 {
     return 1;
 }
+
+int sev_load_incoming_page_enc_bitmap(void *handle, QEMUFile *f)
+{
+    return 1;
+}
+
+int sev_save_outgoing_page_enc_bitmap(void *handle, QEMUFile *f,
+                                      uint64_t start, uint64_t length)
+{
+    return 1;
+}
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index e14b879531..ae607787e7 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -125,6 +125,16 @@ int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
     return 1;
 }
 
+int kvm_memcrypt_load_incoming_page_enc_bitmap(QEMUFile *f)
+{
+    return 1;
+}
+
+int kvm_memcrypt_save_outgoing_page_enc_bitmap(QEMUFile *f)
+{
+    return 1;
+}
+
 
 #ifndef CONFIG_USER_ONLY
 int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index bb6bcc143c..8aa06b4462 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -260,6 +260,19 @@ int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr, uint32_t size,
  */
 int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr);
 
+/**
+ * kvm_memcrypt_load_incoming_page_enc_bitmap: read the page encryption bitmap
+ * from the socket and pass it to the hypervisor.
+ */
+int kvm_memcrypt_load_incoming_page_enc_bitmap(QEMUFile *f);
+
+/**
+ * kvm_memcrypt_save_outgoing_page_enc_bitmap: write the page encryption bitmap
+ * on socket.
+ */
+int kvm_memcrypt_save_outgoing_page_enc_bitmap(QEMUFile *f);
+
+
 #ifdef NEED_CPU_H
 #include "cpu.h"
 
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 752a71b1c0..e08886ca33 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -21,4 +21,7 @@ int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
 int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
                            uint32_t size, uint64_t *bytes_sent);
 int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr);
+int sev_load_incoming_page_enc_bitmap(void *handle, QEMUFile *f);
+int sev_save_outgoing_page_enc_bitmap(void *handle, QEMUFile *f,
+                                      uint64_t start, uint64_t length);
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v2 12/13] migration: add support to migrate page encryption bitmap

[Singh, Brijesh]

When memory encryption is enabled, the hypervisor maintains a page
encryption bitmap which is referred by hypervisor during migratoin to check
if page is private or shared. The bitmap 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. The KVM_{SET,GET}_PAGE_ENC_BITMAP can be used
to get and set the bitmap for a given gfn range.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 accel/kvm/kvm-all.c      |  4 +++
 migration/ram.c          | 11 +++++++
 target/i386/sev.c        | 67 ++++++++++++++++++++++++++++++++++++++++
 target/i386/trace-events |  2 ++
 4 files changed, 84 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 442b1af36e..9e23088a94 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1831,6 +1831,10 @@ static int kvm_init(MachineState *ms)
         kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
         kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
         kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
+        kvm_state->memcrypt_load_incoming_page_enc_bitmap =
+            sev_load_incoming_page_enc_bitmap;
+        kvm_state->memcrypt_save_outgoing_page_enc_bitmap =
+            sev_save_outgoing_page_enc_bitmap;
     }
 
     ret = kvm_arch_init(ms, s);
diff --git a/migration/ram.c b/migration/ram.c
index d179867e1b..3a4bdf3c03 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -78,6 +78,7 @@
 /* 0x80 is reserved in migration.h start with 0x100 next */
 #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
 #define RAM_SAVE_FLAG_ENCRYPTED_PAGE   0x200
+#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400 /* used in target/i386/sev.c */
 
 static inline bool is_zero_range(uint8_t *p, uint64_t size)
 {
@@ -3595,6 +3596,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     flush_compressed_data(rs);
     ram_control_after_iterate(f, RAM_CONTROL_FINISH);
 
+    if (kvm_memcrypt_enabled()) {
+        ret = kvm_memcrypt_save_outgoing_page_enc_bitmap(f);
+    }
+
     rcu_read_unlock();
 
     multifd_send_sync_main();
@@ -4469,6 +4474,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
                     ret = -EINVAL;
             }
             break;
+        case RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP:
+            if (kvm_memcrypt_load_incoming_page_enc_bitmap(f)) {
+                error_report("Failed to load page enc bitmap");
+                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 09a62d6f88..93c6a90806 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -63,6 +63,7 @@ static const char *const sev_fw_errlist[] = {
 };
 
 #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
+#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400
 
 static int
 sev_ioctl(int fd, int cmd, void *data, int *error)
@@ -1189,6 +1190,72 @@ int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
     return sev_receive_update_data(f, ptr);
 }
 
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
+
+int sev_load_incoming_page_enc_bitmap(void *handle, QEMUFile *f)
+{
+    void *bmap;
+    unsigned long npages;
+    unsigned long bmap_size, base_gpa;
+    struct kvm_page_enc_bitmap e = {};
+
+    base_gpa = qemu_get_be64(f);
+    npages = qemu_get_be64(f);
+    bmap_size = qemu_get_be64(f);
+
+    bmap = g_malloc0(bmap_size);
+    qemu_get_buffer(f, (uint8_t *)bmap, bmap_size);
+
+    trace_kvm_sev_load_page_enc_bitmap(base_gpa, npages << TARGET_PAGE_BITS);
+
+    e.start_gfn = base_gpa >> TARGET_PAGE_BITS;
+    e.num_pages = npages;
+    e.enc_bitmap = bmap;
+    if (kvm_vm_ioctl(kvm_state, KVM_SET_PAGE_ENC_BITMAP, &e) == -1) {
+        error_report("KVM_SET_PAGE_ENC_BITMAP ioctl failed %d", errno);
+        g_free(bmap);
+        return 1;
+    }
+
+    g_free(bmap);
+
+    return 0;
+}
+
+int sev_save_outgoing_page_enc_bitmap(void *handle, QEMUFile *f,
+                                      unsigned long start, uint64_t length)
+{
+    uint64_t size;
+    struct kvm_page_enc_bitmap e = {};
+
+    if (!length) {
+        return 0;
+    }
+
+    size = ALIGN((length >> TARGET_PAGE_BITS), /*HOST_LONG_BITS*/ 64) / 8;
+    e.enc_bitmap = g_malloc0(size);
+    e.start_gfn = start >> TARGET_PAGE_BITS;
+    e.num_pages = length >> TARGET_PAGE_BITS;
+
+    trace_kvm_sev_save_page_enc_bitmap(start, length);
+
+    if (kvm_vm_ioctl(kvm_state, KVM_GET_PAGE_ENC_BITMAP, &e) == -1) {
+        error_report("%s: KVM_GET_PAGE_ENC_BITMAP ioctl failed %d",
+                    __func__, errno);
+        g_free(e.enc_bitmap);
+        return 1;
+    }
+
+    qemu_put_be64(f, RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP);
+    qemu_put_be64(f, start);
+    qemu_put_be64(f, e.num_pages);
+    qemu_put_be64(f, size);
+    qemu_put_buffer(f, (uint8_t *)e.enc_bitmap, size);
+
+    g_free(e.enc_bitmap);
+    return 0;
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 609752cca7..4c2be570f9 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -21,3 +21,5 @@ 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) ""
+kvm_sev_save_page_enc_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 0x%" PRIx64
+kvm_sev_load_page_enc_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 0x%" PRIx64
-- 
2.17.1

[Qemu-devel] [PATCH v2 13/13] target/i386: sev: remove migration blocker

[Singh, Brijesh]

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 93c6a90806..48336515a2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -34,7 +34,6 @@
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
 
 static SEVState *sev_state;
-static Error *sev_mig_blocker;
 
 static const char *const sev_fw_errlist[] = {
     "",
@@ -686,7 +685,6 @@ static void
 sev_launch_finish(SEVState *s)
 {
     int ret, error;
-    Error *local_err = NULL;
 
     trace_kvm_sev_launch_finish();
     ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
@@ -697,16 +695,6 @@ sev_launch_finish(SEVState *s)
     }
 
     sev_set_guest_state(SEV_STATE_RUNNING);
-
-    /* add migration blocker */
-    error_setg(&sev_mig_blocker,
-               "SEV: Migration is not implemented");
-    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
-    if (local_err) {
-        error_report_err(local_err);
-        error_free(sev_mig_blocker);
-        exit(1);
-    }
 }
 
 static int
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190710202219.25939-1-brijesh.singh@amd.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support
Type: series
Message-id: 20190710202219.25939-1-brijesh.singh@amd.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
9eb9639 target/i386: sev: remove migration blocker
00c1826 migration: add support to migrate page encryption bitmap
cc5d459 kvm: introduce high-level API to migrate the page encryption bitmap
744b933 target/i386: sev: add support to load incoming encrypted page
0365253 target/i386: sev: add support to encrypt the outgoing page
b616e11 misc.json: add migrate-set-sev-info command
6dbc25d target/i386: sev: do not create launch context for an incoming guest
467cdc0 doc: update AMD SEV to include Live migration flow
a02bd6f doc: update AMD SEV API spec web link
26fee5c kvm: add support to sync the page encryption state bitmap
ec80b9c migration/ram: add support to send encrypted pages
f09bf4e kvm: introduce high-level API to support encrypted page migration
9b1e5ae linux-headers: update kernel header to include SEV migration commands

=== OUTPUT BEGIN ===
1/13 Checking commit 9b1e5aef53f4 (linux-headers: update kernel header to include SEV migration commands)
2/13 Checking commit f09bf4e4d9e0 (kvm: introduce high-level API to support encrypted page migration)
WARNING: line over 80 characters
#45: FILE: accel/kvm/kvm-all.c:177:
+        return kvm_state->memcrypt_save_outgoing_page(kvm_state->memcrypt_handle,

WARNING: line over 80 characters
#56: FILE: accel/kvm/kvm-all.c:188:
+        return kvm_state->memcrypt_load_incoming_page(kvm_state->memcrypt_handle,

total: 0 errors, 2 warnings, 96 lines checked

Patch 2/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/13 Checking commit ec80b9c1d59e (migration/ram: add support to send encrypted pages)
4/13 Checking commit 26fee5cf1cef (kvm: add support to sync the page encryption state bitmap)
WARNING: Block comments use a leading /* on a separate line
#40: FILE: accel/kvm/kvm-all.c:520:
+                 /*HOST_LONG_BITS*/ 64) / 8;

WARNING: line over 80 characters
#110: FILE: include/exec/ram_addr.h:336:
+    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;

WARNING: line over 80 characters
#124: FILE: include/exec/ram_addr.h:350:
+static inline void cpu_physical_memory_set_dirty_enc_lebitmap(unsigned long *bitmap,

WARNING: line over 80 characters
#141: FILE: include/exec/ram_addr.h:387:
+                    atomic_xchg(&blocks[DIRTY_MEMORY_ENCRYPTED][idx][offset], temp);

WARNING: line over 80 characters
#143: FILE: include/exec/ram_addr.h:389:
+                    atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);

WARNING: line over 80 characters
#146: FILE: include/exec/ram_addr.h:392:
+                        atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);

WARNING: Block comments use a leading /* on a separate line
#156: FILE: include/exec/ram_addr.h:414:
+            /* If its encrypted bitmap update, then we need to copy the bitmap

WARNING: line over 80 characters
#160: FILE: include/exec/ram_addr.h:418:
+                cpu_physical_memory_set_encrypted_range(start + i * TARGET_PAGE_SIZE,

WARNING: line over 80 characters
#161: FILE: include/exec/ram_addr.h:419:
+                                                        TARGET_PAGE_SIZE * hpratio,

WARNING: line over 80 characters
#174: FILE: include/exec/ram_addr.h:440:
+static inline void cpu_physical_memory_set_encrypted_lebitmap(unsigned long *bitmap,

WARNING: line over 80 characters
#178: FILE: include/exec/ram_addr.h:444:
+    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, true);

WARNING: line over 80 characters
#185: FILE: include/exec/ram_addr.h:451:
+    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, false);

WARNING: line over 80 characters
#195: FILE: include/exec/ram_addr.h:473:
+    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_ENCRYPTED);

WARNING: line over 80 characters
#220: FILE: include/exec/ram_addr.h:559:
+    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;

total: 0 errors, 14 warnings, 320 lines checked

Patch 4/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/13 Checking commit a02bd6ff047c (doc: update AMD SEV API spec web link)
6/13 Checking commit 467cdc0f381c (doc: update AMD SEV to include Live migration flow)
7/13 Checking commit 6dbc25da4473 (target/i386: sev: do not create launch context for an incoming guest)
8/13 Checking commit b616e11ebffa (misc.json: add migrate-set-sev-info command)
9/13 Checking commit 0365253b8689 (target/i386: sev: add support to encrypt the outgoing page)
WARNING: line over 80 characters
#199: FILE: target/i386/sev.c:978:
+    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_UPDATE_DATA, update, fw_err);

WARNING: Block comments use a leading /* on a separate line
#222: FILE: target/i386/sev.c:1001:
+    /* If this is first call then query the packet header bytes and allocate

total: 0 errors, 2 warnings, 274 lines checked

Patch 9/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/13 Checking commit 744b933d900c (target/i386: sev: add support to load incoming encrypted page)
WARNING: Block comments use a leading /* on a separate line
#167: FILE: target/i386/sev.c:1181:
+    /* If this is first buffer and SEV is not in recieiving state then

total: 0 errors, 1 warnings, 157 lines checked

Patch 10/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/13 Checking commit cc5d45952404 (kvm: introduce high-level API to migrate the page encryption bitmap)
12/13 Checking commit 00c1826c9c19 (migration: add support to migrate page encryption bitmap)
WARNING: line over 80 characters
#40: FILE: migration/ram.c:81:
+#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400 /* used in target/i386/sev.c */

ERROR: spaces required around that '+' (ctx:VxV)
#84: FILE: target/i386/sev.c:1193:
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
                           ^

ERROR: spaces required around that '-' (ctx:VxV)
#84: FILE: target/i386/sev.c:1193:
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
                               ^

ERROR: spaces required around that '-' (ctx:VxV)
#84: FILE: target/i386/sev.c:1193:
+#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
                                          ^

WARNING: Block comments use a leading /* on a separate line
#126: FILE: target/i386/sev.c:1235:
+    size = ALIGN((length >> TARGET_PAGE_BITS), /*HOST_LONG_BITS*/ 64) / 8;

total: 3 errors, 2 warnings, 123 lines checked

Patch 12/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

13/13 Checking commit 9eb9639297de (target/i386: sev: remove migration blocker)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190710202219.25939-1-brijesh.singh@amd.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190710202219.25939-1-brijesh.singh@amd.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

  CC      alpha-softmmu/hw/scsi/virtio-scsi-dataplane.o
/usr/bin/ld: qapi/qapi-commands-misc-target.o: in function `qmp_marshal_migrate_set_sev_info':
/var/tmp/patchew-tester-tmp-iinjo1bi/src/build/qapi/qapi-commands-misc-target.c:363: undefined reference to `qmp_migrate_set_sev_info'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:208: qemu-system-cris] Error 1
make: *** [Makefile:472: cris-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
---
  CC      aarch64-softmmu/hw/misc/bcm2835_mbox.o
/usr/bin/ld: qapi/qapi-commands-misc-target.o: in function `qmp_marshal_migrate_set_sev_info':
/var/tmp/patchew-tester-tmp-iinjo1bi/src/build/qapi/qapi-commands-misc-target.c:363: undefined reference to `qmp_migrate_set_sev_info'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:208: qemu-system-alpha] Error 1
make: *** [Makefile:472: alpha-softmmu/all] Error 2
  CC      aarch64-softmmu/hw/misc/bcm2835_property.o
---
  CC      arm-softmmu/target/arm/debug_helper.o
/usr/bin/ld: qapi/qapi-commands-misc-target.o: in function `qmp_marshal_migrate_set_sev_info':
/var/tmp/patchew-tester-tmp-iinjo1bi/src/build/qapi/qapi-commands-misc-target.c:363: undefined reference to `qmp_migrate_set_sev_info'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:208: qemu-system-aarch64] Error 1
make: *** [Makefile:472: aarch64-softmmu/all] Error 2
  GEN     arm-softmmu/target/arm/decode-vfp.inc.c
---
  LINK    arm-softmmu/qemu-system-arm
/usr/bin/ld: qapi/qapi-commands-misc-target.o: in function `qmp_marshal_migrate_set_sev_info':
/var/tmp/patchew-tester-tmp-iinjo1bi/src/build/qapi/qapi-commands-misc-target.c:363: undefined reference to `qmp_migrate_set_sev_info'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:208: qemu-system-arm] Error 1
make: *** [Makefile:472: arm-softmmu/all] Error 2
=== OUTPUT END ===


The full log is available at
http://patchew.org/logs/20190710202219.25939-1-brijesh.singh@amd.com/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 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.

Note the two patchew errors:
  a) Mostly formatting; 80 char lines, /* comments etc - you should
     check your patches using scripts/checkpatch.pl  to get rid of that
     lot.

  b) There are some build errors on non-x86 softmmu builds.

Dave

> The patch series depends on kernel patches available here:
> https://marc.info/?l=kvm&m=156278967226011&w=2
> 
> The complete tree with patch is available at:
> https://github.com/codomania/qemu/tree/sev-migration-v2
> 
> Changes since v1:
>  - use the dirty log sync APIs to also sync the page encryption bitmap
>    when SEV is active.
> 
> Brijesh Singh (13):
>   linux-headers: update kernel header to include SEV migration commands
>   kvm: introduce high-level API to support encrypted page migration
>   migration/ram: add support to send encrypted pages
>   kvm: add support to sync the page encryption state bitmap
>   doc: update AMD SEV API spec web link
>   doc: update AMD SEV to include Live migration flow
>   target/i386: sev: do not create launch context for an incoming guest
>   misc.json: add migrate-set-sev-info command
>   target/i386: sev: add support to encrypt the outgoing page
>   target/i386: sev: add support to load incoming encrypted page
>   kvm: introduce high-level API to migrate the page encryption bitmap
>   migration: add support to migrate page encryption bitmap
>   target/i386: sev: remove migration blocker
> 
>  accel/kvm/kvm-all.c            | 108 ++++++++
>  accel/kvm/sev-stub.c           |  22 ++
>  accel/stubs/kvm-stub.c         |  22 ++
>  docs/amd-memory-encryption.txt |  44 +++-
>  include/exec/ram_addr.h        | 161 +++++++++++-
>  include/exec/ramlist.h         |   3 +-
>  include/sysemu/kvm.h           |  25 ++
>  include/sysemu/sev.h           |   6 +
>  linux-headers/linux/kvm.h      |  53 ++++
>  migration/ram.c                |  91 ++++++-
>  qapi/misc-target.json          |  18 ++
>  target/i386/monitor.c          |  10 +
>  target/i386/sev-stub.c         |   5 +
>  target/i386/sev.c              | 455 +++++++++++++++++++++++++++++++--
>  target/i386/sev_i386.h         |  11 +-
>  target/i386/trace-events       |   8 +
>  16 files changed, 1016 insertions(+), 26 deletions(-)
> 
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 03/13] migration/ram: add support to send encrypted pages

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 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 kvm_memcrypt_save_outgoing_page() is used
> by the sender to write the encrypted pages onto the socket, similarly the
> kvm_memcrypt_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>
> ---
>  migration/ram.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 908517fc2b..3c8977d508 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -57,6 +57,7 @@
>  #include "qemu/uuid.h"
>  #include "savevm.h"
>  #include "qemu/iov.h"
> +#include "sysemu/kvm.h"
>  
>  /***********************************************************/
>  /* ram save/restore */
> @@ -76,6 +77,7 @@
>  #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_PAGE   0x200

OK, that's our very last usable flag!  Use it wisely!

>  static inline bool is_zero_range(uint8_t *p, uint64_t size)
>  {
> @@ -460,6 +462,9 @@ 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)
>  {
>      CompressParam *param = opaque;
> @@ -2006,6 +2011,36 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
>      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;
> +
> +    p = block->host + offset;
> +
> +    ram_counters.transferred +=
> +        save_page_header(rs, rs->f, block,
> +                    offset | RAM_SAVE_FLAG_ENCRYPTED_PAGE);
> +
> +    ret = kvm_memcrypt_save_outgoing_page(rs->f, p,

I think you need to somehow abstract the kvm_memcrypt stuff; nothing
else in migration actually knows it's dealing with kvm.  So there
should be some indirection - probably through the cpu or the machine
type or something.

Also, this isn't bisectable - you can't make this call in this patch
because you don't define/declare this function until a later patch.


> +                        TARGET_PAGE_SIZE, &bytes_xmit);
> +    if (ret) {
> +        return -1;
> +    }
> +
> +    ram_counters.transferred += bytes_xmit;
> +    ram_counters.normal++;
> +
> +    return 1;
> +}
> +
>  static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>                                   ram_addr_t offset, uint8_t *source_buf)
>  {
> @@ -2450,6 +2485,16 @@ 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 (kvm_memcrypt_enabled()) {
> +        return ram_save_encrypted_page(rs, pss, last_stage);
> +     }
> +
>      if (save_compress_page(rs, block, offset)) {
>          return 1;
>      }
> @@ -4271,7 +4316,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          }
>  
>          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_PAGE)) {
>              RAMBlock *block = ram_block_from_stream(f, flags);
>  
>              /*
> @@ -4391,6 +4437,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                  break;
>              }
>              break;
> +        case RAM_SAVE_FLAG_ENCRYPTED_PAGE:
> +            if (kvm_memcrypt_load_incoming_page(f, host)) {
> +                    error_report("Failed to encrypted incoming data");

'Failed to *load* encrypted page' ?

> +                    ret = -EINVAL;

Do you want to actually return an error code here from
kvm_memcrypt_load_incoming_page, so we can keep hold of whether
it was something like a simple network error for the file stream
or something more complex.

Dave

> +            }
> +            break;
>          case RAM_SAVE_FLAG_EOS:
>              /* normal exit */
>              multifd_recv_sync_main();
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 02/13] kvm: introduce high-level API to support encrypted page migration

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> When memory encryption is enabled in VM, the guest pages will be
> encrypted with the guest-specific key, to protect the confidentiality
> of data in transit. To support the live migration we need to use
> platform specific hooks to access the guest memory.
> 
> The kvm_memcrypt_save_outgoing_page() can be used by the sender to write
> the encrypted pages and metadata associated with it on the socket.
> 
> The kvm_memcrypt_load_incoming_page() can be used by receiver to read the
> incoming encrypted pages from the socket and load into the guest memory.
> 
> Signed-off-by: Brijesh Singh <<brijesh.singh@amd.com>>
> ---
>  accel/kvm/kvm-all.c    | 27 +++++++++++++++++++++++++++
>  accel/kvm/sev-stub.c   | 11 +++++++++++
>  accel/stubs/kvm-stub.c | 12 ++++++++++++
>  include/sysemu/kvm.h   | 12 ++++++++++++
>  include/sysemu/sev.h   |  3 +++
>  5 files changed, 65 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 3d86ae5052..162a2d5085 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -110,6 +110,10 @@ struct KVMState
>      /* memory encryption */
>      void *memcrypt_handle;
>      int (*memcrypt_encrypt_data)(void *handle, uint8_t *ptr, uint64_t len);
> +    int (*memcrypt_save_outgoing_page)(void *ehandle, QEMUFile *f,
> +            uint8_t *ptr, uint32_t sz, uint64_t *bytes_sent);
> +    int (*memcrypt_load_incoming_page)(void *ehandle, QEMUFile *f,
> +            uint8_t *ptr);
>  };
>  
>  KVMState *kvm_state;
> @@ -165,6 +169,29 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
>      return 1;
>  }
>  
> +int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
> +                                    uint32_t size, uint64_t *bytes_sent)
> +{
> +    if (kvm_state->memcrypt_handle &&
> +        kvm_state->memcrypt_save_outgoing_page) {
> +        return kvm_state->memcrypt_save_outgoing_page(kvm_state->memcrypt_handle,
> +                    f, ptr, size, bytes_sent);
> +    }
> +
> +    return 1;

This needs to be commented saying what the return values mean.
I'm not sure what '1' means for the case when this didn't have
encryption support.

> +}
> +
> +int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
> +{
> +    if (kvm_state->memcrypt_handle &&
> +        kvm_state->memcrypt_load_incoming_page) {
> +        return kvm_state->memcrypt_load_incoming_page(kvm_state->memcrypt_handle,
> +                    f, ptr);
> +    }
> +
> +    return 1;
> +}
> +
>  static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
>  {
>      KVMState *s = kvm_state;
> diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
> index 4f97452585..c12a8e005e 100644
> --- a/accel/kvm/sev-stub.c
> +++ b/accel/kvm/sev-stub.c
> @@ -24,3 +24,14 @@ void *sev_guest_init(const char *id)
>  {
>      return NULL;
>  }
> +
> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
> +                           uint32_t size, uint64_t *bytes_sent)
> +{
> +    return 1;
> +}
> +
> +int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
> +{
> +    return 1;
> +}
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 6feb66ed80..e14b879531 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -114,6 +114,18 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
>    return 1;
>  }
>  
> +int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
> +                                    uint32_t size, uint64_t *bytes_sent)
> +{
> +    return 1;
> +}
> +
> +int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
> +{
> +    return 1;
> +}
> +
> +
>  #ifndef CONFIG_USER_ONLY
>  int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>  {
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index acd90aebb6..bb6bcc143c 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -247,6 +247,18 @@ bool kvm_memcrypt_enabled(void);
>   */
>  int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>  
> +/**
> + * kvm_memcrypt_save_outgoing_buffer - encrypt the outgoing buffer
> + * and write to the wire.
> + */
> +int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr, uint32_t size,
> +                                    uint64_t *bytes_sent);
> +
> +/**
> + * kvm_memcrypt_load_incoming_buffer - read the encrypt incoming buffer and copy
> + * the buffer into the guest memory space.
> + */
> +int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr);
>  
>  #ifdef NEED_CPU_H
>  #include "cpu.h"
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index 98c1ec8d38..752a71b1c0 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -18,4 +18,7 @@
>  
>  void *sev_guest_init(const char *id);
>  int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
> +                           uint32_t size, uint64_t *bytes_sent);
> +int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr);
>  #endif
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 05/13] doc: update AMD SEV API spec web link

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/amd-memory-encryption.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index 43bf3ee6a5..abb9a976f5 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -98,7 +98,7 @@ AMD Memory Encryption whitepaper:
>  http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf
>  
>  Secure Encrypted Virtualization Key Management:
> -[1] http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
> +[1] https://developer.amd.com/sev/ (Secure Encrypted Virtualization API)

No; that reference [1] is used a few lines hire up for:

See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
complete flow chart.


so that needs fixing up to actually point to that flowchart or
equivalent.

That site is useful to include, but I guess it also needs a pointer
to the Volume2 section 15.34 or the like.

Dave


>  KVM Forum slides:
>  http://www.linux-kvm.org/images/7/74/02x08A-Thomas_Lendacky-AMDs_Virtualizatoin_Memory_Encryption_Technology.pdf
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 04/13] kvm: add support to sync the page encryption state bitmap

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> The SEV VMs have concept of private and shared memory. The private memory
> is encrypted with guest-specific key, while shared memory may be encrypted
> with hyperivosr key. The KVM_GET_PAGE_ENC_BITMAP can be used to get a
> bitmap indicating whether the guest page is private or shared. A private
> page must be transmitted using the SEV migration commands.
> 
> Add a cpu_physical_memory_sync_encrypted_bitmap() which can be used to sync
> the page encryption bitmap for a given memory region.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  accel/kvm/kvm-all.c     |  38 ++++++++++
>  include/exec/ram_addr.h | 161 ++++++++++++++++++++++++++++++++++++++--
>  include/exec/ramlist.h  |   3 +-
>  migration/ram.c         |  28 ++++++-
>  4 files changed, 222 insertions(+), 8 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 162a2d5085..c935e9366c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -504,6 +504,37 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>  
>  #define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
>  
> +/* sync page_enc bitmap */
> +static int kvm_sync_page_enc_bitmap(KVMMemoryListener *kml,
> +                                    MemoryRegionSection *section,
> +                                    KVMSlot *mem)

How AMD/SEV specific is this? i.e. should this be in a target/ specific
place? 

> +{
> +    unsigned long size;
> +    KVMState *s = kvm_state;
> +    struct kvm_page_enc_bitmap e = {};
> +    ram_addr_t pages = int128_get64(section->size) / getpagesize();
> +    ram_addr_t start = section->offset_within_region +
> +                       memory_region_get_ram_addr(section->mr);
> +
> +    size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
> +                 /*HOST_LONG_BITS*/ 64) / 8;
> +    e.enc_bitmap = g_malloc0(size);
> +    e.start_gfn = mem->start_addr >> TARGET_PAGE_BITS;
> +    e.num_pages = pages;
> +    if (kvm_vm_ioctl(s, KVM_GET_PAGE_ENC_BITMAP, &e) == -1) {
> +        DPRINTF("KVM_GET_PAGE_ENC_BITMAP ioctl failed %d\n", errno);
> +        g_free(e.enc_bitmap);
> +        return 1;
> +    }
> +
> +    cpu_physical_memory_set_encrypted_lebitmap(e.enc_bitmap,
> +                                               start, pages);
> +
> +    g_free(e.enc_bitmap);
> +
> +    return 0;
> +}
> +
>  /**
>   * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
>   * This function updates qemu's dirty bitmap using
> @@ -553,6 +584,13 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
>          }
>  
>          kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
> +
> +        if (kvm_memcrypt_enabled() &&
> +            kvm_sync_page_enc_bitmap(kml, section, mem)) {
> +            g_free(d.dirty_bitmap);
> +            return -1;
> +        }
> +
>          g_free(d.dirty_bitmap);
>      }
>  
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index f96777bb99..6fc6864194 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -51,6 +51,8 @@ struct RAMBlock {
>      unsigned long *unsentmap;
>      /* bitmap of already received pages in postcopy */
>      unsigned long *receivedmap;
> +    /* bitmap of page encryption state for an encrypted guest */
> +    unsigned long *encbmap;
>  };
>  
>  static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> @@ -314,9 +316,41 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>  }
>  
>  #if !defined(_WIN32)
> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> +
> +static inline void cpu_physical_memory_set_encrypted_range(ram_addr_t start,
> +                                                           ram_addr_t length,
> +                                                           unsigned long val)
> +{
> +    unsigned long end, page;
> +    unsigned long * const *src;
> +
> +    if (length == 0) {
> +        return;
> +    }
> +
> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +    page = start >> TARGET_PAGE_BITS;
> +
> +    rcu_read_lock();
> +
> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
> +
> +    while (page < end) {
> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
> +
> +        atomic_xchg(&src[idx][BIT_WORD(offset)], val);
> +        page += num;
> +    }
> +
> +    rcu_read_unlock();
> +}
> +
> +static inline void cpu_physical_memory_set_dirty_enc_lebitmap(unsigned long *bitmap,
>                                                            ram_addr_t start,
> -                                                          ram_addr_t pages)
> +                                                          ram_addr_t pages,
> +                                                          bool enc_map)
>  {
>      unsigned long i, j;
>      unsigned long page_number, c;
> @@ -349,10 +383,14 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>              if (bitmap[k]) {
>                  unsigned long temp = leul_to_cpu(bitmap[k]);
>  
> -                atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
> -                atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
> -                if (tcg_enabled()) {
> -                    atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
> +                if (enc_map) {
> +                    atomic_xchg(&blocks[DIRTY_MEMORY_ENCRYPTED][idx][offset], temp);

It makes me nervous that this is almost but not exactly like the other
bitmaps;  I *think* you're saying the bits here are purely a matter of
state about whether the page is encrypted and not a matter of actually
dirtyness; in particular a page that is encrypted and then becomes dirty
doesn't reset or clear this flag.

> +                } else {
> +                    atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
> +                    atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
> +                    if (tcg_enabled()) {
> +                        atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
> +                    }
>                  }
>              }
>  
> @@ -372,6 +410,17 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>           * especially when most of the memory is not dirty.
>           */
>          for (i = 0; i < len; i++) {
> +
> +            /* If its encrypted bitmap update, then we need to copy the bitmap
> +             * value as-is to the destination.
> +             */
> +            if (enc_map) {
> +                cpu_physical_memory_set_encrypted_range(start + i * TARGET_PAGE_SIZE,
> +                                                        TARGET_PAGE_SIZE * hpratio,
> +                                                        leul_to_cpu(bitmap[i]));
> +                continue;
> +            }
> +
>              if (bitmap[i] != 0) {
>                  c = leul_to_cpu(bitmap[i]);
>                  do {
> @@ -387,6 +436,21 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>          }
>      }
>  }
> +
> +static inline void cpu_physical_memory_set_encrypted_lebitmap(unsigned long *bitmap,
> +                                                              ram_addr_t start,
> +                                                              ram_addr_t pages)
> +{
> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, true);
> +}
> +
> +static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> +                                                          ram_addr_t start,
> +                                                          ram_addr_t pages)
> +{
> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, false);
> +}
> +
>  #endif /* not _WIN32 */
>  
>  bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
> @@ -406,6 +470,7 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
>      cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
>      cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
>      cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_ENCRYPTED);

What are the ordering/consistency rules associated with this data.
Specifically:

  Consider a page that transitions from being shared to encrypted
(does that happen?) - but we've just done the sync's so we know the page
is dirty, but we don't know it's encrypted; so we transmit the page as
unencrypted; what happens?

  I *think* that means we should always sync the encryped bitmap before
the dirty bitmap, so that if it flips we're guaranteed the dirty bitmap
gets flipped again after the flip has happened; but my brain is starting
to hurt....

  But, even if we're guaranteed to have a dirty for the next time
around, I think we're always at risk of transmitting a page that
has just flipped; so we'll be sure to transmit it again correctly,
but we might transmit an encrypted page to a non-encrypted dest or
the reverse - is that OK?


>  }
>  
>  
> @@ -474,5 +539,89 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
>  
>      return num_dirty;
>  }
> +
> +static inline bool cpu_physical_memory_test_encrypted(ram_addr_t start,
> +                                                      ram_addr_t length)
> +{
> +    unsigned long end, page;
> +    bool enc = false;
> +    unsigned long * const *src;
> +
> +    if (length == 0) {
> +        return enc;
> +    }
> +
> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +    page = start >> TARGET_PAGE_BITS;
> +
> +    rcu_read_lock();
> +
> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
> +
> +    while (page < end) {
> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
> +
> +        enc |= atomic_read(&src[idx][BIT_WORD(offset)]);

I'm confused; I thought what I was about to get there was a long* and
you were going to mask out a bit or set of bits.

> +        page += num;
> +    }
> +
> +    rcu_read_unlock();
> +
> +    return enc;
> +}
> +
> +static inline
> +void cpu_physical_memory_sync_encrypted_bitmap(RAMBlock *rb,
> +                                               ram_addr_t start,
> +                                               ram_addr_t length)
> +{
> +    ram_addr_t addr;
> +    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
> +    unsigned long *dest = rb->encbmap;
> +
> +    /* start address and length is aligned at the start of a word? */
> +    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
> +         (start + rb->offset) &&
> +        !(length & ((BITS_PER_LONG << TARGET_PAGE_BITS) - 1))) {
> +        int k;
> +        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);

Feels odd it's an int.

> +        unsigned long * const *src;
> +        unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
> +        unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
> +                                        DIRTY_MEMORY_BLOCK_SIZE);
> +        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> +
> +        rcu_read_lock();
> +
> +        src = atomic_rcu_read(
> +                &ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
> +
> +        for (k = page; k < page + nr; k++) {
> +            unsigned long bits = atomic_read(&src[idx][offset]);
> +            dest[k] = bits;
> +
> +            if (++offset >= BITS_TO_LONGS(DIRTY_MEMORY_BLOCK_SIZE)) {
> +                offset = 0;
> +                idx++;
> +            }
> +        }
> +
> +        rcu_read_unlock();
> +    } else {
> +        ram_addr_t offset = rb->offset;
> +
> +        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> +            long k = (start + addr) >> TARGET_PAGE_BITS;
> +            if (cpu_physical_memory_test_encrypted(start + addr + offset,
> +                                                   TARGET_PAGE_SIZE)) {
> +                set_bit(k, dest);
> +            } else {
> +                clear_bit(k, dest);
> +            }
> +        }
> +    }
> +}
>  #endif
>  #endif
> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> index bc4faa1b00..2a5eab8b11 100644
> --- a/include/exec/ramlist.h
> +++ b/include/exec/ramlist.h
> @@ -11,7 +11,8 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
>  #define DIRTY_MEMORY_VGA       0
>  #define DIRTY_MEMORY_CODE      1
>  #define DIRTY_MEMORY_MIGRATION 2
> -#define DIRTY_MEMORY_NUM       3        /* num of dirty bits */
> +#define DIRTY_MEMORY_ENCRYPTED 3
> +#define DIRTY_MEMORY_NUM       4        /* num of dirty bits */
>  
>  /* The dirty memory bitmap is split into fixed-size blocks to allow growth
>   * under RCU.  The bitmap for a block can be accessed as follows:
> diff --git a/migration/ram.c b/migration/ram.c
> index 3c8977d508..d179867e1b 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1680,6 +1680,9 @@ static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
>      rs->migration_dirty_pages +=
>          cpu_physical_memory_sync_dirty_bitmap(rb, 0, length,
>                                                &rs->num_dirty_pages_period);
> +    if (kvm_memcrypt_enabled()) {
> +        cpu_physical_memory_sync_encrypted_bitmap(rb, 0, length);
> +    }
>  }
>  
>  /**
> @@ -2465,6 +2468,22 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
>      return false;
>  }
>  
> +/**
> + * encrypted_test_bitmap: check if the page is encrypted
> + *
> + * Returns a bool indicating whether the page is encrypted.
> + */
> +static bool encrypted_test_bitmap(RAMState *rs, RAMBlock *block,
> +                                  unsigned long page)
> +{
> +    /* ROM devices contains the unencrypted data */
> +    if (memory_region_is_rom(block->mr)) {
> +        return false;
> +    }
> +
> +    return test_bit(page, block->encbmap);
> +}
> +
>  /**
>   * ram_save_target_page: save one target page
>   *
> @@ -2491,7 +2510,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>       * will take care of accessing the guest memory and re-encrypt it
>       * for the transport purposes.
>       */
> -     if (kvm_memcrypt_enabled()) {
> +     if (kvm_memcrypt_enabled() &&
> +         encrypted_test_bitmap(rs, pss->block, pss->page)) {
>          return ram_save_encrypted_page(rs, pss, last_stage);
>       }
>  
> @@ -2724,6 +2744,8 @@ static void ram_save_cleanup(void *opaque)
>          block->bmap = NULL;
>          g_free(block->unsentmap);
>          block->unsentmap = NULL;
> +        g_free(block->encbmap);
> +        block->encbmap = NULL;
>      }
>  
>      xbzrle_cleanup();
> @@ -3251,6 +3273,10 @@ static void ram_list_init_bitmaps(void)
>                  block->unsentmap = bitmap_new(pages);
>                  bitmap_set(block->unsentmap, 0, pages);
>              }
> +            if (kvm_memcrypt_enabled()) {
> +                block->encbmap = bitmap_new(pages);
> +                bitmap_set(block->encbmap, 0, pages);
> +            }
>          }
>      }
>  }
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 03/13] migration/ram: add support to send encrypted pages

[Singh, Brijesh]

On 7/11/19 12:34 PM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> 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 kvm_memcrypt_save_outgoing_page() is used
>> by the sender to write the encrypted pages onto the socket, similarly the
>> kvm_memcrypt_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>
>> ---
>>   migration/ram.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 908517fc2b..3c8977d508 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -57,6 +57,7 @@
>>   #include "qemu/uuid.h"
>>   #include "savevm.h"
>>   #include "qemu/iov.h"
>> +#include "sysemu/kvm.h"
>>   
>>   /***********************************************************/
>>   /* ram save/restore */
>> @@ -76,6 +77,7 @@
>>   #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_PAGE   0x200
> 
> OK, that's our very last usable flag!  Use it wisely!
> 

Hmm, maybe then I missed something. I thought the flag is 64-bit and
we have more room. Did I miss something ?


>>   static inline bool is_zero_range(uint8_t *p, uint64_t size)
>>   {
>> @@ -460,6 +462,9 @@ 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)
>>   {
>>       CompressParam *param = opaque;
>> @@ -2006,6 +2011,36 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
>>       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;
>> +
>> +    p = block->host + offset;
>> +
>> +    ram_counters.transferred +=
>> +        save_page_header(rs, rs->f, block,
>> +                    offset | RAM_SAVE_FLAG_ENCRYPTED_PAGE);
>> +
>> +    ret = kvm_memcrypt_save_outgoing_page(rs->f, p,
> 
> I think you need to somehow abstract the kvm_memcrypt stuff; nothing
> else in migration actually knows it's dealing with kvm.  So there
> should be some indirection - probably through the cpu or the machine
> type or something.
> 

Currently, there are two interfaces by which we can know if we
are dealing with encrypted guest. kvm_memcrypt_enabled() or
MachineState->memory_encryption pointer. I did realized that
migration code have not dealt with kvm so far.

How about target/i386/sev.c exporting the migration functions and
based on state of MachineState->memory_encryption we call the
SEV migration routines for the encrypted pages?


> Also, this isn't bisectable - you can't make this call in this patch
> because you don't define/declare this function until a later patch.
> 
> 
>> +                        TARGET_PAGE_SIZE, &bytes_xmit);
>> +    if (ret) {
>> +        return -1;
>> +    }
>> +
>> +    ram_counters.transferred += bytes_xmit;
>> +    ram_counters.normal++;
>> +
>> +    return 1;
>> +}
>> +
>>   static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>>                                    ram_addr_t offset, uint8_t *source_buf)
>>   {
>> @@ -2450,6 +2485,16 @@ 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 (kvm_memcrypt_enabled()) {
>> +        return ram_save_encrypted_page(rs, pss, last_stage);
>> +     }
>> +
>>       if (save_compress_page(rs, block, offset)) {
>>           return 1;
>>       }
>> @@ -4271,7 +4316,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>           }
>>   
>>           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_PAGE)) {
>>               RAMBlock *block = ram_block_from_stream(f, flags);
>>   
>>               /*
>> @@ -4391,6 +4437,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>                   break;
>>               }
>>               break;
>> +        case RAM_SAVE_FLAG_ENCRYPTED_PAGE:
>> +            if (kvm_memcrypt_load_incoming_page(f, host)) {
>> +                    error_report("Failed to encrypted incoming data");
> 
> 'Failed to *load* encrypted page' ?

Ah, thanks. It should be *load.

> 
>> +                    ret = -EINVAL;
> 
> Do you want to actually return an error code here from
> kvm_memcrypt_load_incoming_page, so we can keep hold of whether
> it was something like a simple network error for the file stream
> or something more complex.
> 

Currently, the kvm_memcrypt_load_incoming_pages() return 0 or 1.
0 for the success and 1 for the failure. If we enhance the function to
propagate the error code then some of them will not make sense for the
migration code. Mainly those around the SEV FW failure etc.


> Dave
> 
>> +            }
>> +            break;
>>           case RAM_SAVE_FLAG_EOS:
>>               /* normal exit */
>>               multifd_recv_sync_main();
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v2 00/13] Add SEV guest live migration support

[Singh, Brijesh]

On 7/11/19 4:59 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> 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.
> 
> Note the two patchew errors:
>    a) Mostly formatting; 80 char lines, /* comments etc - you should
>       check your patches using scripts/checkpatch.pl  to get rid of that
>       lot.
> 
>    b) There are some build errors on non-x86 softmmu builds.
> 

Dave, thanks for reviews. I will fix these in next version.

Re: [Qemu-devel] [PATCH v2 02/13] kvm: introduce high-level API to support encrypted page migration

[Singh, Brijesh]

On 7/11/19 12:47 PM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> When memory encryption is enabled in VM, the guest pages will be
>> encrypted with the guest-specific key, to protect the confidentiality
>> of data in transit. To support the live migration we need to use
>> platform specific hooks to access the guest memory.
>>
>> The kvm_memcrypt_save_outgoing_page() can be used by the sender to write
>> the encrypted pages and metadata associated with it on the socket.
>>
>> The kvm_memcrypt_load_incoming_page() can be used by receiver to read the
>> incoming encrypted pages from the socket and load into the guest memory.
>>
>> Signed-off-by: Brijesh Singh <<brijesh.singh@amd.com>>
>> ---
>>   accel/kvm/kvm-all.c    | 27 +++++++++++++++++++++++++++
>>   accel/kvm/sev-stub.c   | 11 +++++++++++
>>   accel/stubs/kvm-stub.c | 12 ++++++++++++
>>   include/sysemu/kvm.h   | 12 ++++++++++++
>>   include/sysemu/sev.h   |  3 +++
>>   5 files changed, 65 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 3d86ae5052..162a2d5085 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -110,6 +110,10 @@ struct KVMState
>>       /* memory encryption */
>>       void *memcrypt_handle;
>>       int (*memcrypt_encrypt_data)(void *handle, uint8_t *ptr, uint64_t len);
>> +    int (*memcrypt_save_outgoing_page)(void *ehandle, QEMUFile *f,
>> +            uint8_t *ptr, uint32_t sz, uint64_t *bytes_sent);
>> +    int (*memcrypt_load_incoming_page)(void *ehandle, QEMUFile *f,
>> +            uint8_t *ptr);
>>   };
>>   
>>   KVMState *kvm_state;
>> @@ -165,6 +169,29 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
>>       return 1;
>>   }
>>   
>> +int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
>> +                                    uint32_t size, uint64_t *bytes_sent)
>> +{
>> +    if (kvm_state->memcrypt_handle &&
>> +        kvm_state->memcrypt_save_outgoing_page) {
>> +        return kvm_state->memcrypt_save_outgoing_page(kvm_state->memcrypt_handle,
>> +                    f, ptr, size, bytes_sent);
>> +    }
>> +
>> +    return 1;
> 
> This needs to be commented saying what the return values mean.
> I'm not sure what '1' means for the case when this didn't have
> encryption support.
> 

Agreed, I will add comment in API header about this. The value of zero
means success and anything else is failure.


>> +}
>> +
>> +int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
>> +{
>> +    if (kvm_state->memcrypt_handle &&
>> +        kvm_state->memcrypt_load_incoming_page) {
>> +        return kvm_state->memcrypt_load_incoming_page(kvm_state->memcrypt_handle,
>> +                    f, ptr);
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>>   static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
>>   {
>>       KVMState *s = kvm_state;
>> diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
>> index 4f97452585..c12a8e005e 100644
>> --- a/accel/kvm/sev-stub.c
>> +++ b/accel/kvm/sev-stub.c
>> @@ -24,3 +24,14 @@ void *sev_guest_init(const char *id)
>>   {
>>       return NULL;
>>   }
>> +
>> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
>> +                           uint32_t size, uint64_t *bytes_sent)
>> +{
>> +    return 1;
>> +}
>> +
>> +int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
>> +{
>> +    return 1;
>> +}
>> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
>> index 6feb66ed80..e14b879531 100644
>> --- a/accel/stubs/kvm-stub.c
>> +++ b/accel/stubs/kvm-stub.c
>> @@ -114,6 +114,18 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
>>     return 1;
>>   }
>>   
>> +int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr,
>> +                                    uint32_t size, uint64_t *bytes_sent)
>> +{
>> +    return 1;
>> +}
>> +
>> +int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr)
>> +{
>> +    return 1;
>> +}
>> +
>> +
>>   #ifndef CONFIG_USER_ONLY
>>   int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>>   {
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index acd90aebb6..bb6bcc143c 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -247,6 +247,18 @@ bool kvm_memcrypt_enabled(void);
>>    */
>>   int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>>   
>> +/**
>> + * kvm_memcrypt_save_outgoing_buffer - encrypt the outgoing buffer
>> + * and write to the wire.
>> + */
>> +int kvm_memcrypt_save_outgoing_page(QEMUFile *f, uint8_t *ptr, uint32_t size,
>> +                                    uint64_t *bytes_sent);
>> +
>> +/**
>> + * kvm_memcrypt_load_incoming_buffer - read the encrypt incoming buffer and copy
>> + * the buffer into the guest memory space.
>> + */
>> +int kvm_memcrypt_load_incoming_page(QEMUFile *f, uint8_t *ptr);
>>   
>>   #ifdef NEED_CPU_H
>>   #include "cpu.h"
>> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
>> index 98c1ec8d38..752a71b1c0 100644
>> --- a/include/sysemu/sev.h
>> +++ b/include/sysemu/sev.h
>> @@ -18,4 +18,7 @@
>>   
>>   void *sev_guest_init(const char *id);
>>   int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
>> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
>> +                           uint32_t size, uint64_t *bytes_sent);
>> +int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr);
>>   #endif
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v2 03/13] migration/ram: add support to send encrypted pages

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 
> 
> On 7/11/19 12:34 PM, Dr. David Alan Gilbert wrote:
> > * Singh, Brijesh (brijesh.singh@amd.com) wrote:
> >> 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 kvm_memcrypt_save_outgoing_page() is used
> >> by the sender to write the encrypted pages onto the socket, similarly the
> >> kvm_memcrypt_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>
> >> ---
> >>   migration/ram.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 53 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 908517fc2b..3c8977d508 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -57,6 +57,7 @@
> >>   #include "qemu/uuid.h"
> >>   #include "savevm.h"
> >>   #include "qemu/iov.h"
> >> +#include "sysemu/kvm.h"
> >>   
> >>   /***********************************************************/
> >>   /* ram save/restore */
> >> @@ -76,6 +77,7 @@
> >>   #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_PAGE   0x200
> > 
> > OK, that's our very last usable flag!  Use it wisely!
> > 
> 
> Hmm, maybe then I missed something. I thought the flag is 64-bit and
> we have more room. Did I miss something ?

The 64bit value written in the stream is 
  (the address of the page) | (the set of flags)

so the set of usable flags depends on the minimum page alignment
of which the worst case is ARM with a TARGET_PAGE_BITS_MIN of 10
(most others are 4k at least but that's still only 2 left).

> 
> >>   static inline bool is_zero_range(uint8_t *p, uint64_t size)
> >>   {
> >> @@ -460,6 +462,9 @@ 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)
> >>   {
> >>       CompressParam *param = opaque;
> >> @@ -2006,6 +2011,36 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
> >>       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;
> >> +
> >> +    p = block->host + offset;
> >> +
> >> +    ram_counters.transferred +=
> >> +        save_page_header(rs, rs->f, block,
> >> +                    offset | RAM_SAVE_FLAG_ENCRYPTED_PAGE);
> >> +
> >> +    ret = kvm_memcrypt_save_outgoing_page(rs->f, p,
> > 
> > I think you need to somehow abstract the kvm_memcrypt stuff; nothing
> > else in migration actually knows it's dealing with kvm.  So there
> > should be some indirection - probably through the cpu or the machine
> > type or something.
> > 
> 
> Currently, there are two interfaces by which we can know if we
> are dealing with encrypted guest. kvm_memcrypt_enabled() or
> MachineState->memory_encryption pointer. I did realized that
> migration code have not dealt with kvm so far.
> 
> How about target/i386/sev.c exporting the migration functions and
> based on state of MachineState->memory_encryption we call the
> SEV migration routines for the encrypted pages?

I'm migration not machine; so from my point of view the thing that's
important is making sure we've not got KVM direct dependencies in here;
if you've already got a MachineState->memory_encryption pointer then I'd
hope you could do something like:

    ret = MachineState->memory_encryption->ops->save(....)

> > Also, this isn't bisectable - you can't make this call in this patch
> > because you don't define/declare this function until a later patch.
> > 
> > 
> >> +                        TARGET_PAGE_SIZE, &bytes_xmit);
> >> +    if (ret) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    ram_counters.transferred += bytes_xmit;
> >> +    ram_counters.normal++;
> >> +
> >> +    return 1;
> >> +}
> >> +
> >>   static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
> >>                                    ram_addr_t offset, uint8_t *source_buf)
> >>   {
> >> @@ -2450,6 +2485,16 @@ 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 (kvm_memcrypt_enabled()) {
> >> +        return ram_save_encrypted_page(rs, pss, last_stage);
> >> +     }
> >> +
> >>       if (save_compress_page(rs, block, offset)) {
> >>           return 1;
> >>       }
> >> @@ -4271,7 +4316,8 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >>           }
> >>   
> >>           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_PAGE)) {
> >>               RAMBlock *block = ram_block_from_stream(f, flags);
> >>   
> >>               /*
> >> @@ -4391,6 +4437,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >>                   break;
> >>               }
> >>               break;
> >> +        case RAM_SAVE_FLAG_ENCRYPTED_PAGE:
> >> +            if (kvm_memcrypt_load_incoming_page(f, host)) {
> >> +                    error_report("Failed to encrypted incoming data");
> > 
> > 'Failed to *load* encrypted page' ?
> 
> Ah, thanks. It should be *load.
> 
> > 
> >> +                    ret = -EINVAL;
> > 
> > Do you want to actually return an error code here from
> > kvm_memcrypt_load_incoming_page, so we can keep hold of whether
> > it was something like a simple network error for the file stream
> > or something more complex.
> > 
> 
> Currently, the kvm_memcrypt_load_incoming_pages() return 0 or 1.
> 0 for the success and 1 for the failure. If we enhance the function to
> propagate the error code then some of them will not make sense for the
> migration code. Mainly those around the SEV FW failure etc.

Right, but I would like to get the return code from the actual reading
of data off the wire; just so I can tell a problem is a failure to read
rather than something weird in decryption.

Dave

> 
> > Dave
> > 
> >> +            }
> >> +            break;
> >>           case RAM_SAVE_FLAG_EOS:
> >>               /* normal exit */
> >>               multifd_recv_sync_main();
> >> -- 
> >> 2.17.1
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 07/13] target/i386: sev: do not create launch context for an incoming guest

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 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.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/sev.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6dbdc3cdf1..49baf8fef0 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -789,10 +789,16 @@ sev_guest_init(const char *id)
>          goto err;
>      }
>  
> -    ret = sev_launch_start(s);
> -    if (ret) {
> -        error_report("%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(s);
> +        if (ret) {
> +            error_report("%s: failed to create encryption context", __func__);
> +            goto err;
> +        }
>      }
>  
>      ram_block_notifier_add(&sev_ram_notifier);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 08/13] misc.json: add migrate-set-sev-info command

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> The command can be used by the hypervisor to specify the target Platform
> Diffie-Hellman key (PDH) and certificate chain before starting the SEV
> guest migration. The values passed through the command will be used while
> creating the outgoing encryption context.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

I'm wondering if it would make sense to have these as migration
parameters rather than using a new command.
You could just use string parameters.
(cc'ing Eric and Daniel for interface suggestions)

> ---
>  qapi/misc-target.json  | 18 ++++++++++++++++++
>  target/i386/monitor.c  | 10 ++++++++++
>  target/i386/sev-stub.c |  5 +++++
>  target/i386/sev.c      | 11 +++++++++++
>  target/i386/sev_i386.h |  9 ++++++++-
>  5 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index a00fd821eb..938dcaea14 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -266,3 +266,21 @@
>  ##
>  { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>    'if': 'defined(TARGET_ARM)' }
> +
> +##
> +# @migrate-set-sev-info:
> +#
> +# The command is used to provide the target host information used during the
> +# SEV guest.
> +#
> +# @pdh the target host platform diffie-hellman key encoded in base64
> +#
> +# @plat-cert the target host platform certificate chain encoded in base64
> +#
> +# @amd-cert AMD certificate chain which include ASK and OCA encoded in base64
> +#
> +# Since 4.2
> +#
> +##
> +{ 'command': 'migrate-set-sev-info',
> +  'data': { 'pdh': 'str', 'plat-cert': 'str', 'amd-cert' : 'str' }}
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 1f3b532fc2..4a5f50fb45 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -736,3 +736,13 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
>  
>      return data;
>  }
> +
> +void qmp_migrate_set_sev_info(const char *pdh, const char *plat_cert,
> +                              const char *amd_cert, Error **errp)
> +{
> +    if (sev_enabled()) {
> +        sev_set_migrate_info(pdh, plat_cert, amd_cert);
> +    } else {
> +        error_setg(errp, "SEV is not enabled");
> +    }
> +}
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index e5ee13309c..173bfa6374 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
>  {
>      return NULL;
>  }
> +
> +void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> +                          const char *amd_cert)
> +{
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 49baf8fef0..6c902d0be8 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -825,6 +825,17 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
>      return 0;
>  }
>  
> +void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> +                          const char *amd_cert)
> +{
> +    SEVState *s = sev_state;
> +
> +    s->remote_pdh = g_base64_decode(pdh, &s->remote_pdh_len);
> +    s->remote_plat_cert = g_base64_decode(plat_cert,
> +                                          &s->remote_plat_cert_len);
> +    s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);

What sanity checking is there that it's a valid base64 string of sane
length?

> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 55313441ae..3f3449b346 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -39,7 +39,8 @@ extern uint32_t sev_get_cbit_position(void);
>  extern uint32_t sev_get_reduced_phys_bits(void);
>  extern char *sev_get_launch_measurement(void);
>  extern SevCapability *sev_get_capabilities(void);
> -
> +extern void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> +                                 const char *amd_cert);
>  typedef struct QSevGuestInfo QSevGuestInfo;
>  typedef struct QSevGuestInfoClass QSevGuestInfoClass;
>  
> @@ -81,6 +82,12 @@ struct SEVState {
>      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;
>  };
>  
>  typedef struct SEVState SEVState;
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 08/13] misc.json: add migrate-set-sev-info command

[Daniel P. Berrangé]

On Fri, Jul 12, 2019 at 11:00:22AM +0100, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
> > The command can be used by the hypervisor to specify the target Platform
> > Diffie-Hellman key (PDH) and certificate chain before starting the SEV
> > guest migration. The values passed through the command will be used while
> > creating the outgoing encryption context.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> 
> I'm wondering if it would make sense to have these as migration
> parameters rather than using a new command.
> You could just use string parameters.
> (cc'ing Eric and Daniel for interface suggestions)

Either option would be fine from libvirt's POV I believe. On balance it is
probably slightly easier to deal with migration parameters, since libvirt
already has code for setting many such params.

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: [Qemu-devel] [PATCH v2 09/13] target/i386: sev: add support to encrypt the outgoing page

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 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>
> ---
>  accel/kvm/kvm-all.c      |   1 +
>  target/i386/sev.c        | 229 +++++++++++++++++++++++++++++++++++++++
>  target/i386/sev_i386.h   |   2 +
>  target/i386/trace-events |   3 +
>  4 files changed, 235 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c935e9366c..a9fb447248 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1792,6 +1792,7 @@ static int kvm_init(MachineState *ms)
>          }
>  
>          kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
> +        kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>      }
>  
>      ret = kvm_arch_init(ms, s);
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6c902d0be8..28b36c8035 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -27,6 +27,8 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "migration/blocker.h"
> +#include "migration/qemu-file.h"
> +#include "migration/misc.h"
>  
>  #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
> @@ -718,6 +720,39 @@ sev_vm_state_change(void *opaque, int running, RunState state)
>      }
>  }
>  
> +static void
> +sev_send_finish(void)
> +{
> +    int ret, error;
> +
> +    trace_kvm_sev_send_finish();
> +    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
> +    if (ret) {
> +        error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",

why LAUNCH?

> +                     __func__, ret, error, fw_error_to_str(error));
> +    }
> +
> +    sev_set_guest_state(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_STATE_SEND_UPDATE)) {
> +            sev_send_finish();
> +        }

I don't quite understand SEV_SEND_FINISH; is it just terminating the
migration process or is it actually making the VM unrunnable?
I'm interested in what the behaviour is on a failed migration - do
we lose both VMs or do we potentialyl have a memory clone?
(Neither are pretty!)

> +    }
> +}
> +
> +static Notifier sev_migration_state_notify = {
> +    .notify = sev_migration_state_notifier,
> +};
> +
>  void *
>  sev_guest_init(const char *id)
>  {
> @@ -804,6 +839,7 @@ sev_guest_init(const char *id)
>      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, s);
> +    add_migration_state_change_notifier(&sev_migration_state_notify);
>  
>      return s;
>  err:
> @@ -836,6 +872,199 @@ void sev_set_migrate_info(const char *pdh, const char *plat_cert,
>      s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
>  }
>  
> +static int
> +sev_get_send_session_length(void)
> +{
> +    int ret, fw_err = 0;
> +    struct kvm_sev_send_start *start;
> +
> +    start = g_new0(struct kvm_sev_send_start, 1);

These are tiny structures; they may as well be on the stack rather than
allocating/freeing them.

> +    ret = sev_ioctl(sev_state->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:
> +    g_free(start);
> +    return ret;
> +}
> +
> +static int
> +sev_send_start(SEVState *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;
> +
> +    if (!s->remote_pdh || !s->remote_plat_cert) {
> +        error_report("%s: missing remote PDH or PLAT_CERT", __func__);
> +        return 1;
> +    }
> +
> +    start = g_new0(struct kvm_sev_send_start, 1);
> +
> +    start->pdh_cert_uaddr = (unsigned long) s->remote_pdh;
> +    start->pdh_cert_len = s->remote_pdh_len;
> +
> +    start->plat_cert_uaddr = (unsigned long)s->remote_plat_cert;
> +    start->plat_cert_len = s->remote_plat_cert_len;
> +
> +    start->amd_cert_uaddr = (unsigned long)s->amd_cert;

Should these actually be case via a uint64_t ? They're explicitly
64bit - you might have to go via a uintptr_t to make some compilers
happy?

> +    start->amd_cert_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);
> +    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_cert_uaddr, start->plat_cert_len,
> +                             start->amd_cert_uaddr, start->amd_cert_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(SEV_STATE_SEND_UPDATE);
> +
> +err:
> +    g_free(start);
> +    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;
> +
> +    update = g_malloc0(sizeof(*update));
> +    if (!update) {
> +        return -1;
> +    }
> +
> +    ret = sev_ioctl(sev_state->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:
> +    g_free(update);
> +    return ret;
> +}
> +
> +static int
> +sev_send_update_data(SEVState *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);

When does this get freed?

> +    }
> +
> +    update = g_new0(struct kvm_sev_send_update_data, 1);
> +
> +    /* allocate transport buffer */
> +    trans = g_new(guchar, size);
> +
> +    update->hdr_uaddr = (unsigned long)s->send_packet_hdr;
> +    update->hdr_len = s->send_packet_hdr_len;
> +    update->guest_uaddr = (unsigned long)ptr;
> +    update->guest_len = size;
> +    update->trans_uaddr = (unsigned long)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);
> +    g_free(update);
> +    return ret;
> +}
> +
> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
> +                           uint32_t sz, uint64_t *bytes_sent)
> +{
> +    SEVState *s = sev_state;
> +
> +    /*
> +     * If this is a first buffer then create outgoing encryption context
> +     * and write our PDH, policy and session data.
> +     */
> +    if (!sev_check_state(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/sev_i386.h b/target/i386/sev_i386.h
> index 3f3449b346..2fdca5190d 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -88,6 +88,8 @@ struct SEVState {
>      size_t remote_plat_cert_len;
>      guchar *amd_cert;
>      size_t amd_cert_len;
> +    gchar *send_packet_hdr;
> +    size_t send_packet_hdr_len;
>  };
>  
>  typedef struct SEVState SEVState;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 789c700d4a..b41516cf9f 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -15,3 +15,6 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
>  kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
>  kvm_sev_launch_measurement(const char *value) "data %s"
>  kvm_sev_launch_finish(void) ""
> +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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 10/13] target/i386: sev: add support to load incoming encrypted page

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 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>
> ---
>  accel/kvm/kvm-all.c      |   1 +
>  target/i386/sev.c        | 126 ++++++++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |   3 +
>  3 files changed, 129 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index a9fb447248..7f94dba6f9 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1793,6 +1793,7 @@ static int kvm_init(MachineState *ms)
>  
>          kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>          kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
> +        kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
>      }
>  
>      ret = kvm_arch_init(ms, s);
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 28b36c8035..09a62d6f88 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -708,13 +708,34 @@ sev_launch_finish(SEVState *s)
>      }
>  }
>  
> +static int
> +sev_receive_finish(SEVState *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(SEV_STATE_RUNNING);
> +err:
> +    return ret;
> +}
> +
> +
>  static void
>  sev_vm_state_change(void *opaque, int running, RunState state)
>  {
>      SEVState *s = opaque;
>  
>      if (running) {
> -        if (!sev_check_state(SEV_STATE_RUNNING)) {
> +        if (sev_check_state(SEV_STATE_RECEIVE_UPDATE)) {
> +            sev_receive_finish(s);
> +        } else if (!sev_check_state(SEV_STATE_RUNNING)) {
>              sev_launch_finish(s);
>          }
>      }
> @@ -1065,6 +1086,109 @@ int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
>      return sev_send_update_data(s, f, ptr, sz, bytes_sent);
>  }
>  
> +static int
> +sev_receive_start(QSevGuestInfo *sev, QEMUFile *f)
> +{
> +    int ret = 1;
> +    int fw_error;
> +    struct kvm_sev_receive_start *start;
> +    gchar *session = NULL, *pdh_cert = NULL;
> +
> +    start = g_new0(struct kvm_sev_receive_start, 1);

Same as the send patch; these are tiny so may as well be on the stack

> +    /* 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);

You might want to bound the sizes of pdh_len and session_len
on reading; if the migration stream is badly corrupt you could
end up allocating and then trying to read a few GB ofjunk off the wire.

> +    pdh_cert = g_new(gchar, start->pdh_len);
> +    qemu_get_buffer(f, (uint8_t *)pdh_cert, start->pdh_len);
> +    start->pdh_uaddr = (unsigned long)pdh_cert;
> +
> +    /* get source session data */
> +    start->session_len = qemu_get_be32(f);
> +    session = g_new(gchar, start->session_len);
> +    qemu_get_buffer(f, (uint8_t *)session, start->session_len);
> +    start->session_uaddr = (unsigned long)session;
> +
> +    trace_kvm_sev_receive_start(start->policy, session, pdh_cert);
> +
> +    ret = sev_ioctl(sev_state->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), start->handle, "handle", &error_abort);
> +    sev_set_guest_state(SEV_STATE_RECEIVE_UPDATE);
> +err:
> +    g_free(start);
> +    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;
> +
> +    update = g_new0(struct kvm_sev_receive_update_data, 1);

Similar comments to the _start function

> +    /* get packet header */
> +    update->hdr_len = qemu_get_be32(f);
> +    hdr = g_new(gchar, update->hdr_len);
> +    qemu_get_buffer(f, (uint8_t *)hdr, update->hdr_len);
> +    update->hdr_uaddr = (unsigned long)hdr;
> +
> +    /* get transport buffer */
> +    update->trans_len = qemu_get_be32(f);
> +    trans = g_new(gchar, update->trans_len);
> +    update->trans_uaddr = (unsigned long)trans;
> +    qemu_get_buffer(f, (uint8_t *)update->trans_uaddr, update->trans_len);
> +
> +    update->guest_uaddr = (unsigned long) 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_state->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(update);
> +    g_free(hdr);
> +    return ret;
> +}
> +
> +int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
> +{
> +    SEVState *s = (SEVState *)handle;
> +
> +    /* 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(SEV_STATE_RECEIVE_UPDATE) &&
> +        sev_receive_start(s->sev_info, 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 b41516cf9f..609752cca7 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -18,3 +18,6 @@ kvm_sev_launch_finish(void) ""
>  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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 12/13] migration: add support to migrate page encryption bitmap

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> When memory encryption is enabled, the hypervisor maintains a page
> encryption bitmap which is referred by hypervisor during migratoin to check
> if page is private or shared. The bitmap 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. The KVM_{SET,GET}_PAGE_ENC_BITMAP can be used
> to get and set the bitmap for a given gfn range.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  accel/kvm/kvm-all.c      |  4 +++
>  migration/ram.c          | 11 +++++++
>  target/i386/sev.c        | 67 ++++++++++++++++++++++++++++++++++++++++
>  target/i386/trace-events |  2 ++
>  4 files changed, 84 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 442b1af36e..9e23088a94 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1831,6 +1831,10 @@ static int kvm_init(MachineState *ms)
>          kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>          kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>          kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
> +        kvm_state->memcrypt_load_incoming_page_enc_bitmap =
> +            sev_load_incoming_page_enc_bitmap;
> +        kvm_state->memcrypt_save_outgoing_page_enc_bitmap =
> +            sev_save_outgoing_page_enc_bitmap;
>      }
>  
>      ret = kvm_arch_init(ms, s);
> diff --git a/migration/ram.c b/migration/ram.c
> index d179867e1b..3a4bdf3c03 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -78,6 +78,7 @@
>  /* 0x80 is reserved in migration.h start with 0x100 next */
>  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
>  #define RAM_SAVE_FLAG_ENCRYPTED_PAGE   0x200
> +#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400 /* used in target/i386/sev.c */

OK, you see now we're toast!  We can't use that bit.

I see two ways around this:

  a) Define a flag to mean 'more flags'  - we can reuse the old
RAM_SAVE_FLAG_FULL to mean more-flags, and then send a second 64bit word
that actually contains the flags

  b) Do something clever where RAM_SAVE_FLAG_ENCRYPTED_PAGE | something
is your bitmap.  But then you need to be careful in any confusion during
the decoding.

>  static inline bool is_zero_range(uint8_t *p, uint64_t size)
>  {
> @@ -3595,6 +3596,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>      flush_compressed_data(rs);
>      ram_control_after_iterate(f, RAM_CONTROL_FINISH);
>  
> +    if (kvm_memcrypt_enabled()) {
> +        ret = kvm_memcrypt_save_outgoing_page_enc_bitmap(f);
> +    }
> +
>      rcu_read_unlock();
>  
>      multifd_send_sync_main();
> @@ -4469,6 +4474,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                      ret = -EINVAL;
>              }
>              break;
> +        case RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP:
> +            if (kvm_memcrypt_load_incoming_page_enc_bitmap(f)) {
> +                error_report("Failed to load page enc bitmap");
> +                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 09a62d6f88..93c6a90806 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -63,6 +63,7 @@ static const char *const sev_fw_errlist[] = {
>  };
>  
>  #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
> +#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400
>  
>  static int
>  sev_ioctl(int fd, int cmd, void *data, int *error)
> @@ -1189,6 +1190,72 @@ int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
>      return sev_receive_update_data(f, ptr);
>  }
>  
> +#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
> +
> +int sev_load_incoming_page_enc_bitmap(void *handle, QEMUFile *f)
> +{
> +    void *bmap;
> +    unsigned long npages;
> +    unsigned long bmap_size, base_gpa;
> +    struct kvm_page_enc_bitmap e = {};
> +
> +    base_gpa = qemu_get_be64(f);
> +    npages = qemu_get_be64(f);
> +    bmap_size = qemu_get_be64(f);
> +
> +    bmap = g_malloc0(bmap_size);
> +    qemu_get_buffer(f, (uint8_t *)bmap, bmap_size);

You should validate npages against bmap_size and validate bmap_size
for being huge if possible (although huge VMs are legal).
You could also sanity check base_gpa for alignment.

Treat data coming off the wire as hostile.

> +    if (kvm_vm_ioctl(kvm_state, KVM_SET_PAGE_ENC_BITMAP, &e) == -1) {
> +
> +    trace_kvm_sev_load_page_enc_bitmap(base_gpa, npages << TARGET_PAGE_BITS);
> +
> +    e.start_gfn = base_gpa >> TARGET_PAGE_BITS;
> +    e.num_pages = npages;
> +    e.enc_bitmap = bmap;

> +        error_report("KVM_SET_PAGE_ENC_BITMAP ioctl failed %d", errno);
> +        g_free(bmap);
> +        return 1;
> +    }
> +
> +    g_free(bmap);
> +
> +    return 0;
> +}
> +
> +int sev_save_outgoing_page_enc_bitmap(void *handle, QEMUFile *f,
> +                                      unsigned long start, uint64_t length)
> +{
> +    uint64_t size;
> +    struct kvm_page_enc_bitmap e = {};
> +
> +    if (!length) {
> +        return 0;
> +    }
> +
> +    size = ALIGN((length >> TARGET_PAGE_BITS), /*HOST_LONG_BITS*/ 64) / 8;
> +    e.enc_bitmap = g_malloc0(size);
> +    e.start_gfn = start >> TARGET_PAGE_BITS;
> +    e.num_pages = length >> TARGET_PAGE_BITS;
> +
> +    trace_kvm_sev_save_page_enc_bitmap(start, length);
> +
> +    if (kvm_vm_ioctl(kvm_state, KVM_GET_PAGE_ENC_BITMAP, &e) == -1) {
> +        error_report("%s: KVM_GET_PAGE_ENC_BITMAP ioctl failed %d",
> +                    __func__, errno);
> +        g_free(e.enc_bitmap);
> +        return 1;
> +    }
> +
> +    qemu_put_be64(f, RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP);
> +    qemu_put_be64(f, start);
> +    qemu_put_be64(f, e.num_pages);
> +    qemu_put_be64(f, size);
> +    qemu_put_buffer(f, (uint8_t *)e.enc_bitmap, size);
> +
> +    g_free(e.enc_bitmap);

This question is related to a question I had on an earlier patch;
but how 'stable' is this bitmap - i.e. cpa it change? Even across
a guest reboot?

> +    return 0;
> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 609752cca7..4c2be570f9 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -21,3 +21,5 @@ 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) ""
> +kvm_sev_save_page_enc_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 0x%" PRIx64
> +kvm_sev_load_page_enc_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 0x%" PRIx64
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 13/13] target/i386: sev: remove migration blocker

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/sev.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 93c6a90806..48336515a2 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -34,7 +34,6 @@
>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
>  
>  static SEVState *sev_state;
> -static Error *sev_mig_blocker;
>  
>  static const char *const sev_fw_errlist[] = {
>      "",
> @@ -686,7 +685,6 @@ static void
>  sev_launch_finish(SEVState *s)
>  {
>      int ret, error;
> -    Error *local_err = NULL;
>  
>      trace_kvm_sev_launch_finish();
>      ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
> @@ -697,16 +695,6 @@ sev_launch_finish(SEVState *s)
>      }
>  
>      sev_set_guest_state(SEV_STATE_RUNNING);
> -
> -    /* add migration blocker */
> -    error_setg(&sev_mig_blocker,
> -               "SEV: Migration is not implemented");
> -    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        error_free(sev_mig_blocker);
> -        exit(1);
> -    }
>  }
>  
>  static int
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 05/13] doc: update AMD SEV API spec web link

[Singh, Brijesh]

On 7/11/19 1:06 PM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   docs/amd-memory-encryption.txt | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
>> index 43bf3ee6a5..abb9a976f5 100644
>> --- a/docs/amd-memory-encryption.txt
>> +++ b/docs/amd-memory-encryption.txt
>> @@ -98,7 +98,7 @@ AMD Memory Encryption whitepaper:
>>   http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf
>>   
>>   Secure Encrypted Virtualization Key Management:
>> -[1] http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
>> +[1] https://developer.amd.com/sev/ (Secure Encrypted Virtualization API)
> 
> No; that reference [1] is used a few lines hire up for:
> 
> See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> complete flow chart.
> 
> 
> so that needs fixing up to actually point to that flowchart or
> equivalent.
> 

OK, I will fix them in next rev.

> That site is useful to include, but I guess it also needs a pointer
> to the Volume2 section 15.34 or the like.
> 

Sure, I will add the Volume2 section note.

-Brijesh

Re: [Qemu-devel] [PATCH v2 06/13] doc: update AMD SEV to include Live migration flow

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/amd-memory-encryption.txt | 42 +++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index abb9a976f5..374f4b0a94 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -89,7 +89,47 @@ 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_GET_PAGE_ENC_BITMAP ioctl can be used to get the guest page encryption
> +bitmap. The bitmap 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-sev-info object can be used to pass the target machine's PDH and
> +certificate chain.
> +
> +e.g
> +(QMP) migrate-sev-set-info pdh=<target_pdh> plat-cert=<target_cert_chain> \
> +       amd-cert=<amd_cert>
> +(QMP) migrate tcp:0:4444
> +
> +
> +During the migration flow, the SEND_START is called on the source hypervisor
> +to create outgoing encryption context. The SEV guest policy dectates whether
            ^an outgoing                                       ^ d*i*ctates

> +the certificate passed through the migrate-sev-set-info command will be
> +validate. SEND_UPDATE_DATA is called to encrypt the guest private pages.
          ^ validate*d* ?  How does the cert get validated anyway? As a
guest owner what rules can I set about which other host it gets migrated
to?

Actually, thinking about it, I didn't notice anything in the patchset
that checked/gave diagnostics about the guest policy - shouldn't there
be something that nicely says something like:
  'Guest policy has NOSEND set, guest can not be migrated'

?

> +After migration is completed, SEND_FINISH is called to destroy the encryption
> +context and make the VM non-runnable to protect it against the cloning.
                                                              ^ 'the' not needed
> +
> +On the target machine, RECEIVE_START is called first to create an
> +incoming encryption context. The RECEIVE_UPDATE_DATA is called to copy
> +the receieved 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).
>  
>  References
>  -----------------
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 04/13] kvm: add support to sync the page encryption state bitmap

[Singh, Brijesh]

On 7/11/19 2:05 PM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> The SEV VMs have concept of private and shared memory. The private memory
>> is encrypted with guest-specific key, while shared memory may be encrypted
>> with hyperivosr key. The KVM_GET_PAGE_ENC_BITMAP can be used to get a
>> bitmap indicating whether the guest page is private or shared. A private
>> page must be transmitted using the SEV migration commands.
>>
>> Add a cpu_physical_memory_sync_encrypted_bitmap() which can be used to sync
>> the page encryption bitmap for a given memory region.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   accel/kvm/kvm-all.c     |  38 ++++++++++
>>   include/exec/ram_addr.h | 161 ++++++++++++++++++++++++++++++++++++++--
>>   include/exec/ramlist.h  |   3 +-
>>   migration/ram.c         |  28 ++++++-
>>   4 files changed, 222 insertions(+), 8 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 162a2d5085..c935e9366c 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -504,6 +504,37 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>>   
>>   #define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
>>   
>> +/* sync page_enc bitmap */
>> +static int kvm_sync_page_enc_bitmap(KVMMemoryListener *kml,
>> +                                    MemoryRegionSection *section,
>> +                                    KVMSlot *mem)
> 
> How AMD/SEV specific is this? i.e. should this be in a target/ specific
> place?
> 


For now this is implemented in AMD/SEV specific kernel module.
But the interface exposed to userspace is a generic and can be
used by other vendors memory encryption feature. Because of this
I have added the syncing logic in generic kvm code.



>> +{
>> +    unsigned long size;
>> +    KVMState *s = kvm_state;
>> +    struct kvm_page_enc_bitmap e = {};
>> +    ram_addr_t pages = int128_get64(section->size) / getpagesize();
>> +    ram_addr_t start = section->offset_within_region +
>> +                       memory_region_get_ram_addr(section->mr);
>> +
>> +    size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
>> +                 /*HOST_LONG_BITS*/ 64) / 8;
>> +    e.enc_bitmap = g_malloc0(size);
>> +    e.start_gfn = mem->start_addr >> TARGET_PAGE_BITS;
>> +    e.num_pages = pages;
>> +    if (kvm_vm_ioctl(s, KVM_GET_PAGE_ENC_BITMAP, &e) == -1) {
>> +        DPRINTF("KVM_GET_PAGE_ENC_BITMAP ioctl failed %d\n", errno);
>> +        g_free(e.enc_bitmap);
>> +        return 1;
>> +    }
>> +
>> +    cpu_physical_memory_set_encrypted_lebitmap(e.enc_bitmap,
>> +                                               start, pages);
>> +
>> +    g_free(e.enc_bitmap);
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space
>>    * This function updates qemu's dirty bitmap using
>> @@ -553,6 +584,13 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
>>           }
>>   
>>           kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
>> +
>> +        if (kvm_memcrypt_enabled() &&
>> +            kvm_sync_page_enc_bitmap(kml, section, mem)) {
>> +            g_free(d.dirty_bitmap);
>> +            return -1;
>> +        }
>> +
>>           g_free(d.dirty_bitmap);
>>       }
>>   
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index f96777bb99..6fc6864194 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -51,6 +51,8 @@ struct RAMBlock {
>>       unsigned long *unsentmap;
>>       /* bitmap of already received pages in postcopy */
>>       unsigned long *receivedmap;
>> +    /* bitmap of page encryption state for an encrypted guest */
>> +    unsigned long *encbmap;
>>   };
>>   
>>   static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
>> @@ -314,9 +316,41 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>   }
>>   
>>   #if !defined(_WIN32)
>> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>> +
>> +static inline void cpu_physical_memory_set_encrypted_range(ram_addr_t start,
>> +                                                           ram_addr_t length,
>> +                                                           unsigned long val)
>> +{
>> +    unsigned long end, page;
>> +    unsigned long * const *src;
>> +
>> +    if (length == 0) {
>> +        return;
>> +    }
>> +
>> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
>> +    page = start >> TARGET_PAGE_BITS;
>> +
>> +    rcu_read_lock();
>> +
>> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
>> +
>> +    while (page < end) {
>> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
>> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
>> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
>> +
>> +        atomic_xchg(&src[idx][BIT_WORD(offset)], val);
>> +        page += num;
>> +    }
>> +
>> +    rcu_read_unlock();
>> +}
>> +
>> +static inline void cpu_physical_memory_set_dirty_enc_lebitmap(unsigned long *bitmap,
>>                                                             ram_addr_t start,
>> -                                                          ram_addr_t pages)
>> +                                                          ram_addr_t pages,
>> +                                                          bool enc_map)
>>   {
>>       unsigned long i, j;
>>       unsigned long page_number, c;
>> @@ -349,10 +383,14 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>               if (bitmap[k]) {
>>                   unsigned long temp = leul_to_cpu(bitmap[k]);
>>   
>> -                atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
>> -                atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>> -                if (tcg_enabled()) {
>> -                    atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
>> +                if (enc_map) {
>> +                    atomic_xchg(&blocks[DIRTY_MEMORY_ENCRYPTED][idx][offset], temp);
> 
> It makes me nervous that this is almost but not exactly like the other
> bitmaps;  I *think* you're saying the bits here are purely a matter of
> state about whether the page is encrypted and not a matter of actually
> dirtyness; in particular a page that is encrypted and then becomes dirty
> doesn't reset or clear this flag.


Yes, the bits here are state of the page and they doesn't get reset or
cleared with this flag. I agree its not exactly same, initially I did
went down to the path of querying the bitmap outside the dirty tracking
infrastructure and it proved to be lot of work. This is mainly because
migration code works with RAM offset but the kernel tracks the gfn. So,
we do need to map from Memslot to offset. Dirty bitmap tracking
infrastructure has those mapping logic in-place so I ended up simply
reusing it.


> 
>> +                } else {
>> +                    atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
>> +                    atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>> +                    if (tcg_enabled()) {
>> +                        atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
>> +                    }
>>                   }
>>               }
>>   
>> @@ -372,6 +410,17 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>            * especially when most of the memory is not dirty.
>>            */
>>           for (i = 0; i < len; i++) {
>> +
>> +            /* If its encrypted bitmap update, then we need to copy the bitmap
>> +             * value as-is to the destination.
>> +             */
>> +            if (enc_map) {
>> +                cpu_physical_memory_set_encrypted_range(start + i * TARGET_PAGE_SIZE,
>> +                                                        TARGET_PAGE_SIZE * hpratio,
>> +                                                        leul_to_cpu(bitmap[i]));
>> +                continue;
>> +            }
>> +
>>               if (bitmap[i] != 0) {
>>                   c = leul_to_cpu(bitmap[i]);
>>                   do {
>> @@ -387,6 +436,21 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>           }
>>       }
>>   }
>> +
>> +static inline void cpu_physical_memory_set_encrypted_lebitmap(unsigned long *bitmap,
>> +                                                              ram_addr_t start,
>> +                                                              ram_addr_t pages)
>> +{
>> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, true);
>> +}
>> +
>> +static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>> +                                                          ram_addr_t start,
>> +                                                          ram_addr_t pages)
>> +{
>> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, false);
>> +}
>> +
>>   #endif /* not _WIN32 */
>>   
>>   bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
>> @@ -406,6 +470,7 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
>>       cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
>>       cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
>>       cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
>> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_ENCRYPTED);
> 
> What are the ordering/consistency rules associated with this data.
> Specifically:
> 
>    Consider a page that transitions from being shared to encrypted
> (does that happen?) - but we've just done the sync's so we know the page
> is dirty, but we don't know it's encrypted; so we transmit the page as
> unencrypted; what happens?
> 

When a page is transitioned from private to shared, one (or two) of
the following action will be taken by the guest OS

a) update the pgtable memory

and

b) update the contents of the page

#a is must, #b is optional. The #a will dirty the pgtable memory, so
its safe to assume that pgtable will be sync'ed with correct attribute.
Similarly if  #b is performed then page address will be dirty and it
will be re-transmitted with updated data. But #b is not performed by
the guest then its okay to send the page through encryption path
because the content of that page is encrypted.


>    I *think* that means we should always sync the encryped bitmap before
> the dirty bitmap, so that if it flips we're guaranteed the dirty bitmap
> gets flipped again after the flip has happened; but my brain is starting
> to hurt....
> 
>    But, even if we're guaranteed to have a dirty for the next time
> around, I think we're always at risk of transmitting a page that
> has just flipped; so we'll be sure to transmit it again correctly,
> but we might transmit an encrypted page to a non-encrypted dest or
> the reverse - is that OK?
> 
> 

I don't think order matters much. If page was marked as shared in
pagetable but nobody has touched the contents of it then that page
will still contain encrypted data so its I think its OK to send as
encrypted.


>>   }
>>   
>>   
>> @@ -474,5 +539,89 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
>>   
>>       return num_dirty;
>>   }
>> +
>> +static inline bool cpu_physical_memory_test_encrypted(ram_addr_t start,
>> +                                                      ram_addr_t length)
>> +{
>> +    unsigned long end, page;
>> +    bool enc = false;
>> +    unsigned long * const *src;
>> +
>> +    if (length == 0) {
>> +        return enc;
>> +    }
>> +
>> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
>> +    page = start >> TARGET_PAGE_BITS;
>> +
>> +    rcu_read_lock();
>> +
>> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
>> +
>> +    while (page < end) {
>> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
>> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
>> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
>> +
>> +        enc |= atomic_read(&src[idx][BIT_WORD(offset)]);
> 
> I'm confused; I thought what I was about to get there was a long* and
> you were going to mask out a bit or set of bits.
> 

Ah good catch, thanks. Its bug, currently if one of the bit is set in
long* then I am saying its encryption which is wrong. I should be
masking out a bit and checking the specific position.



>> +        page += num;
>> +    }
>> +
>> +    rcu_read_unlock();
>> +
>> +    return enc;
>> +}
>> +
>> +static inline
>> +void cpu_physical_memory_sync_encrypted_bitmap(RAMBlock *rb,
>> +                                               ram_addr_t start,
>> +                                               ram_addr_t length)
>> +{
>> +    ram_addr_t addr;
>> +    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
>> +    unsigned long *dest = rb->encbmap;
>> +
>> +    /* start address and length is aligned at the start of a word? */
>> +    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
>> +         (start + rb->offset) &&
>> +        !(length & ((BITS_PER_LONG << TARGET_PAGE_BITS) - 1))) {
>> +        int k;
>> +        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
> 
> Feels odd it's an int.
> 
>> +        unsigned long * const *src;
>> +        unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
>> +        unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
>> +                                        DIRTY_MEMORY_BLOCK_SIZE);
>> +        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
>> +
>> +        rcu_read_lock();
>> +
>> +        src = atomic_rcu_read(
>> +                &ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
>> +
>> +        for (k = page; k < page + nr; k++) {
>> +            unsigned long bits = atomic_read(&src[idx][offset]);
>> +            dest[k] = bits;
>> +
>> +            if (++offset >= BITS_TO_LONGS(DIRTY_MEMORY_BLOCK_SIZE)) {
>> +                offset = 0;
>> +                idx++;
>> +            }
>> +        }
>> +
>> +        rcu_read_unlock();
>> +    } else {
>> +        ram_addr_t offset = rb->offset;
>> +
>> +        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
>> +            long k = (start + addr) >> TARGET_PAGE_BITS;
>> +            if (cpu_physical_memory_test_encrypted(start + addr + offset,
>> +                                                   TARGET_PAGE_SIZE)) {
>> +                set_bit(k, dest);
>> +            } else {
>> +                clear_bit(k, dest);
>> +            }
>> +        }
>> +    }
>> +}
>>   #endif
>>   #endif
>> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
>> index bc4faa1b00..2a5eab8b11 100644
>> --- a/include/exec/ramlist.h
>> +++ b/include/exec/ramlist.h
>> @@ -11,7 +11,8 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
>>   #define DIRTY_MEMORY_VGA       0
>>   #define DIRTY_MEMORY_CODE      1
>>   #define DIRTY_MEMORY_MIGRATION 2
>> -#define DIRTY_MEMORY_NUM       3        /* num of dirty bits */
>> +#define DIRTY_MEMORY_ENCRYPTED 3
>> +#define DIRTY_MEMORY_NUM       4        /* num of dirty bits */
>>   
>>   /* The dirty memory bitmap is split into fixed-size blocks to allow growth
>>    * under RCU.  The bitmap for a block can be accessed as follows:
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 3c8977d508..d179867e1b 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1680,6 +1680,9 @@ static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
>>       rs->migration_dirty_pages +=
>>           cpu_physical_memory_sync_dirty_bitmap(rb, 0, length,
>>                                                 &rs->num_dirty_pages_period);
>> +    if (kvm_memcrypt_enabled()) {
>> +        cpu_physical_memory_sync_encrypted_bitmap(rb, 0, length);
>> +    }
>>   }
>>   
>>   /**
>> @@ -2465,6 +2468,22 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
>>       return false;
>>   }
>>   
>> +/**
>> + * encrypted_test_bitmap: check if the page is encrypted
>> + *
>> + * Returns a bool indicating whether the page is encrypted.
>> + */
>> +static bool encrypted_test_bitmap(RAMState *rs, RAMBlock *block,
>> +                                  unsigned long page)
>> +{
>> +    /* ROM devices contains the unencrypted data */
>> +    if (memory_region_is_rom(block->mr)) {
>> +        return false;
>> +    }
>> +
>> +    return test_bit(page, block->encbmap);
>> +}
>> +
>>   /**
>>    * ram_save_target_page: save one target page
>>    *
>> @@ -2491,7 +2510,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>        * will take care of accessing the guest memory and re-encrypt it
>>        * for the transport purposes.
>>        */
>> -     if (kvm_memcrypt_enabled()) {
>> +     if (kvm_memcrypt_enabled() &&
>> +         encrypted_test_bitmap(rs, pss->block, pss->page)) {
>>           return ram_save_encrypted_page(rs, pss, last_stage);
>>        }
>>   
>> @@ -2724,6 +2744,8 @@ static void ram_save_cleanup(void *opaque)
>>           block->bmap = NULL;
>>           g_free(block->unsentmap);
>>           block->unsentmap = NULL;
>> +        g_free(block->encbmap);
>> +        block->encbmap = NULL;
>>       }
>>   
>>       xbzrle_cleanup();
>> @@ -3251,6 +3273,10 @@ static void ram_list_init_bitmaps(void)
>>                   block->unsentmap = bitmap_new(pages);
>>                   bitmap_set(block->unsentmap, 0, pages);
>>               }
>> +            if (kvm_memcrypt_enabled()) {
>> +                block->encbmap = bitmap_new(pages);
>> +                bitmap_set(block->encbmap, 0, pages);
>> +            }
>>           }
>>       }
>>   }
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v2 08/13] misc.json: add migrate-set-sev-info command

[Singh, Brijesh]

On 7/12/19 5:09 AM, Daniel P. Berrangé wrote:
> On Fri, Jul 12, 2019 at 11:00:22AM +0100, Dr. David Alan Gilbert wrote:
>> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>>> The command can be used by the hypervisor to specify the target Platform
>>> Diffie-Hellman key (PDH) and certificate chain before starting the SEV
>>> guest migration. The values passed through the command will be used while
>>> creating the outgoing encryption context.
>>>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>
>> I'm wondering if it would make sense to have these as migration
>> parameters rather than using a new command.
>> You could just use string parameters.
>> (cc'ing Eric and Daniel for interface suggestions)
> 
> Either option would be fine from libvirt's POV I believe. On balance it is
> probably slightly easier to deal with migration parameters, since libvirt
> already has code for setting many such params.
> 

OK, I will look into adding migration parameter for this.

thanks

Re: [Qemu-devel] [PATCH v2 09/13] target/i386: sev: add support to encrypt the outgoing page

[Singh, Brijesh]

On 7/12/19 5:43 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> 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>
>> ---
>>   accel/kvm/kvm-all.c      |   1 +
>>   target/i386/sev.c        | 229 +++++++++++++++++++++++++++++++++++++++
>>   target/i386/sev_i386.h   |   2 +
>>   target/i386/trace-events |   3 +
>>   4 files changed, 235 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index c935e9366c..a9fb447248 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1792,6 +1792,7 @@ static int kvm_init(MachineState *ms)
>>           }
>>   
>>           kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>> +        kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>>       }
>>   
>>       ret = kvm_arch_init(ms, s);
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 6c902d0be8..28b36c8035 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -27,6 +27,8 @@
>>   #include "sysemu/sysemu.h"
>>   #include "trace.h"
>>   #include "migration/blocker.h"
>> +#include "migration/qemu-file.h"
>> +#include "migration/misc.h"
>>   
>>   #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>>   #define DEFAULT_SEV_DEVICE      "/dev/sev"
>> @@ -718,6 +720,39 @@ sev_vm_state_change(void *opaque, int running, RunState state)
>>       }
>>   }
>>   
>> +static void
>> +sev_send_finish(void)
>> +{
>> +    int ret, error;
>> +
>> +    trace_kvm_sev_send_finish();
>> +    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
>> +    if (ret) {
>> +        error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",
> 
> why LAUNCH?


Its typo. I will fix in next rev.


> 
>> +                     __func__, ret, error, fw_error_to_str(error));
>> +    }
>> +
>> +    sev_set_guest_state(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_STATE_SEND_UPDATE)) {
>> +            sev_send_finish();
>> +        }
> 
> I don't quite understand SEV_SEND_FINISH; is it just terminating the
> migration process or is it actually making the VM unrunnable?
> I'm interested in what the behaviour is on a failed migration - do
> we lose both VMs or do we potentialyl have a memory clone?
> (Neither are pretty!)
> 


The SEV_SEND_FINISH will make the VM unrunnable. So basically a
failed migration will result both VMs unrunnable. The SEV FW commands
are designed in that way to prevent the memory clone.


>> +    }
>> +}
>> +
>> +static Notifier sev_migration_state_notify = {
>> +    .notify = sev_migration_state_notifier,
>> +};
>> +
>>   void *
>>   sev_guest_init(const char *id)
>>   {
>> @@ -804,6 +839,7 @@ sev_guest_init(const char *id)
>>       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, s);
>> +    add_migration_state_change_notifier(&sev_migration_state_notify);
>>   
>>       return s;
>>   err:
>> @@ -836,6 +872,199 @@ void sev_set_migrate_info(const char *pdh, const char *plat_cert,
>>       s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
>>   }
>>   
>> +static int
>> +sev_get_send_session_length(void)
>> +{
>> +    int ret, fw_err = 0;
>> +    struct kvm_sev_send_start *start;
>> +
>> +    start = g_new0(struct kvm_sev_send_start, 1);
> 
> These are tiny structures; they may as well be on the stack rather than
> allocating/freeing them.


Noted.


> 
>> +    ret = sev_ioctl(sev_state->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:
>> +    g_free(start);
>> +    return ret;
>> +}
>> +
>> +static int
>> +sev_send_start(SEVState *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;
>> +
>> +    if (!s->remote_pdh || !s->remote_plat_cert) {
>> +        error_report("%s: missing remote PDH or PLAT_CERT", __func__);
>> +        return 1;
>> +    }
>> +
>> +    start = g_new0(struct kvm_sev_send_start, 1);
>> +
>> +    start->pdh_cert_uaddr = (unsigned long) s->remote_pdh;
>> +    start->pdh_cert_len = s->remote_pdh_len;
>> +
>> +    start->plat_cert_uaddr = (unsigned long)s->remote_plat_cert;
>> +    start->plat_cert_len = s->remote_plat_cert_len;
>> +
>> +    start->amd_cert_uaddr = (unsigned long)s->amd_cert;
> 
> Should these actually be case via a uint64_t ? They're explicitly
> 64bit - you might have to go via a uintptr_t to make some compilers
> happy?
> 

Noted.

>> +    start->amd_cert_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);
>> +    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_cert_uaddr, start->plat_cert_len,
>> +                             start->amd_cert_uaddr, start->amd_cert_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(SEV_STATE_SEND_UPDATE);
>> +
>> +err:
>> +    g_free(start);
>> +    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;
>> +
>> +    update = g_malloc0(sizeof(*update));
>> +    if (!update) {
>> +        return -1;
>> +    }
>> +
>> +    ret = sev_ioctl(sev_state->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:
>> +    g_free(update);
>> +    return ret;
>> +}
>> +
>> +static int
>> +sev_send_update_data(SEVState *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);
> 
> When does this get freed?
> 

Ah, we could free it in SEND_FINISH to avoid leaking.


>> +    }
>> +
>> +    update = g_new0(struct kvm_sev_send_update_data, 1);
>> +
>> +    /* allocate transport buffer */
>> +    trans = g_new(guchar, size);
>> +
>> +    update->hdr_uaddr = (unsigned long)s->send_packet_hdr;
>> +    update->hdr_len = s->send_packet_hdr_len;
>> +    update->guest_uaddr = (unsigned long)ptr;
>> +    update->guest_len = size;
>> +    update->trans_uaddr = (unsigned long)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);
>> +    g_free(update);
>> +    return ret;
>> +}
>> +
>> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
>> +                           uint32_t sz, uint64_t *bytes_sent)
>> +{
>> +    SEVState *s = sev_state;
>> +
>> +    /*
>> +     * If this is a first buffer then create outgoing encryption context
>> +     * and write our PDH, policy and session data.
>> +     */
>> +    if (!sev_check_state(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/sev_i386.h b/target/i386/sev_i386.h
>> index 3f3449b346..2fdca5190d 100644
>> --- a/target/i386/sev_i386.h
>> +++ b/target/i386/sev_i386.h
>> @@ -88,6 +88,8 @@ struct SEVState {
>>       size_t remote_plat_cert_len;
>>       guchar *amd_cert;
>>       size_t amd_cert_len;
>> +    gchar *send_packet_hdr;
>> +    size_t send_packet_hdr_len;
>>   };
>>   
>>   typedef struct SEVState SEVState;
>> diff --git a/target/i386/trace-events b/target/i386/trace-events
>> index 789c700d4a..b41516cf9f 100644
>> --- a/target/i386/trace-events
>> +++ b/target/i386/trace-events
>> @@ -15,3 +15,6 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
>>   kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
>>   kvm_sev_launch_measurement(const char *value) "data %s"
>>   kvm_sev_launch_finish(void) ""
>> +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
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v2 10/13] target/i386: sev: add support to load incoming encrypted page

[Singh, Brijesh]

On 7/12/19 6:02 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> 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>
>> ---
>>   accel/kvm/kvm-all.c      |   1 +
>>   target/i386/sev.c        | 126 ++++++++++++++++++++++++++++++++++++++-
>>   target/i386/trace-events |   3 +
>>   3 files changed, 129 insertions(+), 1 deletion(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index a9fb447248..7f94dba6f9 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1793,6 +1793,7 @@ static int kvm_init(MachineState *ms)
>>   
>>           kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>>           kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>> +        kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
>>       }
>>   
>>       ret = kvm_arch_init(ms, s);
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 28b36c8035..09a62d6f88 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -708,13 +708,34 @@ sev_launch_finish(SEVState *s)
>>       }
>>   }
>>   
>> +static int
>> +sev_receive_finish(SEVState *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(SEV_STATE_RUNNING);
>> +err:
>> +    return ret;
>> +}
>> +
>> +
>>   static void
>>   sev_vm_state_change(void *opaque, int running, RunState state)
>>   {
>>       SEVState *s = opaque;
>>   
>>       if (running) {
>> -        if (!sev_check_state(SEV_STATE_RUNNING)) {
>> +        if (sev_check_state(SEV_STATE_RECEIVE_UPDATE)) {
>> +            sev_receive_finish(s);
>> +        } else if (!sev_check_state(SEV_STATE_RUNNING)) {
>>               sev_launch_finish(s);
>>           }
>>       }
>> @@ -1065,6 +1086,109 @@ int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
>>       return sev_send_update_data(s, f, ptr, sz, bytes_sent);
>>   }
>>   
>> +static int
>> +sev_receive_start(QSevGuestInfo *sev, QEMUFile *f)
>> +{
>> +    int ret = 1;
>> +    int fw_error;
>> +    struct kvm_sev_receive_start *start;
>> +    gchar *session = NULL, *pdh_cert = NULL;
>> +
>> +    start = g_new0(struct kvm_sev_receive_start, 1);
> 
> Same as the send patch; these are tiny so may as well be on the stack
> 
>> +    /* 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);
> 
> You might want to bound the sizes of pdh_len and session_len
> on reading; if the migration stream is badly corrupt you could
> end up allocating and then trying to read a few GB ofjunk off the wire.
> 

Good point. Noted.


>> +    pdh_cert = g_new(gchar, start->pdh_len);
>> +    qemu_get_buffer(f, (uint8_t *)pdh_cert, start->pdh_len);
>> +    start->pdh_uaddr = (unsigned long)pdh_cert;
>> +
>> +    /* get source session data */
>> +    start->session_len = qemu_get_be32(f);
>> +    session = g_new(gchar, start->session_len);
>> +    qemu_get_buffer(f, (uint8_t *)session, start->session_len);
>> +    start->session_uaddr = (unsigned long)session;
>> +
>> +    trace_kvm_sev_receive_start(start->policy, session, pdh_cert);
>> +
>> +    ret = sev_ioctl(sev_state->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), start->handle, "handle", &error_abort);
>> +    sev_set_guest_state(SEV_STATE_RECEIVE_UPDATE);
>> +err:
>> +    g_free(start);
>> +    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;
>> +
>> +    update = g_new0(struct kvm_sev_receive_update_data, 1);
> 
> Similar comments to the _start function
> 

Noted.


>> +    /* get packet header */
>> +    update->hdr_len = qemu_get_be32(f);
>> +    hdr = g_new(gchar, update->hdr_len);
>> +    qemu_get_buffer(f, (uint8_t *)hdr, update->hdr_len);
>> +    update->hdr_uaddr = (unsigned long)hdr;
>> +
>> +    /* get transport buffer */
>> +    update->trans_len = qemu_get_be32(f);
>> +    trans = g_new(gchar, update->trans_len);
>> +    update->trans_uaddr = (unsigned long)trans;
>> +    qemu_get_buffer(f, (uint8_t *)update->trans_uaddr, update->trans_len);
>> +
>> +    update->guest_uaddr = (unsigned long) 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_state->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(update);
>> +    g_free(hdr);
>> +    return ret;
>> +}
>> +
>> +int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
>> +{
>> +    SEVState *s = (SEVState *)handle;
>> +
>> +    /* 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(SEV_STATE_RECEIVE_UPDATE) &&
>> +        sev_receive_start(s->sev_info, 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 b41516cf9f..609752cca7 100644
>> --- a/target/i386/trace-events
>> +++ b/target/i386/trace-events
>> @@ -18,3 +18,6 @@ kvm_sev_launch_finish(void) ""
>>   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
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v2 09/13] target/i386: sev: add support to encrypt the outgoing page

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 
> 
> On 7/12/19 5:43 AM, Dr. David Alan Gilbert wrote:
> > * Singh, Brijesh (brijesh.singh@amd.com) wrote:
> >> 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>
> >> ---
> >>   accel/kvm/kvm-all.c      |   1 +
> >>   target/i386/sev.c        | 229 +++++++++++++++++++++++++++++++++++++++
> >>   target/i386/sev_i386.h   |   2 +
> >>   target/i386/trace-events |   3 +
> >>   4 files changed, 235 insertions(+)
> >>
> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >> index c935e9366c..a9fb447248 100644
> >> --- a/accel/kvm/kvm-all.c
> >> +++ b/accel/kvm/kvm-all.c
> >> @@ -1792,6 +1792,7 @@ static int kvm_init(MachineState *ms)
> >>           }
> >>   
> >>           kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
> >> +        kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
> >>       }
> >>   
> >>       ret = kvm_arch_init(ms, s);
> >> diff --git a/target/i386/sev.c b/target/i386/sev.c
> >> index 6c902d0be8..28b36c8035 100644
> >> --- a/target/i386/sev.c
> >> +++ b/target/i386/sev.c
> >> @@ -27,6 +27,8 @@
> >>   #include "sysemu/sysemu.h"
> >>   #include "trace.h"
> >>   #include "migration/blocker.h"
> >> +#include "migration/qemu-file.h"
> >> +#include "migration/misc.h"
> >>   
> >>   #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
> >>   #define DEFAULT_SEV_DEVICE      "/dev/sev"
> >> @@ -718,6 +720,39 @@ sev_vm_state_change(void *opaque, int running, RunState state)
> >>       }
> >>   }
> >>   
> >> +static void
> >> +sev_send_finish(void)
> >> +{
> >> +    int ret, error;
> >> +
> >> +    trace_kvm_sev_send_finish();
> >> +    ret = sev_ioctl(sev_state->sev_fd, KVM_SEV_SEND_FINISH, 0, &error);
> >> +    if (ret) {
> >> +        error_report("%s: LAUNCH_FINISH ret=%d fw_error=%d '%s'",
> > 
> > why LAUNCH?
> 
> 
> Its typo. I will fix in next rev.
> 
> 
> > 
> >> +                     __func__, ret, error, fw_error_to_str(error));
> >> +    }
> >> +
> >> +    sev_set_guest_state(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_STATE_SEND_UPDATE)) {
> >> +            sev_send_finish();
> >> +        }
> > 
> > I don't quite understand SEV_SEND_FINISH; is it just terminating the
> > migration process or is it actually making the VM unrunnable?
> > I'm interested in what the behaviour is on a failed migration - do
> > we lose both VMs or do we potentialyl have a memory clone?
> > (Neither are pretty!)
> > 
> 
> 
> The SEV_SEND_FINISH will make the VM unrunnable. So basically a
> failed migration will result both VMs unrunnable. The SEV FW commands
> are designed in that way to prevent the memory clone.

OK, can you add a note to the docs about that - the libvirt people will
also want to know that they can't restart the source on a failure.
(What happens if you try a 'cont' - how does it fail?)

It would be nice to fix this problem; I can imagine a system where
part of the key needed by the destinatino to decrypt the memory isn't
sent until very late in the day, and thus a failure before that point
would be safe.   I've no clue if that's actually doable!

Dave

> 
> >> +    }
> >> +}
> >> +
> >> +static Notifier sev_migration_state_notify = {
> >> +    .notify = sev_migration_state_notifier,
> >> +};
> >> +
> >>   void *
> >>   sev_guest_init(const char *id)
> >>   {
> >> @@ -804,6 +839,7 @@ sev_guest_init(const char *id)
> >>       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, s);
> >> +    add_migration_state_change_notifier(&sev_migration_state_notify);
> >>   
> >>       return s;
> >>   err:
> >> @@ -836,6 +872,199 @@ void sev_set_migrate_info(const char *pdh, const char *plat_cert,
> >>       s->amd_cert = g_base64_decode(amd_cert, &s->amd_cert_len);
> >>   }
> >>   
> >> +static int
> >> +sev_get_send_session_length(void)
> >> +{
> >> +    int ret, fw_err = 0;
> >> +    struct kvm_sev_send_start *start;
> >> +
> >> +    start = g_new0(struct kvm_sev_send_start, 1);
> > 
> > These are tiny structures; they may as well be on the stack rather than
> > allocating/freeing them.
> 
> 
> Noted.
> 
> 
> > 
> >> +    ret = sev_ioctl(sev_state->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:
> >> +    g_free(start);
> >> +    return ret;
> >> +}
> >> +
> >> +static int
> >> +sev_send_start(SEVState *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;
> >> +
> >> +    if (!s->remote_pdh || !s->remote_plat_cert) {
> >> +        error_report("%s: missing remote PDH or PLAT_CERT", __func__);
> >> +        return 1;
> >> +    }
> >> +
> >> +    start = g_new0(struct kvm_sev_send_start, 1);
> >> +
> >> +    start->pdh_cert_uaddr = (unsigned long) s->remote_pdh;
> >> +    start->pdh_cert_len = s->remote_pdh_len;
> >> +
> >> +    start->plat_cert_uaddr = (unsigned long)s->remote_plat_cert;
> >> +    start->plat_cert_len = s->remote_plat_cert_len;
> >> +
> >> +    start->amd_cert_uaddr = (unsigned long)s->amd_cert;
> > 
> > Should these actually be case via a uint64_t ? They're explicitly
> > 64bit - you might have to go via a uintptr_t to make some compilers
> > happy?
> > 
> 
> Noted.
> 
> >> +    start->amd_cert_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);
> >> +    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_cert_uaddr, start->plat_cert_len,
> >> +                             start->amd_cert_uaddr, start->amd_cert_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(SEV_STATE_SEND_UPDATE);
> >> +
> >> +err:
> >> +    g_free(start);
> >> +    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;
> >> +
> >> +    update = g_malloc0(sizeof(*update));
> >> +    if (!update) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    ret = sev_ioctl(sev_state->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:
> >> +    g_free(update);
> >> +    return ret;
> >> +}
> >> +
> >> +static int
> >> +sev_send_update_data(SEVState *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);
> > 
> > When does this get freed?
> > 
> 
> Ah, we could free it in SEND_FINISH to avoid leaking.
> 
> 
> >> +    }
> >> +
> >> +    update = g_new0(struct kvm_sev_send_update_data, 1);
> >> +
> >> +    /* allocate transport buffer */
> >> +    trans = g_new(guchar, size);
> >> +
> >> +    update->hdr_uaddr = (unsigned long)s->send_packet_hdr;
> >> +    update->hdr_len = s->send_packet_hdr_len;
> >> +    update->guest_uaddr = (unsigned long)ptr;
> >> +    update->guest_len = size;
> >> +    update->trans_uaddr = (unsigned long)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);
> >> +    g_free(update);
> >> +    return ret;
> >> +}
> >> +
> >> +int sev_save_outgoing_page(void *handle, QEMUFile *f, uint8_t *ptr,
> >> +                           uint32_t sz, uint64_t *bytes_sent)
> >> +{
> >> +    SEVState *s = sev_state;
> >> +
> >> +    /*
> >> +     * If this is a first buffer then create outgoing encryption context
> >> +     * and write our PDH, policy and session data.
> >> +     */
> >> +    if (!sev_check_state(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/sev_i386.h b/target/i386/sev_i386.h
> >> index 3f3449b346..2fdca5190d 100644
> >> --- a/target/i386/sev_i386.h
> >> +++ b/target/i386/sev_i386.h
> >> @@ -88,6 +88,8 @@ struct SEVState {
> >>       size_t remote_plat_cert_len;
> >>       guchar *amd_cert;
> >>       size_t amd_cert_len;
> >> +    gchar *send_packet_hdr;
> >> +    size_t send_packet_hdr_len;
> >>   };
> >>   
> >>   typedef struct SEVState SEVState;
> >> diff --git a/target/i386/trace-events b/target/i386/trace-events
> >> index 789c700d4a..b41516cf9f 100644
> >> --- a/target/i386/trace-events
> >> +++ b/target/i386/trace-events
> >> @@ -15,3 +15,6 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
> >>   kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
> >>   kvm_sev_launch_measurement(const char *value) "data %s"
> >>   kvm_sev_launch_finish(void) ""
> >> +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
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 12/13] migration: add support to migrate page encryption bitmap

[Singh, Brijesh]

On 7/12/19 6:30 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>> When memory encryption is enabled, the hypervisor maintains a page
>> encryption bitmap which is referred by hypervisor during migratoin to check
>> if page is private or shared. The bitmap 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. The KVM_{SET,GET}_PAGE_ENC_BITMAP can be used
>> to get and set the bitmap for a given gfn range.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   accel/kvm/kvm-all.c      |  4 +++
>>   migration/ram.c          | 11 +++++++
>>   target/i386/sev.c        | 67 ++++++++++++++++++++++++++++++++++++++++
>>   target/i386/trace-events |  2 ++
>>   4 files changed, 84 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 442b1af36e..9e23088a94 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1831,6 +1831,10 @@ static int kvm_init(MachineState *ms)
>>           kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>>           kvm_state->memcrypt_save_outgoing_page = sev_save_outgoing_page;
>>           kvm_state->memcrypt_load_incoming_page = sev_load_incoming_page;
>> +        kvm_state->memcrypt_load_incoming_page_enc_bitmap =
>> +            sev_load_incoming_page_enc_bitmap;
>> +        kvm_state->memcrypt_save_outgoing_page_enc_bitmap =
>> +            sev_save_outgoing_page_enc_bitmap;
>>       }
>>   
>>       ret = kvm_arch_init(ms, s);
>> diff --git a/migration/ram.c b/migration/ram.c
>> index d179867e1b..3a4bdf3c03 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -78,6 +78,7 @@
>>   /* 0x80 is reserved in migration.h start with 0x100 next */
>>   #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
>>   #define RAM_SAVE_FLAG_ENCRYPTED_PAGE   0x200
>> +#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400 /* used in target/i386/sev.c */
> 
> OK, you see now we're toast!  We can't use that bit.
> 
> I see two ways around this:
> 
>    a) Define a flag to mean 'more flags'  - we can reuse the old
> RAM_SAVE_FLAG_FULL to mean more-flags, and then send a second 64bit word
> that actually contains the flags
> 
>    b) Do something clever where RAM_SAVE_FLAG_ENCRYPTED_PAGE | something
> is your bitmap.  But then you need to be careful in any confusion during
> the decoding.
> 

Yes, I will look into doing something which does not require adding a
new flag.


>>   static inline bool is_zero_range(uint8_t *p, uint64_t size)
>>   {
>> @@ -3595,6 +3596,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>>       flush_compressed_data(rs);
>>       ram_control_after_iterate(f, RAM_CONTROL_FINISH);
>>   
>> +    if (kvm_memcrypt_enabled()) {
>> +        ret = kvm_memcrypt_save_outgoing_page_enc_bitmap(f);
>> +    }
>> +
>>       rcu_read_unlock();
>>   
>>       multifd_send_sync_main();
>> @@ -4469,6 +4474,12 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>                       ret = -EINVAL;
>>               }
>>               break;
>> +        case RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP:
>> +            if (kvm_memcrypt_load_incoming_page_enc_bitmap(f)) {
>> +                error_report("Failed to load page enc bitmap");
>> +                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 09a62d6f88..93c6a90806 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -63,6 +63,7 @@ static const char *const sev_fw_errlist[] = {
>>   };
>>   
>>   #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
>> +#define RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP       0x400
>>   
>>   static int
>>   sev_ioctl(int fd, int cmd, void *data, int *error)
>> @@ -1189,6 +1190,72 @@ int sev_load_incoming_page(void *handle, QEMUFile *f, uint8_t *ptr)
>>       return sev_receive_update_data(f, ptr);
>>   }
>>   
>> +#define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
>> +
>> +int sev_load_incoming_page_enc_bitmap(void *handle, QEMUFile *f)
>> +{
>> +    void *bmap;
>> +    unsigned long npages;
>> +    unsigned long bmap_size, base_gpa;
>> +    struct kvm_page_enc_bitmap e = {};
>> +
>> +    base_gpa = qemu_get_be64(f);
>> +    npages = qemu_get_be64(f);
>> +    bmap_size = qemu_get_be64(f);
>> +
>> +    bmap = g_malloc0(bmap_size);
>> +    qemu_get_buffer(f, (uint8_t *)bmap, bmap_size);
> 
> You should validate npages against bmap_size and validate bmap_size
> for being huge if possible (although huge VMs are legal).
> You could also sanity check base_gpa for alignment.
> 
> Treat data coming off the wire as hostile.
> 

Noted.


>> +    if (kvm_vm_ioctl(kvm_state, KVM_SET_PAGE_ENC_BITMAP, &e) == -1) {
>> +
>> +    trace_kvm_sev_load_page_enc_bitmap(base_gpa, npages << TARGET_PAGE_BITS);
>> +
>> +    e.start_gfn = base_gpa >> TARGET_PAGE_BITS;
>> +    e.num_pages = npages;
>> +    e.enc_bitmap = bmap;
> 
>> +        error_report("KVM_SET_PAGE_ENC_BITMAP ioctl failed %d", errno);
>> +        g_free(bmap);
>> +        return 1;
>> +    }
>> +
>> +    g_free(bmap);
>> +
>> +    return 0;
>> +}
>> +
>> +int sev_save_outgoing_page_enc_bitmap(void *handle, QEMUFile *f,
>> +                                      unsigned long start, uint64_t length)
>> +{
>> +    uint64_t size;
>> +    struct kvm_page_enc_bitmap e = {};
>> +
>> +    if (!length) {
>> +        return 0;
>> +    }
>> +
>> +    size = ALIGN((length >> TARGET_PAGE_BITS), /*HOST_LONG_BITS*/ 64) / 8;
>> +    e.enc_bitmap = g_malloc0(size);
>> +    e.start_gfn = start >> TARGET_PAGE_BITS;
>> +    e.num_pages = length >> TARGET_PAGE_BITS;
>> +
>> +    trace_kvm_sev_save_page_enc_bitmap(start, length);
>> +
>> +    if (kvm_vm_ioctl(kvm_state, KVM_GET_PAGE_ENC_BITMAP, &e) == -1) {
>> +        error_report("%s: KVM_GET_PAGE_ENC_BITMAP ioctl failed %d",
>> +                    __func__, errno);
>> +        g_free(e.enc_bitmap);
>> +        return 1;
>> +    }
>> +
>> +    qemu_put_be64(f, RAM_SAVE_FLAG_PAGE_ENCRYPTED_BITMAP);
>> +    qemu_put_be64(f, start);
>> +    qemu_put_be64(f, e.num_pages);
>> +    qemu_put_be64(f, size);
>> +    qemu_put_buffer(f, (uint8_t *)e.enc_bitmap, size);
>> +
>> +    g_free(e.enc_bitmap);
> 
> This question is related to a question I had on an earlier patch;
> but how 'stable' is this bitmap - i.e. cpa it change? Even across
> a guest reboot?
> 

Yes, its very much possible that bitmap will change across the reboots.
When page state is changed from private to shared or vice versa
KVM get immediate notification through a hypercall and updates the
bitmap.

At the very last stage of migration we should ensure that the snapshot
of bitmap is transmitted to the destination hypervisor. Once the
guest is resumed on destination its possible that it will flip
the page attribute. Typically the pages are converted from private
to shared for DMA operations or when driver doing dma_alloc_xxx etc.

So far, userspace does not have control of making a page shared. So
the updates to bitmap will be less frequent. Most of the update happens
during the kernel bootup or we driver reload etc.



>> +    return 0;
>> +}
>> +
>>   static void
>>   sev_register_types(void)
>>   {
>> diff --git a/target/i386/trace-events b/target/i386/trace-events
>> index 609752cca7..4c2be570f9 100644
>> --- a/target/i386/trace-events
>> +++ b/target/i386/trace-events
>> @@ -21,3 +21,5 @@ 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) ""
>> +kvm_sev_save_page_enc_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 0x%" PRIx64
>> +kvm_sev_load_page_enc_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 0x%" PRIx64
>> -- 
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v2 03/13] migration/ram: add support to send encrypted pages

[Singh, Brijesh]

On 7/12/19 4:27 AM, Dr. David Alan Gilbert wrote:

[snip]

>>>
>>> OK, that's our very last usable flag!  Use it wisely!
>>>
>>
>> Hmm, maybe then I missed something. I thought the flag is 64-bit and
>> we have more room. Did I miss something ?
> 
> The 64bit value written in the stream is
>    (the address of the page) | (the set of flags)
> 
> so the set of usable flags depends on the minimum page alignment
> of which the worst case is ARM with a TARGET_PAGE_BITS_MIN of 10
> (most others are 4k at least but that's still only 2 left).
> 
>>

Got it, thanks


>>
>> Currently, there are two interfaces by which we can know if we
>> are dealing with encrypted guest. kvm_memcrypt_enabled() or
>> MachineState->memory_encryption pointer. I did realized that
>> migration code have not dealt with kvm so far.
>>
>> How about target/i386/sev.c exporting the migration functions and
>> based on state of MachineState->memory_encryption we call the
>> SEV migration routines for the encrypted pages?
> 
> I'm migration not machine; so from my point of view the thing that's
> important is making sure we've not got KVM direct dependencies in here;
> if you've already got a MachineState->memory_encryption pointer then I'd
> hope you could do something like:
> 
>      ret = MachineState->memory_encryption->ops->save(....)
> 

I will look into doing something like this.

thanks

Re: [Qemu-devel] [PATCH v2 04/13] kvm: add support to sync the page encryption state bitmap

[Dr. David Alan Gilbert]

* Singh, Brijesh (brijesh.singh@amd.com) wrote:
> 
> 
> On 7/11/19 2:05 PM, Dr. David Alan Gilbert wrote:
> > * Singh, Brijesh (brijesh.singh@amd.com) wrote:
> >> The SEV VMs have concept of private and shared memory. The private memory
> >> is encrypted with guest-specific key, while shared memory may be encrypted
> >> with hyperivosr key. The KVM_GET_PAGE_ENC_BITMAP can be used to get a
> >> bitmap indicating whether the guest page is private or shared. A private
> >> page must be transmitted using the SEV migration commands.
> >>
> >> Add a cpu_physical_memory_sync_encrypted_bitmap() which can be used to sync
> >> the page encryption bitmap for a given memory region.
> >>
> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> >> ---
> >>   accel/kvm/kvm-all.c     |  38 ++++++++++
> >>   include/exec/ram_addr.h | 161 ++++++++++++++++++++++++++++++++++++++--
> >>   include/exec/ramlist.h  |   3 +-
> >>   migration/ram.c         |  28 ++++++-
> >>   4 files changed, 222 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >> index 162a2d5085..c935e9366c 100644
> >> --- a/accel/kvm/kvm-all.c
> >> +++ b/accel/kvm/kvm-all.c
> >> @@ -504,6 +504,37 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
> >>   
> >>   #define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
> >>   
> >> +/* sync page_enc bitmap */
> >> +static int kvm_sync_page_enc_bitmap(KVMMemoryListener *kml,
> >> +                                    MemoryRegionSection *section,
> >> +                                    KVMSlot *mem)
> > 
> > How AMD/SEV specific is this? i.e. should this be in a target/ specific
> > place?
> > 
> 
> 
> For now this is implemented in AMD/SEV specific kernel module.
> But the interface exposed to userspace is a generic and can be
> used by other vendors memory encryption feature. Because of this
> I have added the syncing logic in generic kvm code.

Ok, I'm not sure if anyone else will have quite the same bitmap
semantics; but we'll see.

<snip>

> >> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> >> index f96777bb99..6fc6864194 100644
> >> --- a/include/exec/ram_addr.h
> >> +++ b/include/exec/ram_addr.h
> >> @@ -51,6 +51,8 @@ struct RAMBlock {
> >>       unsigned long *unsentmap;
> >>       /* bitmap of already received pages in postcopy */
> >>       unsigned long *receivedmap;
> >> +    /* bitmap of page encryption state for an encrypted guest */
> >> +    unsigned long *encbmap;
> >>   };
> >>   
> >>   static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> >> @@ -314,9 +316,41 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
> >>   }
> >>   
> >>   #if !defined(_WIN32)
> >> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> >> +
> >> +static inline void cpu_physical_memory_set_encrypted_range(ram_addr_t start,
> >> +                                                           ram_addr_t length,
> >> +                                                           unsigned long val)
> >> +{
> >> +    unsigned long end, page;
> >> +    unsigned long * const *src;
> >> +
> >> +    if (length == 0) {
> >> +        return;
> >> +    }
> >> +
> >> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> >> +    page = start >> TARGET_PAGE_BITS;
> >> +
> >> +    rcu_read_lock();
> >> +
> >> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
> >> +
> >> +    while (page < end) {
> >> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> >> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> >> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
> >> +
> >> +        atomic_xchg(&src[idx][BIT_WORD(offset)], val);
> >> +        page += num;
> >> +    }
> >> +
> >> +    rcu_read_unlock();
> >> +}
> >> +
> >> +static inline void cpu_physical_memory_set_dirty_enc_lebitmap(unsigned long *bitmap,
> >>                                                             ram_addr_t start,
> >> -                                                          ram_addr_t pages)
> >> +                                                          ram_addr_t pages,
> >> +                                                          bool enc_map)
> >>   {
> >>       unsigned long i, j;
> >>       unsigned long page_number, c;
> >> @@ -349,10 +383,14 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> >>               if (bitmap[k]) {
> >>                   unsigned long temp = leul_to_cpu(bitmap[k]);
> >>   
> >> -                atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
> >> -                atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
> >> -                if (tcg_enabled()) {
> >> -                    atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
> >> +                if (enc_map) {
> >> +                    atomic_xchg(&blocks[DIRTY_MEMORY_ENCRYPTED][idx][offset], temp);
> > 
> > It makes me nervous that this is almost but not exactly like the other
> > bitmaps;  I *think* you're saying the bits here are purely a matter of
> > state about whether the page is encrypted and not a matter of actually
> > dirtyness; in particular a page that is encrypted and then becomes dirty
> > doesn't reset or clear this flag.
> 
> 
> Yes, the bits here are state of the page and they doesn't get reset or
> cleared with this flag. I agree its not exactly same, initially I did
> went down to the path of querying the bitmap outside the dirty tracking
> infrastructure and it proved to be lot of work. This is mainly because
> migration code works with RAM offset but the kernel tracks the gfn. So,
> we do need to map from Memslot to offset. Dirty bitmap tracking
> infrastructure has those mapping logic in-place so I ended up simply
> reusing it.

Hmm OK; it could be too confusing - just make sure you add a comment;
e.g. 'Note: not actually dirty flags, see ...' where appropriate.
You may end up renaming/cloning a few functions for clarity.

> 
> > 
> >> +                } else {
> >> +                    atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
> >> +                    atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
> >> +                    if (tcg_enabled()) {
> >> +                        atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
> >> +                    }
> >>                   }
> >>               }
> >>   
> >> @@ -372,6 +410,17 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> >>            * especially when most of the memory is not dirty.
> >>            */
> >>           for (i = 0; i < len; i++) {
> >> +
> >> +            /* If its encrypted bitmap update, then we need to copy the bitmap
> >> +             * value as-is to the destination.
> >> +             */
> >> +            if (enc_map) {
> >> +                cpu_physical_memory_set_encrypted_range(start + i * TARGET_PAGE_SIZE,
> >> +                                                        TARGET_PAGE_SIZE * hpratio,
> >> +                                                        leul_to_cpu(bitmap[i]));
> >> +                continue;
> >> +            }
> >> +
> >>               if (bitmap[i] != 0) {
> >>                   c = leul_to_cpu(bitmap[i]);
> >>                   do {
> >> @@ -387,6 +436,21 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> >>           }
> >>       }
> >>   }
> >> +
> >> +static inline void cpu_physical_memory_set_encrypted_lebitmap(unsigned long *bitmap,
> >> +                                                              ram_addr_t start,
> >> +                                                              ram_addr_t pages)
> >> +{
> >> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, true);
> >> +}
> >> +
> >> +static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> >> +                                                          ram_addr_t start,
> >> +                                                          ram_addr_t pages)
> >> +{
> >> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, false);
> >> +}
> >> +
> >>   #endif /* not _WIN32 */
> >>   
> >>   bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
> >> @@ -406,6 +470,7 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
> >>       cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
> >>       cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
> >>       cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
> >> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_ENCRYPTED);
> > 
> > What are the ordering/consistency rules associated with this data.
> > Specifically:
> > 
> >    Consider a page that transitions from being shared to encrypted
> > (does that happen?) - but we've just done the sync's so we know the page
> > is dirty, but we don't know it's encrypted; so we transmit the page as
> > unencrypted; what happens?
> > 
> 
> When a page is transitioned from private to shared, one (or two) of
> the following action will be taken by the guest OS
> 
> a) update the pgtable memory
> 
> and
> 
> b) update the contents of the page
> 
> #a is must, #b is optional. The #a will dirty the pgtable memory, so
> its safe to assume that pgtable will be sync'ed with correct attribute.
> Similarly if  #b is performed then page address will be dirty and it
> will be re-transmitted with updated data. But #b is not performed by
> the guest then its okay to send the page through encryption path
> because the content of that page is encrypted.

What's the relationship between updating the pgtable memory and this
bitmap you're syncing?

> 
> 
> >    I *think* that means we should always sync the encryped bitmap before
> > the dirty bitmap, so that if it flips we're guaranteed the dirty bitmap
> > gets flipped again after the flip has happened; but my brain is starting
> > to hurt....
> > 
> >    But, even if we're guaranteed to have a dirty for the next time
> > around, I think we're always at risk of transmitting a page that
> > has just flipped; so we'll be sure to transmit it again correctly,
> > but we might transmit an encrypted page to a non-encrypted dest or
> > the reverse - is that OK?
> > 
> > 
> 
> I don't think order matters much. If page was marked as shared in
> pagetable but nobody has touched the contents of it then that page
> will still contain encrypted data so its I think its OK to send as
> encrypted.

So are we really saying that the transfer of the contents of guest RAM
doesn't matter if it's encrypted or not - you could transfer all pages
as if they were encrypted even if they're actually shared - as long
as the bitmap is right at the end?

Dave

> 
> >>   }
> >>   
> >>   
> >> @@ -474,5 +539,89 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> >>   
> >>       return num_dirty;
> >>   }
> >> +
> >> +static inline bool cpu_physical_memory_test_encrypted(ram_addr_t start,
> >> +                                                      ram_addr_t length)
> >> +{
> >> +    unsigned long end, page;
> >> +    bool enc = false;
> >> +    unsigned long * const *src;
> >> +
> >> +    if (length == 0) {
> >> +        return enc;
> >> +    }
> >> +
> >> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> >> +    page = start >> TARGET_PAGE_BITS;
> >> +
> >> +    rcu_read_lock();
> >> +
> >> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
> >> +
> >> +    while (page < end) {
> >> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> >> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> >> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
> >> +
> >> +        enc |= atomic_read(&src[idx][BIT_WORD(offset)]);
> > 
> > I'm confused; I thought what I was about to get there was a long* and
> > you were going to mask out a bit or set of bits.
> > 
> 
> Ah good catch, thanks. Its bug, currently if one of the bit is set in
> long* then I am saying its encryption which is wrong. I should be
> masking out a bit and checking the specific position.
> 
> 
> 
> >> +        page += num;
> >> +    }
> >> +
> >> +    rcu_read_unlock();
> >> +
> >> +    return enc;
> >> +}
> >> +
> >> +static inline
> >> +void cpu_physical_memory_sync_encrypted_bitmap(RAMBlock *rb,
> >> +                                               ram_addr_t start,
> >> +                                               ram_addr_t length)
> >> +{
> >> +    ram_addr_t addr;
> >> +    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
> >> +    unsigned long *dest = rb->encbmap;
> >> +
> >> +    /* start address and length is aligned at the start of a word? */
> >> +    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
> >> +         (start + rb->offset) &&
> >> +        !(length & ((BITS_PER_LONG << TARGET_PAGE_BITS) - 1))) {
> >> +        int k;
> >> +        int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
> > 
> > Feels odd it's an int.
> > 
> >> +        unsigned long * const *src;
> >> +        unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
> >> +        unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
> >> +                                        DIRTY_MEMORY_BLOCK_SIZE);
> >> +        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> >> +
> >> +        rcu_read_lock();
> >> +
> >> +        src = atomic_rcu_read(
> >> +                &ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
> >> +
> >> +        for (k = page; k < page + nr; k++) {
> >> +            unsigned long bits = atomic_read(&src[idx][offset]);
> >> +            dest[k] = bits;
> >> +
> >> +            if (++offset >= BITS_TO_LONGS(DIRTY_MEMORY_BLOCK_SIZE)) {
> >> +                offset = 0;
> >> +                idx++;
> >> +            }
> >> +        }
> >> +
> >> +        rcu_read_unlock();
> >> +    } else {
> >> +        ram_addr_t offset = rb->offset;
> >> +
> >> +        for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
> >> +            long k = (start + addr) >> TARGET_PAGE_BITS;
> >> +            if (cpu_physical_memory_test_encrypted(start + addr + offset,
> >> +                                                   TARGET_PAGE_SIZE)) {
> >> +                set_bit(k, dest);
> >> +            } else {
> >> +                clear_bit(k, dest);
> >> +            }
> >> +        }
> >> +    }
> >> +}
> >>   #endif
> >>   #endif
> >> diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
> >> index bc4faa1b00..2a5eab8b11 100644
> >> --- a/include/exec/ramlist.h
> >> +++ b/include/exec/ramlist.h
> >> @@ -11,7 +11,8 @@ typedef struct RAMBlockNotifier RAMBlockNotifier;
> >>   #define DIRTY_MEMORY_VGA       0
> >>   #define DIRTY_MEMORY_CODE      1
> >>   #define DIRTY_MEMORY_MIGRATION 2
> >> -#define DIRTY_MEMORY_NUM       3        /* num of dirty bits */
> >> +#define DIRTY_MEMORY_ENCRYPTED 3
> >> +#define DIRTY_MEMORY_NUM       4        /* num of dirty bits */
> >>   
> >>   /* The dirty memory bitmap is split into fixed-size blocks to allow growth
> >>    * under RCU.  The bitmap for a block can be accessed as follows:
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 3c8977d508..d179867e1b 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -1680,6 +1680,9 @@ static void migration_bitmap_sync_range(RAMState *rs, RAMBlock *rb,
> >>       rs->migration_dirty_pages +=
> >>           cpu_physical_memory_sync_dirty_bitmap(rb, 0, length,
> >>                                                 &rs->num_dirty_pages_period);
> >> +    if (kvm_memcrypt_enabled()) {
> >> +        cpu_physical_memory_sync_encrypted_bitmap(rb, 0, length);
> >> +    }
> >>   }
> >>   
> >>   /**
> >> @@ -2465,6 +2468,22 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
> >>       return false;
> >>   }
> >>   
> >> +/**
> >> + * encrypted_test_bitmap: check if the page is encrypted
> >> + *
> >> + * Returns a bool indicating whether the page is encrypted.
> >> + */
> >> +static bool encrypted_test_bitmap(RAMState *rs, RAMBlock *block,
> >> +                                  unsigned long page)
> >> +{
> >> +    /* ROM devices contains the unencrypted data */
> >> +    if (memory_region_is_rom(block->mr)) {
> >> +        return false;
> >> +    }
> >> +
> >> +    return test_bit(page, block->encbmap);
> >> +}
> >> +
> >>   /**
> >>    * ram_save_target_page: save one target page
> >>    *
> >> @@ -2491,7 +2510,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
> >>        * will take care of accessing the guest memory and re-encrypt it
> >>        * for the transport purposes.
> >>        */
> >> -     if (kvm_memcrypt_enabled()) {
> >> +     if (kvm_memcrypt_enabled() &&
> >> +         encrypted_test_bitmap(rs, pss->block, pss->page)) {
> >>           return ram_save_encrypted_page(rs, pss, last_stage);
> >>        }
> >>   
> >> @@ -2724,6 +2744,8 @@ static void ram_save_cleanup(void *opaque)
> >>           block->bmap = NULL;
> >>           g_free(block->unsentmap);
> >>           block->unsentmap = NULL;
> >> +        g_free(block->encbmap);
> >> +        block->encbmap = NULL;
> >>       }
> >>   
> >>       xbzrle_cleanup();
> >> @@ -3251,6 +3273,10 @@ static void ram_list_init_bitmaps(void)
> >>                   block->unsentmap = bitmap_new(pages);
> >>                   bitmap_set(block->unsentmap, 0, pages);
> >>               }
> >> +            if (kvm_memcrypt_enabled()) {
> >> +                block->encbmap = bitmap_new(pages);
> >> +                bitmap_set(block->encbmap, 0, pages);
> >> +            }
> >>           }
> >>       }
> >>   }
> >> -- 
> >> 2.17.1
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 04/13] kvm: add support to sync the page encryption state bitmap

[Singh, Brijesh]

On 7/16/19 6:44 AM, Dr. David Alan Gilbert wrote:
> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>>
>>
>> On 7/11/19 2:05 PM, Dr. David Alan Gilbert wrote:
>>> * Singh, Brijesh (brijesh.singh@amd.com) wrote:
>>>> The SEV VMs have concept of private and shared memory. The private memory
>>>> is encrypted with guest-specific key, while shared memory may be encrypted
>>>> with hyperivosr key. The KVM_GET_PAGE_ENC_BITMAP can be used to get a
>>>> bitmap indicating whether the guest page is private or shared. A private
>>>> page must be transmitted using the SEV migration commands.
>>>>
>>>> Add a cpu_physical_memory_sync_encrypted_bitmap() which can be used to sync
>>>> the page encryption bitmap for a given memory region.
>>>>
>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>> ---
>>>>    accel/kvm/kvm-all.c     |  38 ++++++++++
>>>>    include/exec/ram_addr.h | 161 ++++++++++++++++++++++++++++++++++++++--
>>>>    include/exec/ramlist.h  |   3 +-
>>>>    migration/ram.c         |  28 ++++++-
>>>>    4 files changed, 222 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>> index 162a2d5085..c935e9366c 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -504,6 +504,37 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>>>>    
>>>>    #define ALIGN(x, y)  (((x)+(y)-1) & ~((y)-1))
>>>>    
>>>> +/* sync page_enc bitmap */
>>>> +static int kvm_sync_page_enc_bitmap(KVMMemoryListener *kml,
>>>> +                                    MemoryRegionSection *section,
>>>> +                                    KVMSlot *mem)
>>>
>>> How AMD/SEV specific is this? i.e. should this be in a target/ specific
>>> place?
>>>
>>
>>
>> For now this is implemented in AMD/SEV specific kernel module.
>> But the interface exposed to userspace is a generic and can be
>> used by other vendors memory encryption feature. Because of this
>> I have added the syncing logic in generic kvm code.
> 
> Ok, I'm not sure if anyone else will have quite the same bitmap
> semantics; but we'll see.
> 
> <snip>
> 
>>>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>>>> index f96777bb99..6fc6864194 100644
>>>> --- a/include/exec/ram_addr.h
>>>> +++ b/include/exec/ram_addr.h
>>>> @@ -51,6 +51,8 @@ struct RAMBlock {
>>>>        unsigned long *unsentmap;
>>>>        /* bitmap of already received pages in postcopy */
>>>>        unsigned long *receivedmap;
>>>> +    /* bitmap of page encryption state for an encrypted guest */
>>>> +    unsigned long *encbmap;
>>>>    };
>>>>    
>>>>    static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
>>>> @@ -314,9 +316,41 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>>>    }
>>>>    
>>>>    #if !defined(_WIN32)
>>>> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>>> +
>>>> +static inline void cpu_physical_memory_set_encrypted_range(ram_addr_t start,
>>>> +                                                           ram_addr_t length,
>>>> +                                                           unsigned long val)
>>>> +{
>>>> +    unsigned long end, page;
>>>> +    unsigned long * const *src;
>>>> +
>>>> +    if (length == 0) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
>>>> +    page = start >> TARGET_PAGE_BITS;
>>>> +
>>>> +    rcu_read_lock();
>>>> +
>>>> +    src = atomic_rcu_read(&ram_list.dirty_memory[DIRTY_MEMORY_ENCRYPTED])->blocks;
>>>> +
>>>> +    while (page < end) {
>>>> +        unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
>>>> +        unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
>>>> +        unsigned long num = MIN(end - page, DIRTY_MEMORY_BLOCK_SIZE - offset);
>>>> +
>>>> +        atomic_xchg(&src[idx][BIT_WORD(offset)], val);
>>>> +        page += num;
>>>> +    }
>>>> +
>>>> +    rcu_read_unlock();
>>>> +}
>>>> +
>>>> +static inline void cpu_physical_memory_set_dirty_enc_lebitmap(unsigned long *bitmap,
>>>>                                                              ram_addr_t start,
>>>> -                                                          ram_addr_t pages)
>>>> +                                                          ram_addr_t pages,
>>>> +                                                          bool enc_map)
>>>>    {
>>>>        unsigned long i, j;
>>>>        unsigned long page_number, c;
>>>> @@ -349,10 +383,14 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>>>                if (bitmap[k]) {
>>>>                    unsigned long temp = leul_to_cpu(bitmap[k]);
>>>>    
>>>> -                atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
>>>> -                atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>>>> -                if (tcg_enabled()) {
>>>> -                    atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
>>>> +                if (enc_map) {
>>>> +                    atomic_xchg(&blocks[DIRTY_MEMORY_ENCRYPTED][idx][offset], temp);
>>>
>>> It makes me nervous that this is almost but not exactly like the other
>>> bitmaps;  I *think* you're saying the bits here are purely a matter of
>>> state about whether the page is encrypted and not a matter of actually
>>> dirtyness; in particular a page that is encrypted and then becomes dirty
>>> doesn't reset or clear this flag.
>>
>>
>> Yes, the bits here are state of the page and they doesn't get reset or
>> cleared with this flag. I agree its not exactly same, initially I did
>> went down to the path of querying the bitmap outside the dirty tracking
>> infrastructure and it proved to be lot of work. This is mainly because
>> migration code works with RAM offset but the kernel tracks the gfn. So,
>> we do need to map from Memslot to offset. Dirty bitmap tracking
>> infrastructure has those mapping logic in-place so I ended up simply
>> reusing it.
> 
> Hmm OK; it could be too confusing - just make sure you add a comment;
> e.g. 'Note: not actually dirty flags, see ...' where appropriate.
> You may end up renaming/cloning a few functions for clarity.
> 

OK, I will rename some of those functions to avoid the confusion.


>>
>>>
>>>> +                } else {
>>>> +                    atomic_or(&blocks[DIRTY_MEMORY_MIGRATION][idx][offset], temp);
>>>> +                    atomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>>>> +                    if (tcg_enabled()) {
>>>> +                        atomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset], temp);
>>>> +                    }
>>>>                    }
>>>>                }
>>>>    
>>>> @@ -372,6 +410,17 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>>>             * especially when most of the memory is not dirty.
>>>>             */
>>>>            for (i = 0; i < len; i++) {
>>>> +
>>>> +            /* If its encrypted bitmap update, then we need to copy the bitmap
>>>> +             * value as-is to the destination.
>>>> +             */
>>>> +            if (enc_map) {
>>>> +                cpu_physical_memory_set_encrypted_range(start + i * TARGET_PAGE_SIZE,
>>>> +                                                        TARGET_PAGE_SIZE * hpratio,
>>>> +                                                        leul_to_cpu(bitmap[i]));
>>>> +                continue;
>>>> +            }
>>>> +
>>>>                if (bitmap[i] != 0) {
>>>>                    c = leul_to_cpu(bitmap[i]);
>>>>                    do {
>>>> @@ -387,6 +436,21 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>>>            }
>>>>        }
>>>>    }
>>>> +
>>>> +static inline void cpu_physical_memory_set_encrypted_lebitmap(unsigned long *bitmap,
>>>> +                                                              ram_addr_t start,
>>>> +                                                              ram_addr_t pages)
>>>> +{
>>>> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, true);
>>>> +}
>>>> +
>>>> +static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>>> +                                                          ram_addr_t start,
>>>> +                                                          ram_addr_t pages)
>>>> +{
>>>> +    return cpu_physical_memory_set_dirty_enc_lebitmap(bitmap, start, pages, false);
>>>> +}
>>>> +
>>>>    #endif /* not _WIN32 */
>>>>    
>>>>    bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
>>>> @@ -406,6 +470,7 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
>>>>        cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION);
>>>>        cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA);
>>>>        cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
>>>> +    cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_ENCRYPTED);
>>>
>>> What are the ordering/consistency rules associated with this data.
>>> Specifically:
>>>
>>>     Consider a page that transitions from being shared to encrypted
>>> (does that happen?) - but we've just done the sync's so we know the page
>>> is dirty, but we don't know it's encrypted; so we transmit the page as
>>> unencrypted; what happens?
>>>
>>
>> When a page is transitioned from private to shared, one (or two) of
>> the following action will be taken by the guest OS
>>
>> a) update the pgtable memory
>>
>> and
>>
>> b) update the contents of the page
>>
>> #a is must, #b is optional. The #a will dirty the pgtable memory, so
>> its safe to assume that pgtable will be sync'ed with correct attribute.
>> Similarly if  #b is performed then page address will be dirty and it
>> will be re-transmitted with updated data. But #b is not performed by
>> the guest then its okay to send the page through encryption path
>> because the content of that page is encrypted.
> 
> What's the relationship between updating the pgtable memory and this
> bitmap you're syncing?
> 


When guest toggles the encryption attribute of a page in a pgtable
memory it issues a hypercall. The hypercall contains two information:
a) encryption state
b) gfn

KVM updates the bitmap with the page encryption state, we are syncing
this bitmap during the migration to get the gfn encryption state. If
gfn is private then use SEV command else fallback to standard migration
flow.



>>
>>
>>>     I *think* that means we should always sync the encryped bitmap before
>>> the dirty bitmap, so that if it flips we're guaranteed the dirty bitmap
>>> gets flipped again after the flip has happened; but my brain is starting
>>> to hurt....
>>>
>>>     But, even if we're guaranteed to have a dirty for the next time
>>> around, I think we're always at risk of transmitting a page that
>>> has just flipped; so we'll be sure to transmit it again correctly,
>>> but we might transmit an encrypted page to a non-encrypted dest or
>>> the reverse - is that OK?
>>>
>>>
>>
>> I don't think order matters much. If page was marked as shared in
>> pagetable but nobody has touched the contents of it then that page
>> will still contain encrypted data so its I think its OK to send as
>> encrypted.
> 
> So are we really saying that the transfer of the contents of guest RAM
> doesn't matter if it's encrypted or not - you could transfer all pages
> as if they were encrypted even if they're actually shared - as long
> as the bitmap is right at the end?
> 

That's not what I mean. I was trying to say the order of sync
does not effect the outcome if the page state is changed while we
are migrating the guest.

A flow should be:
a) Before migration, query the bitmap
b) Transfer the pages based on the bitmap state
c) If page state is changed during the migration, it will force dirty
   bitmap tracker to resync the bitmaps. This is because changing the
   page state will cause pgtable memory updates.
d) If contents of the page is not changed then dirty page tracker will
   not re-transmit the page hence we will not get chance to resend the
   page with updated state. This is OK.
e) If the content of the page is changed after guest updates the page
    state then dirty tracker will see that page is changed and attempt
    to re-trasmit the page based on re-sync'ed bitmap state.

-Brijesh

Re: [Qemu-devel] [PATCH v2 06/13] doc: update AMD SEV to include Live migration flow

[Venu Busireddy]

On 2019-07-10 20:23:03 +0000, Singh, Brijesh wrote:
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  docs/amd-memory-encryption.txt | 42 +++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/amd-memory-encryption.txt b/docs/amd-memory-encryption.txt
> index abb9a976f5..374f4b0a94 100644
> --- a/docs/amd-memory-encryption.txt
> +++ b/docs/amd-memory-encryption.txt
> @@ -89,7 +89,47 @@ 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_GET_PAGE_ENC_BITMAP ioctl can be used to get the guest page encryption
> +bitmap. The bitmap 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-sev-info object can be used to pass the target machine's PDH and
> +certificate chain.
> +
> +e.g
> +(QMP) migrate-sev-set-info pdh=<target_pdh> plat-cert=<target_cert_chain> \

'migrate-sev-set-info' needs to be changed to 'migrate-set-sev-info'.

> +       amd-cert=<amd_cert>
> +(QMP) migrate tcp:0:4444
> +
> +
> +During the migration flow, the SEND_START is called on the source hypervisor
> +to create outgoing encryption context. The SEV guest policy dectates whether
> +the certificate passed through the migrate-sev-set-info command will be

Same here.

> +validate. 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 the 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 receieved 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).
>  
>  References
>  -----------------
> -- 
> 2.17.1
> 
>