| Message ID | 20180215153955.3253-17-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
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, if v10 is needed then I can remove them all or can submit a follow-up patch. 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
* 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
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; + } + + 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
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(+)