* Brijesh Singh (brijesh.singh@xxxxxxx) wrote:
>
>
> On 2/16/18 9:47 AM, Dr. David Alan Gilbert wrote:
> > * Brijesh Singh (brijesh.singh@xxxxxxx) 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@xxxxxxxxxx>
> >> Cc: Richard Henderson <rth@xxxxxxxxxxx>
> >> Cc: Eduardo Habkost <ehabkost@xxxxxxxxxx>
> >> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> >> ---
> >> 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@xxxxxxxxxx / Manchester, UK
>
>
--
Dr. David Alan Gilbert / dgilbert@xxxxxxxxxx / Manchester, UK
