Re: [Qemu-devel] [PATCH v9 16/29] sev/i386: add command to encrypt guest memory region

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> The KVM_SEV_LAUNCH_UPDATE_DATA command is used to encrypt a guest memory
> region using the VM Encryption Key created using LAUNCH_START.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  accel/kvm/kvm-all.c      |  2 ++
>  include/sysemu/sev.h     |  1 +
>  stubs/sev.c              |  5 +++++
>  target/i386/sev.c        | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target/i386/trace-events |  1 +
>  5 files changed, 58 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4468c8fe002c..4974c00c46fb 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1679,6 +1679,8 @@ static int kvm_init(MachineState *ms)
>          if (!kvm_state->memcrypt_handle) {
>              goto err;
>          }
> +
> +        kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
>      }
>  
>      ret = kvm_arch_init(ms, s);
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index 5c8c549b68ec..c16102b05ec4 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -69,5 +69,6 @@ struct SEVState {
>  typedef struct SEVState SEVState;
>  
>  void *sev_guest_init(const char *id);
> +int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
>  
>  #endif
> diff --git a/stubs/sev.c b/stubs/sev.c
> index 24c7b0c3e04d..74182bb545e2 100644
> --- a/stubs/sev.c
> +++ b/stubs/sev.c
> @@ -15,6 +15,11 @@
>  #include "qemu-common.h"
>  #include "sysemu/sev.h"
>  
> +int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
> +{
> +    return 1;
> +}
> +
>  SevState sev_get_current_state(void)
>  {
>      return SEV_STATE_UNINIT;
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6f767084fd57..04a64b5bc61d 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -90,6 +90,12 @@ fw_error_to_str(int code)
>      return sev_fw_errlist[code];
>  }
>  
> +static bool
> +sev_check_state(SevState state)
> +{
> +    return current_sev_guest_state == state ? true : false;
> +}
> +
>  static void
>  sev_set_guest_state(SevState new_state)
>  {
> @@ -466,6 +472,36 @@ sev_launch_start(SEVState *s)
>      return 0;
>  }
>  
> +static int
> +sev_launch_update_data(uint8_t *addr, uint64_t len)
> +{
> +    int ret, fw_error;
> +    struct kvm_sev_launch_update_data *update;
> +
> +    if (addr == NULL || len <= 0) {
> +        return 1;
> +    }
> +
> +    update = g_malloc0(sizeof(*update));
> +    if (!update) {
> +        return 1;
> +    }
> 
Keep checking for the g_malloc0 use - it will never return NULL;
if you want it to be safe from running out of memory use g_try_malloc0
otherwise you can just remove the !update check.
Also it's better to use the g_new0 macro (or g_try_new0) - it's neater
and avoids the whole sizeof thing.
(You have that in a bunch of the patches)

Dave

> +    update->uaddr = (__u64)addr;
> +    update->len = len;
> +    trace_kvm_sev_launch_update_data(addr, len);
> +    ret = sev_ioctl(KVM_SEV_LAUNCH_UPDATE_DATA, update, &fw_error);
> +    if (ret) {
> +        error_report("%s: LAUNCH_UPDATE ret=%d fw_error=%d '%s'",
> +                __func__, ret, fw_error, fw_error_to_str(fw_error));
> +        goto err;
> +    }
> +
> +err:
> +    g_free(update);
> +    return ret;
> +}
> +
>  void *
>  sev_guest_init(const char *id)
>  {
> @@ -540,6 +576,19 @@ err:
>      return NULL;
>  }
>  
> +int
> +sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
> +{
> +    assert(handle);
> +
> +    /* if SEV is in update state then encrypt the data else do nothing */
> +    if (sev_check_state(SEV_STATE_LUPDATE)) {
> +        return sev_launch_update_data(ptr, len);
> +    }
> +
> +    return 0;
> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 9402251e9991..c0cd8e93217f 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -12,3 +12,4 @@ kvm_memcrypt_register_region(void *addr, size_t len) "addr %p len 0x%lu"
>  kvm_memcrypt_unregister_region(void *addr, size_t len) "addr %p len 0x%lu"
>  kvm_sev_change_state(const char *old, const char *new) "%s -> %s"
>  kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session %p pdh %p"
> +kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK