* Brijesh Singh (brijesh.singh@xxxxxxx) wrote:
>
>
> 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.
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@xxxxxxxxxx / Manchester, UK
> >
--
Dr. David Alan Gilbert / dgilbert@xxxxxxxxxx / Manchester, UK
