On 02/01/2018 06:13 AM, Dr. David Alan Gilbert wrote:
* Brijesh Singh (brijesh.singh@xxxxxxx) 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@xxxxxxxxxx>
Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
---
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@xxxxxxxxxx / Manchester, UK
