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.
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}.[...]+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.
Thanks, i will fix this.
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.+ 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.
+} +[...]
