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

[Eduardo Habkost <ehabkost@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:
> 

Are those config options documented somewhere?

> [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"

Why a separate config file loading mechanism? If the user really
needs to load the SEV configuration data from a separate file,
you can just use regular config sections and use -readconfig.

Now, about the format of the new config sections ("sev" and
"sev-launch"): I am not sure adding new command-line options and
config sections is necessary. Is it possible to implement it as a
combination of:
* new options to existing command-line options and/or
* new options to existing objects and/or
* new options to existing devices and/or
* new types for -object? (see how crypto secrets and net filters
  are configured, for an example)


[...]
>  extern bool kvm_allowed;
> +extern bool kvm_sev_allowed;

Can we place this inside struct KVMState?

>  extern bool kvm_kernel_irqchip;
>  extern bool kvm_split_irqchip;
>  extern bool kvm_async_interrupts_allowed;
[...]
> @@ -1745,6 +1747,10 @@ static int kvm_init(MachineState *ms)
>  
>      kvm_state = s;
>  
> +    if (!sev_init(kvm_state)) {
> +        kvm_sev_allowed = true;
> +    }

sev_init() errors are being ignored here. sev_init() could report
errors properly using Error** (and kvm_init() should not ignore
them).

> +
>      if (kvm_eventfds_allowed) {
>          s->memory_listener.listener.eventfd_add = kvm_mem_ioeventfd_add;
>          s->memory_listener.listener.eventfd_del = kvm_mem_ioeventfd_del;
[...]
> +
> +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;

Can we place this pointer inside struct KVMState?

[...]
> +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)

Function name confused me (it seemed to read only one u8 value).

> +{
> +    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]);

Function doesn't check for array length.

> +        DPRINTF("%02hhx", val[i]);
> +        v += 2;
> +        i++;
> +    }
> +    DPRINTF("\n");
> +
> +    return i;

Do we really need to write our own parser? I wonder if we can
reuse crypto/secret.c for loading the keys.

> +}
> +
[...]

-- 
Eduardo