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 (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      |  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 / address@hidden / Manchester, UK
>