Re: [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:
> 
> 
> On 2/16/18 9:47 AM, Dr. David Alan Gilbert wrote:
> > * 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)
> 
> I didn't realized that g_malloc0() will never return NULL. I checked
> just glib doc,

Yes, all of the normal glib allocators assert on failure to allocate
rather than returning NULL.   Our normal advice is to use
g_new0/g_malloc0 for small items and you don't need to check it; but for
anything large use the g_try_* variants, they do return NULL on a
failure.

> if v10 is needed then I can remove them all or can submit
> a follow-up patch.

If there are no other changes needed then it's fine as a followup;
if you need to reroll anyway then lets tidy them all up.

Dave

> 
> thanks
> 
> 
> > 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
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK