Re: [Qemu-devel] [PATCH v3 11/14] migration: add support to migrate page encryption bitmap

[Dr. David Alan Gilbert]

* Singh, Brijesh (address@hidden) 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 <address@hidden>
> ---
>  accel/kvm/kvm-all.c      | 27 ++++++++++++
>  accel/kvm/sev-stub.c     | 11 +++++
>  include/sysemu/sev.h     |  6 +++
>  target/i386/sev.c        | 93 ++++++++++++++++++++++++++++++++++++++++
>  target/i386/trace-events |  2 +
>  5 files changed, 139 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ba0e7fa2be..f4d136b022 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -185,10 +185,37 @@ static int kvm_memcrypt_load_incoming_page(QEMUFile *f, 
> uint8_t *ptr)
>      return sev_load_incoming_page(kvm_state->memcrypt_handle, f, ptr);
>  }
>  
> +static int kvm_memcrypt_save_outgoing_bitmap(QEMUFile *f)
> +{
> +    KVMMemoryListener *kml = &kvm_state->memory_listener;
> +    KVMState *s = kvm_state;
> +    int ret = 1, i;
> +
> +    /* iterate through all the registered slots and send the bitmap */
> +    for (i = 0; i < s->nr_slots; i++) {
> +        KVMSlot *mem = &kml->slots[i];
> +        ret = sev_save_outgoing_bitmap(s->memcrypt_handle, f, 
> mem->start_addr,
> +                                       mem->memory_size,
> +                                       (i + 1) == s->nr_slots);
> +        if (ret) {
> +            return 1;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static int kvm_memcrypt_load_incoming_bitmap(QEMUFile *f)
> +{
> +    return sev_load_incoming_bitmap(kvm_state->memcrypt_handle, f);
> +}
> +
>  static struct MachineMemoryEncryptionOps sev_memory_encryption_ops = {
>      .save_setup = kvm_memcrypt_save_setup,
>      .save_outgoing_page = kvm_memcrypt_save_outgoing_page,
>      .load_incoming_page = kvm_memcrypt_load_incoming_page,
> +    .save_outgoing_bitmap = kvm_memcrypt_save_outgoing_bitmap,
> +    .load_incoming_bitmap = kvm_memcrypt_load_incoming_bitmap,
>  };
>  
>  int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)
> diff --git a/accel/kvm/sev-stub.c b/accel/kvm/sev-stub.c
> index 1b6773ef72..fa96225abc 100644
> --- a/accel/kvm/sev-stub.c
> +++ b/accel/kvm/sev-stub.c
> @@ -41,3 +41,14 @@ int sev_load_incoming_page(void *handle, QEMUFile *f, 
> uint8_t *ptr)
>  {
>      return 1;
>  }
> +
> +int sev_save_outgoing_bitmap(void *handle, QEMUFile *f,
> +                             unsigned long start, uint64_t length, bool last)
> +{
> +    return 1;
> +}
> +
> +int sev_load_incoming_bitmap(void *handle, QEMUFile *f)
> +{
> +    return 1;
> +}
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index e9371bd2dd..f777083c94 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -16,6 +16,9 @@
>  
>  #include "sysemu/kvm.h"
>  
> +#define RAM_SAVE_ENCRYPTED_PAGE        0x1
> +#define RAM_SAVE_ENCRYPTED_BITMAP      0x2
> +
>  void *sev_guest_init(const char *id);
>  int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
>  int sev_save_setup(void *handle, const char *pdh, const char *plat_cert,
> @@ -23,4 +26,7 @@ int sev_save_setup(void *handle, const char *pdh, const 
> char *plat_cert,
>  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_bitmap(void *handle, QEMUFile *f);
> +int sev_save_outgoing_bitmap(void *handle, QEMUFile *f, unsigned long start,
> +                             uint64_t length, bool last);
>  #endif
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index a689011991..9d643e720c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -65,6 +65,8 @@ static const char *const sev_fw_errlist[] = {
>  #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
>  
>  #define SEV_FW_BLOB_MAX_SIZE            0x4000          /* 16KB */
> +#define ENCRYPTED_BITMAP_CONTINUE       0x1
> +#define ENCRYPTED_BITMAP_END            0x2
>  
>  static int
>  sev_ioctl(int fd, int cmd, void *data, int *error)
> @@ -1232,6 +1234,97 @@ 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_bitmap(void *handle, QEMUFile *f)
> +{
> +    void *bmap;
> +    unsigned long bmap_size, base_gpa;
> +    unsigned long npages, expected_size, length;
> +    struct kvm_page_enc_bitmap e = {};
> +    int status;
> +
> +    status = qemu_get_be32(f);
> +
> +    while (status != ENCRYPTED_BITMAP_END) {

It would be good to be more defensive - I think you're always expecting
ENCRYPTED_BITMAP_CONTINUE?   I'd error out if the status
isn't one of the few things you expect.

(I know I might be a bit paranoid, but I'm used to very broken migration
streams!)

> +        base_gpa = qemu_get_be64(f);
> +        npages = qemu_get_be64(f);
> +        bmap_size = qemu_get_be64(f);

It's also a useful place to try a qemu_file_get_error(f) - it helps spot
whether an error is really a broken header or the stream ran out of
bytes unexpectedly.

But what you have is OK, but never any harm to make it more defensive.


Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> +        /*
> +         * Before allocating the bitmap buffer, lets do some bound check to
> +         * ensure that we are not dealing with corrupted stream.
> +         */
> +        length = npages << TARGET_PAGE_BITS;
> +        expected_size = ALIGN((length >> TARGET_PAGE_BITS), 64) / 8;
> +        if (expected_size != bmap_size) {
> +            error_report("corrupted bitmap expected size %ld got %ld",
> +                    expected_size, bmap_size);
> +            return 1;
> +        }
> +
> +        bmap = g_malloc0(bmap_size);
> +        qemu_get_buffer(f, (uint8_t *)bmap, bmap_size);
> +
> +        trace_kvm_sev_load_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);
> +
> +        status = qemu_get_be32(f);
> +    }
> +
> +    return 0;
> +}
> +
> +int sev_save_outgoing_bitmap(void *handle, QEMUFile *f,
> +                             unsigned long start, uint64_t length, bool last)
> +{
> +    uint64_t size;
> +    struct kvm_page_enc_bitmap e = {};
> +
> +    if (!length) {
> +        /* nothing to send */
> +        goto done;
> +    }
> +
> +    size = ALIGN((length >> TARGET_PAGE_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_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_be32(f, ENCRYPTED_BITMAP_CONTINUE);
> +    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);
> +
> +done:
> +    if (last) {
> +        qemu_put_be32(f, ENCRYPTED_BITMAP_END);
> +    }
> +    return 0;
> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 609752cca7..853a3870ab 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_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 
> 0x%" PRIx64
> +kvm_sev_load_bitmap(uint64_t start, uint64_t len) "start 0x%" PRIx64 " len 
> 0x%" PRIx64
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK