| Message ID | 20180129174132.108925-11-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Brijesh Singh (brijesh.singh@amd.com) wrote: > When memory encryption is enabled, KVM_SEV_INIT command is used to > initialize the platform. The command loads the SEV related persistent > data from non-volatile storage and initializes the platform context. > This command should be first issued before invoking any other guest > commands provided by the SEV firmware. Some minor comments rather than full review. > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > accel/kvm/kvm-all.c | 15 ++++++ > accel/kvm/sev.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++ > accel/kvm/trace-events | 2 + > include/sysemu/sev.h | 10 ++++ > 4 files changed, 151 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index f290f487a573..a9b16846675e 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -38,6 +38,7 @@ > #include "qemu/event_notifier.h" > #include "trace.h" > #include "hw/irq.h" > +#include "sysemu/sev.h" > > #include "hw/boards.h" > > @@ -103,6 +104,9 @@ struct KVMState > #endif > KVMMemoryListener memory_listener; > QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus; > + > + /* memory encryption */ > + void *memcrypt_handle; > }; > > KVMState *kvm_state; > @@ -1632,6 +1636,17 @@ static int kvm_init(MachineState *ms) > > kvm_state = s; > > + /* > + * if memory encryption object is specified then initialize the memory > + * encryption context. > + * */ Style: should be */ > + if (ms->memory_encryption) { > + kvm_state->memcrypt_handle = sev_guest_init(ms->memory_encryption); > + if (!kvm_state->memcrypt_handle) { > + goto err; > + } > + } > + > ret = kvm_arch_init(ms, s); > if (ret < 0) { > goto err; > diff --git a/accel/kvm/sev.c b/accel/kvm/sev.c > index e93fdfeb0c8f..be1791e510b3 100644 > --- a/accel/kvm/sev.c > +++ b/accel/kvm/sev.c > @@ -18,10 +18,72 @@ > #include "sysemu/kvm.h" > #include "sysemu/sev.h" > #include "sysemu/sysemu.h" > +#include "trace.h" > > #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ > #define DEFAULT_SEV_DEVICE "/dev/sev" > > +static int sev_fd; > + > +#define SEV_FW_MAX_ERROR 0x17 > + > +static char sev_fw_errlist[SEV_FW_MAX_ERROR][100] = { Perhaps: static const char *sev_few_errlist[] = { ? > + "", > + "Platform state is invalid", > + "Guest state is invalid", > + "Platform configuration is invalid", > + "Buffer too small", > + "Platform is already owned", > + "Certificate is invalid", > + "Policy is not allowed", > + "Guest is not active", > + "Invalid address", > + "Bad signature", > + "Bad measurement", > + "Asid is already owned", > + "Invalid ASID", > + "WBINVD is required", > + "DF_FLUSH is required", > + "Guest handle is invalid", > + "Invalid command", > + "Guest is active", > + "Hardware error", > + "Hardware unsafe", > + "Feature not supported", > + "Invalid parameter" > +}; > + > +static int > +sev_ioctl(int cmd, void *data, int *error) > +{ > + int r; > + struct kvm_sev_cmd input; > + > + memset(&input, 0x0, sizeof(input)); > + > + input.id = cmd; > + input.sev_fd = sev_fd; > + input.data = (__u64)data; > + > + r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, &input); > + > + if (error) { > + *error = input.error; > + } > + > + return r; > +} > + > +static char * > +fw_error_to_str(int code) > +{ > + if (code > SEV_FW_MAX_ERROR) { I'm trying to convince myself whether that should be >= and whether the maximum error is really 0x16 ? Your list up there has 23 entries so I think trying to access error 0x17 would be bad. > + return NULL; It might be better to return 'No error' or the like unless you're going to be testing for NULL; that way you never end up with a Null getting out. > + } > + > + return sev_fw_errlist[code]; > +} > + > static void > qsev_guest_finalize(Object *obj) > { > @@ -170,6 +232,68 @@ static const TypeInfo qsev_guest_info = { > } > }; > > +static QSevGuestInfo * > +lookup_sev_guest_info(const char *id) > +{ > + Object *obj; > + QSevGuestInfo *info; > + > + obj = object_resolve_path_component(object_get_objects_root(), id); > + if (!obj) { > + return NULL; > + } > + > + info = (QSevGuestInfo *) > + object_dynamic_cast(obj, TYPE_QSEV_GUEST_INFO); > + if (!info) { > + return NULL; > + } > + > + return info; > +} > + > +void * > +sev_guest_init(const char *id) > +{ > + SEVState *s; > + char *devname; > + int ret, fw_error; > + > + s = g_malloc0(sizeof(SEVState)); g_new0 is easier. > + if (!s) { > + return NULL; > + } and allocation aborts rather than returning NULL (unless you use the _try_ version of g_new) > + > + s->sev_info = lookup_sev_guest_info(id); > + if (!s->sev_info) { > + error_report("%s: '%s' is not a valid '%s' object", > + __func__, id, TYPE_QSEV_GUEST_INFO); > + goto err; > + } > + > + devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", NULL); > + sev_fd = open(devname, O_RDWR); > + if (sev_fd < 0) { > + error_report("%s: Failed to open %s '%s'", __func__, > + devname, strerror(errno)); > + goto err; > + } > + g_free(devname); > + > + trace_kvm_sev_init(); > + ret = sev_ioctl(KVM_SEV_INIT, NULL, &fw_error); > + if (ret) { > + error_report("%s: failed to initialize ret=%d fw_error=%d '%s'", > + __func__, ret, fw_error, fw_error_to_str(fw_error)); > + goto err; > + } > + > + return s; > +err: > + g_free(s); > + return NULL; > +} > + > static void > sev_register_types(void) > { > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events > index f89ba5578dc1..ea487e5a5913 100644 > --- a/accel/kvm/trace-events > +++ b/accel/kvm/trace-events > @@ -13,3 +13,5 @@ kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d vi > kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d" > kvm_irqchip_release_virq(int virq) "virq %d" > > +# sev.c > +kvm_sev_init(void) "" > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h > index d2621a9d1100..6aec25bc05e5 100644 > --- a/include/sysemu/sev.h > +++ b/include/sysemu/sev.h > @@ -14,6 +14,8 @@ > #ifndef QEMU_SEV_H > #define QEMU_SEV_H > > +#include <linux/kvm.h> > + > #include "qom/object.h" > #include "qapi/error.h" > #include "sysemu/kvm.h" > @@ -49,5 +51,13 @@ struct QSevGuestInfoClass { > ObjectClass parent_class; > }; > > +struct SEVState { > + QSevGuestInfo *sev_info; > +}; > + > +typedef struct SEVState SEVState; > + > +void *sev_guest_init(const char *id); > + > #endif > > -- > 2.9.5 > Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 02/01/2018 06:13 AM, Dr. David Alan Gilbert wrote: > * Brijesh Singh (brijesh.singh@amd.com) wrote: >> When memory encryption is enabled, KVM_SEV_INIT command is used to >> initialize the platform. The command loads the SEV related persistent >> data from non-volatile storage and initializes the platform context. >> This command should be first issued before invoking any other guest >> commands provided by the SEV firmware. > > Some minor comments rather than full review. > >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> accel/kvm/kvm-all.c | 15 ++++++ >> accel/kvm/sev.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++ >> accel/kvm/trace-events | 2 + >> include/sysemu/sev.h | 10 ++++ >> 4 files changed, 151 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index f290f487a573..a9b16846675e 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -38,6 +38,7 @@ >> #include "qemu/event_notifier.h" >> #include "trace.h" >> #include "hw/irq.h" >> +#include "sysemu/sev.h" >> >> #include "hw/boards.h" >> >> @@ -103,6 +104,9 @@ struct KVMState >> #endif >> KVMMemoryListener memory_listener; >> QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus; >> + >> + /* memory encryption */ >> + void *memcrypt_handle; >> }; >> >> KVMState *kvm_state; >> @@ -1632,6 +1636,17 @@ static int kvm_init(MachineState *ms) >> >> kvm_state = s; >> >> + /* >> + * if memory encryption object is specified then initialize the memory >> + * encryption context. >> + * */ > > Style: should be */ Ah noted, I will fix this. > >> + if (ms->memory_encryption) { >> + kvm_state->memcrypt_handle = sev_guest_init(ms->memory_encryption); >> + if (!kvm_state->memcrypt_handle) { >> + goto err; >> + } >> + } >> + >> ret = kvm_arch_init(ms, s); >> if (ret < 0) { >> goto err; >> diff --git a/accel/kvm/sev.c b/accel/kvm/sev.c >> index e93fdfeb0c8f..be1791e510b3 100644 >> --- a/accel/kvm/sev.c >> +++ b/accel/kvm/sev.c >> @@ -18,10 +18,72 @@ >> #include "sysemu/kvm.h" >> #include "sysemu/sev.h" >> #include "sysemu/sysemu.h" >> +#include "trace.h" >> >> #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ >> #define DEFAULT_SEV_DEVICE "/dev/sev" >> >> +static int sev_fd; >> + >> +#define SEV_FW_MAX_ERROR 0x17 >> + >> +static char sev_fw_errlist[SEV_FW_MAX_ERROR][100] = { > > Perhaps: > static const char *sev_few_errlist[] = { > ? > Sure, we can make it const, i will take care of this in next rev. >> + "", >> + "Platform state is invalid", >> + "Guest state is invalid", >> + "Platform configuration is invalid", >> + "Buffer too small", >> + "Platform is already owned", >> + "Certificate is invalid", >> + "Policy is not allowed", >> + "Guest is not active", >> + "Invalid address", >> + "Bad signature", >> + "Bad measurement", >> + "Asid is already owned", >> + "Invalid ASID", >> + "WBINVD is required", >> + "DF_FLUSH is required", >> + "Guest handle is invalid", >> + "Invalid command", >> + "Guest is active", >> + "Hardware error", >> + "Hardware unsafe", >> + "Feature not supported", >> + "Invalid parameter" >> +}; >> + >> +static int >> +sev_ioctl(int cmd, void *data, int *error) >> +{ >> + int r; >> + struct kvm_sev_cmd input; >> + >> + memset(&input, 0x0, sizeof(input)); >> + >> + input.id = cmd; >> + input.sev_fd = sev_fd; >> + input.data = (__u64)data; >> + >> + r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, &input); >> + >> + if (error) { >> + *error = input.error; >> + } >> + >> + return r; >> +} >> + >> +static char * >> +fw_error_to_str(int code) >> +{ >> + if (code > SEV_FW_MAX_ERROR) { > > I'm trying to convince myself whether that should be >= and whether the > maximum error is really 0x16 ? Your list up there has 23 entries > so I think trying to access error 0x17 would be bad. > Good catch, it should be >=. >> + return NULL; > > It might be better to return 'No error' or the like unless you're going > to be testing for NULL; that way you never end up with a Null getting > out. > Sure, I can do that. >> + } >> + >> + return sev_fw_errlist[code]; >> +} >> + >> static void >> qsev_guest_finalize(Object *obj) >> { >> @@ -170,6 +232,68 @@ static const TypeInfo qsev_guest_info = { >> } >> }; >> >> +static QSevGuestInfo * >> +lookup_sev_guest_info(const char *id) >> +{ >> + Object *obj; >> + QSevGuestInfo *info; >> + >> + obj = object_resolve_path_component(object_get_objects_root(), id); >> + if (!obj) { >> + return NULL; >> + } >> + >> + info = (QSevGuestInfo *) >> + object_dynamic_cast(obj, TYPE_QSEV_GUEST_INFO); >> + if (!info) { >> + return NULL; >> + } >> + >> + return info; >> +} >> + >> +void * >> +sev_guest_init(const char *id) >> +{ >> + SEVState *s; >> + char *devname; >> + int ret, fw_error; >> + >> + s = g_malloc0(sizeof(SEVState)); > > g_new0 is easier. Sure, I can switch to using g_new0. > >> + if (!s) { >> + return NULL; >> + } > > and allocation aborts rather than returning NULL (unless you use the > _try_ version of g_new) Agreed, I will abort on allocation failure. > >> + >> + s->sev_info = lookup_sev_guest_info(id); >> + if (!s->sev_info) { >> + error_report("%s: '%s' is not a valid '%s' object", >> + __func__, id, TYPE_QSEV_GUEST_INFO); >> + goto err; >> + } >> + >> + devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", NULL); >> + sev_fd = open(devname, O_RDWR); >> + if (sev_fd < 0) { >> + error_report("%s: Failed to open %s '%s'", __func__, >> + devname, strerror(errno)); >> + goto err; >> + } >> + g_free(devname); >> + >> + trace_kvm_sev_init(); >> + ret = sev_ioctl(KVM_SEV_INIT, NULL, &fw_error); >> + if (ret) { >> + error_report("%s: failed to initialize ret=%d fw_error=%d '%s'", >> + __func__, ret, fw_error, fw_error_to_str(fw_error)); >> + goto err; >> + } >> + >> + return s; >> +err: >> + g_free(s); >> + return NULL; >> +} >> + >> static void >> sev_register_types(void) >> { >> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events >> index f89ba5578dc1..ea487e5a5913 100644 >> --- a/accel/kvm/trace-events >> +++ b/accel/kvm/trace-events >> @@ -13,3 +13,5 @@ kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d vi >> kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d" >> kvm_irqchip_release_virq(int virq) "virq %d" >> >> +# sev.c >> +kvm_sev_init(void) "" >> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h >> index d2621a9d1100..6aec25bc05e5 100644 >> --- a/include/sysemu/sev.h >> +++ b/include/sysemu/sev.h >> @@ -14,6 +14,8 @@ >> #ifndef QEMU_SEV_H >> #define QEMU_SEV_H >> >> +#include <linux/kvm.h> >> + >> #include "qom/object.h" >> #include "qapi/error.h" >> #include "sysemu/kvm.h" >> @@ -49,5 +51,13 @@ struct QSevGuestInfoClass { >> ObjectClass parent_class; >> }; >> >> +struct SEVState { >> + QSevGuestInfo *sev_info; >> +}; >> + >> +typedef struct SEVState SEVState; >> + >> +void *sev_guest_init(const char *id); >> + >> #endif >> >> -- >> 2.9.5 >> > > Dave > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Brijesh Singh (brijesh.singh@amd.com) wrote: > > > On 02/01/2018 06:13 AM, Dr. David Alan Gilbert wrote: > > * Brijesh Singh (brijesh.singh@amd.com) wrote: > > > When memory encryption is enabled, KVM_SEV_INIT command is used to > > > initialize the platform. The command loads the SEV related persistent > > > data from non-volatile storage and initializes the platform context. > > > This command should be first issued before invoking any other guest > > > commands provided by the SEV firmware. > > > > Some minor comments rather than full review. > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > > --- > > > accel/kvm/kvm-all.c | 15 ++++++ > > > accel/kvm/sev.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > accel/kvm/trace-events | 2 + > > > include/sysemu/sev.h | 10 ++++ > > > 4 files changed, 151 insertions(+) > > > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > > index f290f487a573..a9b16846675e 100644 > > > --- a/accel/kvm/kvm-all.c > > > +++ b/accel/kvm/kvm-all.c > > > @@ -38,6 +38,7 @@ > > > #include "qemu/event_notifier.h" > > > #include "trace.h" > > > #include "hw/irq.h" > > > +#include "sysemu/sev.h" > > > #include "hw/boards.h" > > > @@ -103,6 +104,9 @@ struct KVMState > > > #endif > > > KVMMemoryListener memory_listener; > > > QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus; > > > + > > > + /* memory encryption */ > > > + void *memcrypt_handle; > > > }; > > > KVMState *kvm_state; > > > @@ -1632,6 +1636,17 @@ static int kvm_init(MachineState *ms) > > > kvm_state = s; > > > + /* > > > + * if memory encryption object is specified then initialize the memory > > > + * encryption context. > > > + * */ > > > > Style: should be */ > > > Ah noted, I will fix this. > > > > > > > + if (ms->memory_encryption) { > > > + kvm_state->memcrypt_handle = sev_guest_init(ms->memory_encryption); > > > + if (!kvm_state->memcrypt_handle) { > > > + goto err; > > > + } > > > + } > > > + > > > ret = kvm_arch_init(ms, s); > > > if (ret < 0) { > > > goto err; > > > diff --git a/accel/kvm/sev.c b/accel/kvm/sev.c > > > index e93fdfeb0c8f..be1791e510b3 100644 > > > --- a/accel/kvm/sev.c > > > +++ b/accel/kvm/sev.c > > > @@ -18,10 +18,72 @@ > > > #include "sysemu/kvm.h" > > > #include "sysemu/sev.h" > > > #include "sysemu/sysemu.h" > > > +#include "trace.h" > > > #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ > > > #define DEFAULT_SEV_DEVICE "/dev/sev" > > > +static int sev_fd; > > > + > > > +#define SEV_FW_MAX_ERROR 0x17 > > > + > > > +static char sev_fw_errlist[SEV_FW_MAX_ERROR][100] = { > > > > Perhaps: > > static const char *sev_few_errlist[] = { > > ? > > > > Sure, we can make it const, i will take care of this in next rev. > > > > > + "", > > > + "Platform state is invalid", > > > + "Guest state is invalid", > > > + "Platform configuration is invalid", > > > + "Buffer too small", > > > + "Platform is already owned", > > > + "Certificate is invalid", > > > + "Policy is not allowed", > > > + "Guest is not active", > > > + "Invalid address", > > > + "Bad signature", > > > + "Bad measurement", > > > + "Asid is already owned", > > > + "Invalid ASID", > > > + "WBINVD is required", > > > + "DF_FLUSH is required", > > > + "Guest handle is invalid", > > > + "Invalid command", > > > + "Guest is active", > > > + "Hardware error", > > > + "Hardware unsafe", > > > + "Feature not supported", > > > + "Invalid parameter" > > > +}; > > > + > > > +static int > > > +sev_ioctl(int cmd, void *data, int *error) > > > +{ > > > + int r; > > > + struct kvm_sev_cmd input; > > > + > > > + memset(&input, 0x0, sizeof(input)); > > > + > > > + input.id = cmd; > > > + input.sev_fd = sev_fd; > > > + input.data = (__u64)data; > > > + > > > + r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, &input); > > > + > > > + if (error) { > > > + *error = input.error; > > > + } > > > + > > > + return r; > > > +} > > > + > > > +static char * > > > +fw_error_to_str(int code) > > > +{ > > > + if (code > SEV_FW_MAX_ERROR) { > > > > I'm trying to convince myself whether that should be >= and whether the > > maximum error is really 0x16 ? Your list up there has 23 entries > > so I think trying to access error 0x17 would be bad. > > > > Good catch, it should be >=. > > > > > + return NULL; > > > > It might be better to return 'No error' or the like unless you're going > > to be testing for NULL; that way you never end up with a Null getting > > out. > > > > Sure, I can do that. > > > > > + } > > > + > > > + return sev_fw_errlist[code]; > > > +} > > > + > > > static void > > > qsev_guest_finalize(Object *obj) > > > { > > > @@ -170,6 +232,68 @@ static const TypeInfo qsev_guest_info = { > > > } > > > }; > > > +static QSevGuestInfo * > > > +lookup_sev_guest_info(const char *id) > > > +{ > > > + Object *obj; > > > + QSevGuestInfo *info; > > > + > > > + obj = object_resolve_path_component(object_get_objects_root(), id); > > > + if (!obj) { > > > + return NULL; > > > + } > > > + > > > + info = (QSevGuestInfo *) > > > + object_dynamic_cast(obj, TYPE_QSEV_GUEST_INFO); > > > + if (!info) { > > > + return NULL; > > > + } > > > + > > > + return info; > > > +} > > > + > > > +void * > > > +sev_guest_init(const char *id) > > > +{ > > > + SEVState *s; > > > + char *devname; > > > + int ret, fw_error; > > > + > > > + s = g_malloc0(sizeof(SEVState)); > > > > g_new0 is easier. > > > Sure, I can switch to using g_new0. > > > > > > > + if (!s) { > > > + return NULL; > > > + } > > > > and allocation aborts rather than returning NULL (unless you use the > > _try_ version of g_new) > > > Agreed, I will abort on allocation failure. No, I mean you don't need to, since g_new0 etc will abort themselves on an allocation failure; there's no need to check. Dave > > > > > > + > > > + s->sev_info = lookup_sev_guest_info(id); > > > + if (!s->sev_info) { > > > + error_report("%s: '%s' is not a valid '%s' object", > > > + __func__, id, TYPE_QSEV_GUEST_INFO); > > > + goto err; > > > + } > > > + > > > + devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", NULL); > > > + sev_fd = open(devname, O_RDWR); > > > + if (sev_fd < 0) { > > > + error_report("%s: Failed to open %s '%s'", __func__, > > > + devname, strerror(errno)); > > > + goto err; > > > + } > > > + g_free(devname); > > > + > > > + trace_kvm_sev_init(); > > > + ret = sev_ioctl(KVM_SEV_INIT, NULL, &fw_error); > > > + if (ret) { > > > + error_report("%s: failed to initialize ret=%d fw_error=%d '%s'", > > > + __func__, ret, fw_error, fw_error_to_str(fw_error)); > > > + goto err; > > > + } > > > + > > > + return s; > > > +err: > > > + g_free(s); > > > + return NULL; > > > +} > > > + > > > static void > > > sev_register_types(void) > > > { > > > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events > > > index f89ba5578dc1..ea487e5a5913 100644 > > > --- a/accel/kvm/trace-events > > > +++ b/accel/kvm/trace-events > > > @@ -13,3 +13,5 @@ kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d vi > > > kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d" > > > kvm_irqchip_release_virq(int virq) "virq %d" > > > +# sev.c > > > +kvm_sev_init(void) "" > > > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h > > > index d2621a9d1100..6aec25bc05e5 100644 > > > --- a/include/sysemu/sev.h > > > +++ b/include/sysemu/sev.h > > > @@ -14,6 +14,8 @@ > > > #ifndef QEMU_SEV_H > > > #define QEMU_SEV_H > > > +#include <linux/kvm.h> > > > + > > > #include "qom/object.h" > > > #include "qapi/error.h" > > > #include "sysemu/kvm.h" > > > @@ -49,5 +51,13 @@ struct QSevGuestInfoClass { > > > ObjectClass parent_class; > > > }; > > > +struct SEVState { > > > + QSevGuestInfo *sev_info; > > > +}; > > > + > > > +typedef struct SEVState SEVState; > > > + > > > +void *sev_guest_init(const char *id); > > > + > > > #endif > > > -- > > > 2.9.5 > > > > > > > Dave > > > > -- > > 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 f290f487a573..a9b16846675e 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -38,6 +38,7 @@ #include "qemu/event_notifier.h" #include "trace.h" #include "hw/irq.h" +#include "sysemu/sev.h" #include "hw/boards.h" @@ -103,6 +104,9 @@ struct KVMState #endif KVMMemoryListener memory_listener; QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus; + + /* memory encryption */ + void *memcrypt_handle; }; KVMState *kvm_state; @@ -1632,6 +1636,17 @@ static int kvm_init(MachineState *ms) kvm_state = s; + /* + * if memory encryption object is specified then initialize the memory + * encryption context. + * */ + if (ms->memory_encryption) { + kvm_state->memcrypt_handle = sev_guest_init(ms->memory_encryption); + if (!kvm_state->memcrypt_handle) { + goto err; + } + } + ret = kvm_arch_init(ms, s); if (ret < 0) { goto err; diff --git a/accel/kvm/sev.c b/accel/kvm/sev.c index e93fdfeb0c8f..be1791e510b3 100644 --- a/accel/kvm/sev.c +++ b/accel/kvm/sev.c @@ -18,10 +18,72 @@ #include "sysemu/kvm.h" #include "sysemu/sev.h" #include "sysemu/sysemu.h" +#include "trace.h" #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ #define DEFAULT_SEV_DEVICE "/dev/sev" +static int sev_fd; + +#define SEV_FW_MAX_ERROR 0x17 + +static char sev_fw_errlist[SEV_FW_MAX_ERROR][100] = { + "", + "Platform state is invalid", + "Guest state is invalid", + "Platform configuration is invalid", + "Buffer too small", + "Platform is already owned", + "Certificate is invalid", + "Policy is not allowed", + "Guest is not active", + "Invalid address", + "Bad signature", + "Bad measurement", + "Asid is already owned", + "Invalid ASID", + "WBINVD is required", + "DF_FLUSH is required", + "Guest handle is invalid", + "Invalid command", + "Guest is active", + "Hardware error", + "Hardware unsafe", + "Feature not supported", + "Invalid parameter" +}; + +static int +sev_ioctl(int cmd, void *data, int *error) +{ + int r; + struct kvm_sev_cmd input; + + memset(&input, 0x0, sizeof(input)); + + input.id = cmd; + input.sev_fd = sev_fd; + input.data = (__u64)data; + + r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_OP, &input); + + if (error) { + *error = input.error; + } + + return r; +} + +static char * +fw_error_to_str(int code) +{ + if (code > SEV_FW_MAX_ERROR) { + return NULL; + } + + return sev_fw_errlist[code]; +} + static void qsev_guest_finalize(Object *obj) { @@ -170,6 +232,68 @@ static const TypeInfo qsev_guest_info = { } }; +static QSevGuestInfo * +lookup_sev_guest_info(const char *id) +{ + Object *obj; + QSevGuestInfo *info; + + obj = object_resolve_path_component(object_get_objects_root(), id); + if (!obj) { + return NULL; + } + + info = (QSevGuestInfo *) + object_dynamic_cast(obj, TYPE_QSEV_GUEST_INFO); + if (!info) { + return NULL; + } + + return info; +} + +void * +sev_guest_init(const char *id) +{ + SEVState *s; + char *devname; + int ret, fw_error; + + s = g_malloc0(sizeof(SEVState)); + if (!s) { + return NULL; + } + + s->sev_info = lookup_sev_guest_info(id); + if (!s->sev_info) { + error_report("%s: '%s' is not a valid '%s' object", + __func__, id, TYPE_QSEV_GUEST_INFO); + goto err; + } + + devname = object_property_get_str(OBJECT(s->sev_info), "sev-device", NULL); + sev_fd = open(devname, O_RDWR); + if (sev_fd < 0) { + error_report("%s: Failed to open %s '%s'", __func__, + devname, strerror(errno)); + goto err; + } + g_free(devname); + + trace_kvm_sev_init(); + ret = sev_ioctl(KVM_SEV_INIT, NULL, &fw_error); + if (ret) { + error_report("%s: failed to initialize ret=%d fw_error=%d '%s'", + __func__, ret, fw_error, fw_error_to_str(fw_error)); + goto err; + } + + return s; +err: + g_free(s); + return NULL; +} + static void sev_register_types(void) { diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index f89ba5578dc1..ea487e5a5913 100644 --- a/accel/kvm/trace-events +++ b/accel/kvm/trace-events @@ -13,3 +13,5 @@ kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d vi kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d" kvm_irqchip_release_virq(int virq) "virq %d" +# sev.c +kvm_sev_init(void) "" diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h index d2621a9d1100..6aec25bc05e5 100644 --- a/include/sysemu/sev.h +++ b/include/sysemu/sev.h @@ -14,6 +14,8 @@ #ifndef QEMU_SEV_H #define QEMU_SEV_H +#include <linux/kvm.h> + #include "qom/object.h" #include "qapi/error.h" #include "sysemu/kvm.h" @@ -49,5 +51,13 @@ struct QSevGuestInfoClass { ObjectClass parent_class; }; +struct SEVState { + QSevGuestInfo *sev_info; +}; + +typedef struct SEVState SEVState; + +void *sev_guest_init(const char *id); + #endif
When memory encryption is enabled, KVM_SEV_INIT command is used to initialize the platform. The command loads the SEV related persistent data from non-volatile storage and initializes the platform context. This command should be first issued before invoking any other guest commands provided by the SEV firmware. Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- accel/kvm/kvm-all.c | 15 ++++++ accel/kvm/sev.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++ accel/kvm/trace-events | 2 + include/sysemu/sev.h | 10 ++++ 4 files changed, 151 insertions(+)