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

["Singh, Brijesh" <brijesh.singh@amd.com>]

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
>