Re: [Qemu-devel] [RFC PATCH v1 06/22] sev: add initial SEV support

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Sep 13, 2016 at 10:47:47AM -0400, Brijesh Singh wrote:
> This patch adds the initial support required to integrate Secure
> Encrypted Virtualization feature, the patch include the following
> changes:
> 
> - adds sev.c and sev.h files: the file will contain SEV APIs implemention.
> - add kvm_sev_enabled(): similar to kvm_enabled() this function can be
>   used to check if sev is enabled on this guest.
> - implement functions to parse SEV specific configuration file.
> 
> A typical SEV config file looks like this:
> 
> [sev-launch]
> 	flags = "00000000"
> 	policy	= "000000"
> 	dh_pub_qx = "0123456789abcdef0123456789abcdef"
> 	dh_pub_qy = "0123456789abcdef0123456789abcdef"
> 	nonce = "0123456789abcdef"
> 	vcpu_count = "1"
> 	vcpu_length = "30"
> 	vcpu_mask = "00ab"

We do not want to be inventing new config options for this - we should
be using QOM via -object to create this.



> diff --git a/sev.c b/sev.c
> new file mode 100644
> index 0000000..2d71ca6
> --- /dev/null
> +++ b/sev.c
> @@ -0,0 +1,282 @@
> +/*
> + * QEMU SEV support
> + *
> + * Copyright Advanced Micro Devices 2016
> + *
> + * Author:
> + *      Brijesh Singh <brijesh.singh@amd.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +
> +#include <linux/kvm.h>
> +
> +#include "qemu-common.h"
> +#include "qemu/atomic.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +#include "hw/hw.h"
> +#include "hw/pci/msi.h"
> +#include "hw/s390x/adapter.h"
> +#include "exec/gdbstub.h"
> +#include "sysemu/kvm_int.h"
> +#include "sysemu/sev.h"
> +#include "qemu/bswap.h"
> +#include "exec/memory.h"
> +#include "exec/ram_addr.h"
> +#include "exec/address-spaces.h"
> +#include "qemu/event_notifier.h"
> +#include "trace.h"
> +#include "hw/irq.h"
> +
> +//#define DEBUG_SEV
> +
> +#ifdef DEBUG_SEV
> +#define DPRINTF(fmt, ...) \
> +    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +struct SEVInfo {
> +    uint8_t state;  /* guest current state */
> +    uint8_t type;   /* guest type (encrypted, unencrypted) */
> +    struct kvm_sev_launch_start *launch_start;
> +    struct kvm_sev_launch_update *launch_update;
> +    struct kvm_sev_launch_finish *launch_finish;
> +};
> +
> +typedef struct SEVInfo SEVInfo;
> +static SEVInfo *sev_info;
> +static const char *cfg_file;
> +
> +enum {
> +    LAUNCH_OPTS = 0,
> +};
> +
> +enum {
> +    PRE_ENCRYPTED_GUEST = 0,
> +    UNENCRYPTED_GUEST,
> +};
> +
> +static QemuOptsList launch_opts = {
> +    .name = "sev-launch",
> +    .head = QTAILQ_HEAD_INITIALIZER(launch_opts.head),
> +    .desc = {
> +        {
> +            .name = "flags",
> +            .type = QEMU_OPT_NUMBER,
> +        },
> +        {
> +            .name = "policy",
> +            .type = QEMU_OPT_NUMBER,
> +        },
> +        {
> +            .name = "dh_pub_qx",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "dh_pub_qy",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "nonce",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "vcpu_length",
> +            .type = QEMU_OPT_NUMBER,
> +        },
> +        {
> +            .name = "vcpu_count",
> +            .type = QEMU_OPT_NUMBER,
> +        },
> +        {
> +            .name = "vcpu_mask",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};



> +
> +static QemuOptsList *config_groups[] = {
> +    &launch_opts,
> +    NULL
> +};
> +
> +struct add_rule_data {
> +    SEVInfo *s;
> +    int action;
> +};
> +
> +static unsigned int read_config_u32(QemuOpts *opts, const char *key)
> +{
> +    unsigned int val;
> +
> +    val = qemu_opt_get_number_del(opts, key, -1);
> +    DPRINTF(" %s = %08x\n", key, val);
> +
> +    return val;
> +}
> +
> +static int read_config_u8(QemuOpts *opts, const char *key, uint8_t *val)
> +{
> +    int i;
> +    const char *v;
> +
> +    v = qemu_opt_get(opts, key);
> +    if (!v) {
> +        return 0;
> +    }
> +
> +    DPRINTF(" %s = ", key);
> +    i = 0;
> +    while (*v) {
> +        sscanf(v, "%2hhx", &val[i]);
> +        DPRINTF("%02hhx", val[i]);
> +        v += 2;
> +        i++;
> +    }
> +    DPRINTF("\n");
> +
> +    return i;
> +}
> +
> +static int add_rule(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    struct add_rule_data *d = opaque;
> +
> +    switch (d->action) {
> +        case LAUNCH_OPTS: {
> +            struct kvm_sev_launch_start *start;
> +            struct kvm_sev_launch_update *update;
> +            struct kvm_sev_launch_finish *finish;
> +
> +            /* LAUNCH_START parameters */
> +            start = g_malloc0(sizeof(*start));
> +
> +            DPRINTF("Parsing 'sev-launch' parameters\n");
> +            start->flags = read_config_u32(opts, "flags");
> +            start->policy = read_config_u32(opts, "policy");
> +            read_config_u8(opts, "nonce", start->nonce);
> +            read_config_u8(opts, "dh_pub_qx", start->dh_pub_qx);
> +            read_config_u8(opts, "dh_pub_qy", start->dh_pub_qy);
> +            sev_info->launch_start = start;
> +
> +            /* LAUNCH_UPDATE */
> +            update = g_malloc0(sizeof(*update));
> +            sev_info->launch_update = update;
> +
> +            /* LAUNCH_FINISH parameters */
> +            finish = g_malloc0(sizeof(*finish));
> +
> +            finish->vcpu_count = read_config_u32(opts, "vcpu_count");
> +            finish->vcpu_length = read_config_u32(opts, "vcpu_length");
> +            if (qemu_opt_get(opts, "vcpu_mask")) {
> +                finish->vcpu_mask_length =
> +                                    strlen(qemu_opt_get(opts, "vcpu_mask")) / 2;
> +                finish->vcpu_mask_addr = (unsigned long)
> +                                        g_malloc0(finish->vcpu_length);
> +                read_config_u8(opts, "vcpu_mask",
> +                        (uint8_t *)finish->vcpu_mask_addr);
> +            }
> +
> +            sev_info->launch_finish = finish;
> +
> +            break;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static int parse_add_rules(QemuOptsList *list, struct add_rule_data *d)
> +{
> +    Error *local_err = NULL;
> +
> +    qemu_opts_foreach(list, add_rule, d, &local_err);
> +    if (local_err) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int parse_sev_cfg(SEVInfo *s, int type, const char *filename)
> +{
> +    FILE *f;
> +    int ret = 0;
> +    struct add_rule_data d;
> +
> +    if (filename) {
> +        f = fopen(filename, "r");
> +        if (f == NULL) {
> +            return 1;
> +        }
> +
> +        ret = qemu_config_parse(f, config_groups, filename);
> +        if (ret < 0) {
> +            fprintf(stderr, "SEV: could not parse config file\n");
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    switch (type) {
> +    case LAUNCH_OPTS:
> +        d.s = s;
> +        d.action = type;
> +        ret = parse_add_rules(&launch_opts, &d);
> +        break;
> +    }
> +
> +    return ret;
> +
> +}
> +
> +int sev_init(KVMState *kvm_state)
> +{
> +    QemuOpts *opts;
> +    const char *type;
> +
> +    opts = qemu_find_opts_singleton("sev");
> +    cfg_file = qemu_opt_get(opts, "config");
> +    if (!cfg_file) {
> +        return 1;
> +    }
> +
> +    type = qemu_opt_get(opts, "type");
> +    if (!type) {
> +        return 1;
> +    }
> +
> +    sev_info = calloc(1, sizeof(*sev_info));
> +    if (!sev_info) {
> +        return 1;
> +    }

Use 'g_new0' for allocation and dont check the return
value since g_new0 aborts on oom.

> +
> +    if (!strcmp(type, "unencrypted")) {
> +        sev_info->type = UNENCRYPTED_GUEST;
> +    } else if (!strcmp(type, "encrypted")) {
> +        sev_info->type = PRE_ENCRYPTED_GUEST;

You should define an enum in qapi-schema.json for these
values, and use an enum property for 'type' in the QOM
object you then create. This automatically then handles
the string <-> int conversion

> +    } else {
> +        fprintf(stderr, "SEV: unsupported type '%s'\n", type);
> +        goto err;
> +    }
> +
> +    /* call SEV launch start APIs based on guest type */
> +
> +    return 0;
> +err:
> +    free(sev_info);
> +    sev_info = NULL;
> +    return 1;
> +}

All the command line argument handling in this file should be removed.
Instead you should define a user creatable object type to represent
the properties you need to have configured. If you want a simple
example of how to define a QOM object type, take a look at the code
in crypto/secret.c


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|