diff mbox

[v6,10/23] sev: add command to initialize the memory encryption context

Message ID 20180129174132.108925-11-brijesh.singh@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Brijesh Singh Jan. 29, 2018, 5:41 p.m. UTC
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(+)

Comments

Dr. David Alan Gilbert Feb. 1, 2018, 12:13 p.m. UTC | #1
* 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
Brijesh Singh Feb. 1, 2018, 3:33 p.m. UTC | #2
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
>
Dr. David Alan Gilbert Feb. 1, 2018, 3:46 p.m. UTC | #3
* 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 mbox

Patch

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