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

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Eduardo,

On 09/13/2016 10:58 AM, Eduardo Habkost wrote:
>>
>> A typical SEV config file looks like this:
>>
>
> Are those config options documented somewhere?
>

Various commands and parameters are documented [1]

[1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf

>> [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.
>
I will look into -readconfig and see if we can use that.

> 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)
>
>

All these config parameters should be provided by the guest owner before 
launching or migrating a guest. I believe we need to make changes in 
libvirt, virsh and other upper layer software stack to communicate with 
guest owner to get these input parameters. For development purposes I 
choose a simple config file to get these parameters. I am not sure if we 
will able to "add new option to a existing objects/device" but we can 
look into creating a "new type for -object" or we can simply accept a fd 
from upper layer and read the fd to get these parameters.

> [...]
>>  extern bool kvm_allowed;
>> +extern bool kvm_sev_allowed;
>
> Can we place this inside struct KVMState?
>
Yes, i will add this in v2.
>>  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).
Thanks, will fix in v2.
>
>> +
>>      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?
>
Will try to move into 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).
>
I will fix the name, the function converts string into a hex bytes 
array. e.g "12aabbccdd" is converted to val[] = {0x12,0xaa,0xbb,0xcc, 0xdd}.

>> +{
>> +    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.
Thanks, i will fix this.
>
>> +        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.
>
I just looked at crypto/secret.c for loading the keys but not sure if 
will able to reuse the secret_load routines, this is mainly because the 
SEV inputs parameters are different compare to what we have in 
crypto/secrets.c. I will still look more closely and see if we can find 
some common code.
>> +}
>> +
> [...]
>