| Message ID | 147377806784.11859.11149856529336910514.stgit@brijesh-build-machine (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +} > + [...]
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. >> +} >> + > [...] >
On Tue, Sep 13, 2016 at 02:54:40PM -0500, Brijesh Singh wrote: > 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 Could you write a TL;DR summary? IMHO it's not reasonable to ask that people read spec documents in order to understand QEMU command line parameters or config flags. Some text should also go into docs/ > > > [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. > > > +} > > > + > > [...] > >
(CCing Daniel Berrange in case he has feedback on the nonce/dh_pub_qx/dh_pub_qy loading/parsing at the end of this message) On Tue, Sep 13, 2016 at 02:54:40PM -0500, Brijesh Singh wrote: > 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 If I understand correctly, the docs describe the firmware interface. The interface provided by QEMU is not the same thing, and needs to be documented as well (even if it contains pointers to sections or tables in the firmware interface docs). Some of the questions I have about the fields are: * Do we really need the user to provide all the options below? * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask, for example? * Is bit 0 (KS) the only bit that can be set on flags? If so, why not a boolean "ks" option? * Is "policy" the guest policy structure described at page 23? If so, why exposing the raw value instead of separate fields for each bit/field in the structure? (and only for the ones that are supposed to be set by the user) * If vcpu_mask is a bitmap for each VCPU, should we represent it as a list of VCPU indexes? A good way to model this data and document it more properly is through a QAPI schema. grep for "opts_visitor_new()" in the code for examples where QEMU options are parsed according to a QAPI schema. The downside is that using a QAPI visitor is (AFAIK) not possible if using -object like I suggest below. > > > > [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. I was thinking of something like: -object sev-launch-rule,flags=0,policy=0,dh_pub_qx=XXXXX,dh_pub_qy=YYYYY,nonce=ZZZZ,vcpu_count=1,vcpu_length=30,vcpu_mask=00ab \ -machine pc,accel=kvm,sev=on # see note below[1] With this, you won't need separate code for command-line, config files, and QMP commands. They will all be able to use the same mechanisms. ...but this conflicts with the idea of using QAPI. So I am not sure which way to go. (But either way we go, we need a clearer and better documented set of parameters). [1] I would go even further and separate the accel object options from the machine options, but this would require reworking the accelerator configuration inside QEMU. e.g.: -object kvm-accel,sev=on,id=kvm0 -machine pc,accel=kvm0 [...] > > > +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. There are other parameters, sure, but maybe it would be appropriate to just load nonce/dh_pub_qx/dh_pub_qy as TTYPE_QCRYPTO_SECRET object(s) (-object secret,...)? I am not sure because I don't understand the crypto part fully. Daniel, what do you think?
On Tue, Sep 13, 2016 at 07:00:44PM -0300, Eduardo Habkost wrote: > (CCing Daniel Berrange in case he has feedback on the > nonce/dh_pub_qx/dh_pub_qy loading/parsing at the end of this > message) > > On Tue, Sep 13, 2016 at 02:54:40PM -0500, Brijesh Singh wrote: > > 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 > > If I understand correctly, the docs describe the firmware > interface. The interface provided by QEMU is not the same thing, > and needs to be documented as well (even if it contains pointers > to sections or tables in the firmware interface docs). > > Some of the questions I have about the fields are: > * Do we really need the user to provide all the options below? > * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask, > for example? > * Is bit 0 (KS) the only bit that can be set on flags? If so, why > not a boolean "ks" option? > * Is "policy" the guest policy structure described at page 23? If > so, why exposing the raw value instead of separate fields for > each bit/field in the structure? (and only for the ones that > are supposed to be set by the user) > * If vcpu_mask is a bitmap for each VCPU, should we represent it > as a list of VCPU indexes? > > A good way to model this data and document it more properly is > through a QAPI schema. grep for "opts_visitor_new()" in the code > for examples where QEMU options are parsed according to a QAPI > schema. The downside is that using a QAPI visitor is (AFAIK) not > possible if using -object like I suggest below. It needs to use QOM really, not QAPI, since it has to be user creatable on the CLI and we don't want to invent new command line arguments. > > > > +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. > > There are other parameters, sure, but maybe it would be > appropriate to just load nonce/dh_pub_qx/dh_pub_qy as > TTYPE_QCRYPTO_SECRET object(s) (-object secret,...)? I am not > sure because I don't understand the crypto part fully. The secrets object is used for information that has to be kept private from eavesdroppers. Based on the param names here 'dh_pub_qx' it sounds like this is non-sensitive "public" data, so would not need to use the secrets object, but it is hard to say for sure without close look at the technical details. Regards, Daniel
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
On Wed, Sep 14, 2016 at 09:30:51AM +0100, Daniel P. Berrange wrote: > On Tue, Sep 13, 2016 at 07:00:44PM -0300, Eduardo Habkost wrote: > > (CCing Daniel Berrange in case he has feedback on the > > nonce/dh_pub_qx/dh_pub_qy loading/parsing at the end of this > > message) > > > > On Tue, Sep 13, 2016 at 02:54:40PM -0500, Brijesh Singh wrote: > > > 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 > > > > If I understand correctly, the docs describe the firmware > > interface. The interface provided by QEMU is not the same thing, > > and needs to be documented as well (even if it contains pointers > > to sections or tables in the firmware interface docs). > > > > Some of the questions I have about the fields are: > > * Do we really need the user to provide all the options below? > > * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask, > > for example? > > * Is bit 0 (KS) the only bit that can be set on flags? If so, why > > not a boolean "ks" option? > > * Is "policy" the guest policy structure described at page 23? If > > so, why exposing the raw value instead of separate fields for > > each bit/field in the structure? (and only for the ones that > > are supposed to be set by the user) > > * If vcpu_mask is a bitmap for each VCPU, should we represent it > > as a list of VCPU indexes? > > > > A good way to model this data and document it more properly is > > through a QAPI schema. grep for "opts_visitor_new()" in the code > > for examples where QEMU options are parsed according to a QAPI > > schema. The downside is that using a QAPI visitor is (AFAIK) not > > possible if using -object like I suggest below. > > It needs to use QOM really, not QAPI, since it has to be user > creatable on the CLI and we don't want to invent new command > line arguments. As much as I don't like not being able to use the QAPI schema to document -object, this is true. [...] > > > > > > > > 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. > > > > There are other parameters, sure, but maybe it would be > > appropriate to just load nonce/dh_pub_qx/dh_pub_qy as > > TTYPE_QCRYPTO_SECRET object(s) (-object secret,...)? I am not > > sure because I don't understand the crypto part fully. > > The secrets object is used for information that has to be kept > private from eavesdroppers. Based on the param names here > 'dh_pub_qx' it sounds like this is non-sensitive "public" > data, so would not need to use the secrets object, but it > is hard to say for sure without close look at the technical > details. They are a public key and nonce for ECDH key agreement, so they are public (AFAICS). So let's forget about "-object secret". I just want to ensure we either reuse existing parsing code, or put the new parser in common code that can be reused.
On Wed, Sep 14, 2016 at 08:54:12AM -0300, Eduardo Habkost wrote: > On Wed, Sep 14, 2016 at 09:30:51AM +0100, Daniel P. Berrange wrote: > > On Tue, Sep 13, 2016 at 07:00:44PM -0300, Eduardo Habkost wrote: > > > (CCing Daniel Berrange in case he has feedback on the > > > nonce/dh_pub_qx/dh_pub_qy loading/parsing at the end of this > > > message) > > > > > > On Tue, Sep 13, 2016 at 02:54:40PM -0500, Brijesh Singh wrote: > > > > 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 > > > > > > If I understand correctly, the docs describe the firmware > > > interface. The interface provided by QEMU is not the same thing, > > > and needs to be documented as well (even if it contains pointers > > > to sections or tables in the firmware interface docs). > > > > > > Some of the questions I have about the fields are: > > > * Do we really need the user to provide all the options below? > > > * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask, > > > for example? > > > * Is bit 0 (KS) the only bit that can be set on flags? If so, why > > > not a boolean "ks" option? > > > * Is "policy" the guest policy structure described at page 23? If > > > so, why exposing the raw value instead of separate fields for > > > each bit/field in the structure? (and only for the ones that > > > are supposed to be set by the user) > > > * If vcpu_mask is a bitmap for each VCPU, should we represent it > > > as a list of VCPU indexes? > > > > > > A good way to model this data and document it more properly is > > > through a QAPI schema. grep for "opts_visitor_new()" in the code > > > for examples where QEMU options are parsed according to a QAPI > > > schema. The downside is that using a QAPI visitor is (AFAIK) not > > > possible if using -object like I suggest below. > > > > It needs to use QOM really, not QAPI, since it has to be user > > creatable on the CLI and we don't want to invent new command > > line arguments. > > As much as I don't like not being able to use the QAPI schema to > document -object, this is true. FWIW, in the medium-long term there is clear scope for adding a 'object' type to the QAPI schema, that could be used to generate the boilerplate code for QOM, so we can reconcile these eventually. Regards, Daniel
>> Various commands and parameters are documented [1] >> >> [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf > > If I understand correctly, the docs describe the firmware > interface. The interface provided by QEMU is not the same thing, > and needs to be documented as well (even if it contains pointers > to sections or tables in the firmware interface docs). > > Some of the questions I have about the fields are: > * Do we really need the user to provide all the options below? > * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask, > for example? Good question, I don't think we need to get this information from guest owner and it can be calculated from KVM. I will check with security folks on how this information is used in measurement generation and make the changes accordingly. > * Is bit 0 (KS) the only bit that can be set on flags? If so, why > not a boolean "ks" option? Agreed. I will fix in v2. > * Is "policy" the guest policy structure described at page 23? If > so, why exposing the raw value instead of separate fields for > each bit/field in the structure? (and only for the ones that > are supposed to be set by the user) Yes policy is described in chapter 3, page 23. I am open to separate the fields. Let me know if something like this works sev-launch-rule,flags.ks=0,policy.dbg=0,policy.ks=0,policy.nosend=0,... > * If vcpu_mask is a bitmap for each VCPU, should we represent it > as a list of VCPU indexes? > I will check on this one. > A good way to model this data and document it more properly is > through a QAPI schema. grep for "opts_visitor_new()" in the code > for examples where QEMU options are parsed according to a QAPI > schema. The downside is that using a QAPI visitor is (AFAIK) not > possible if using -object like I suggest below. > >> >>>> [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" >>> >> >> 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. > > I was thinking of something like: > > -object sev-launch-rule,flags=0,policy=0,dh_pub_qx=XXXXX,dh_pub_qy=YYYYY,nonce=ZZZZ,vcpu_count=1,vcpu_length=30,vcpu_mask=00ab \ > -machine pc,accel=kvm,sev=on # see note below[1] > > With this, you won't need separate code for command-line, config > files, and QMP commands. They will all be able to use the same > mechanisms. > > ...but this conflicts with the idea of using QAPI. So I am not > sure which way to go. (But either way we go, we need a clearer > and better documented set of parameters). > I am open to idea and need direction on which way to go. I will work on documenting the parameters and usages. Should I consider implementing your below approach in v2 ? -object sev-launch-rule,flags=0,policy=0,dh_pub_qx=XXXXX,dh_pub_qy=YYYYY,nonce=ZZZZ,vcpu_count=1,vcpu_length=30,vcpu_mask=00ab \ -machine pc,accel=kvm,sev=on Any tips on which qemu file i can use as reference during implementation. Thanks. > [1] I would go even further and separate the accel object options > from the machine options, but this would require reworking > the accelerator configuration inside QEMU. e.g.: > > -object kvm-accel,sev=on,id=kvm0 > -machine pc,accel=kvm0 > > [...] >>> 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. > > There are other parameters, sure, but maybe it would be > appropriate to just load nonce/dh_pub_qx/dh_pub_qy as > TTYPE_QCRYPTO_SECRET object(s) (-object secret,...)? I am not > sure because I don't understand the crypto part fully. > > Daniel, what do you think? >
On Wed, Sep 14, 2016 at 11:10:54AM -0500, Brijesh Singh wrote: > > I am open to idea and need direction on which way to go. I will work on > documenting the parameters and usages. Should I consider implementing your > below approach in v2 ? > > -object sev-launch-rule,flags=0,policy=0,dh_pub_qx=XXXXX,dh_pub_qy=YYYYY,nonce=ZZZZ,vcpu_count=1,vcpu_length=30,vcpu_mask=00ab > \ -machine pc,accel=kvm,sev=on > > Any tips on which qemu file i can use as reference during implementation. Take a look at crypto/secret.c for an example of how to create a QOM object that can be loaded with '-object'. There are more examples in backends/{hostmem,rng}*.c too. Regards, Daniel
On Wed, Sep 14, 2016 at 11:10:54AM -0500, Brijesh Singh wrote: > > > > Various commands and parameters are documented [1] > > > > > > [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf > > > > If I understand correctly, the docs describe the firmware > > interface. The interface provided by QEMU is not the same thing, > > and needs to be documented as well (even if it contains pointers > > to sections or tables in the firmware interface docs). > > > > Some of the questions I have about the fields are: > > * Do we really need the user to provide all the options below? > > * Can't QEMU or KVM calculate vcpu_count/vcpu_length/vcpu_mask, > > for example? > > Good question, I don't think we need to get this information from guest > owner and it can be calculated from KVM. I will check with security folks on > how this information is used in measurement generation and make the changes > accordingly. > > > * Is bit 0 (KS) the only bit that can be set on flags? If so, why > > not a boolean "ks" option? > Agreed. I will fix in v2. > > > * Is "policy" the guest policy structure described at page 23? If > > so, why exposing the raw value instead of separate fields for > > each bit/field in the structure? (and only for the ones that > > are supposed to be set by the user) > > Yes policy is described in chapter 3, page 23. I am open to separate the > fields. > Let me know if something like this works > > sev-launch-rule,flags.ks=0,policy.dbg=0,policy.ks=0,policy.nosend=0,... My question is, does all of it have to be sev specific? For example, add a generic flag to block debug commands from monitor. When blocked, and if sev happens to be enabled, you can run guest with debug disabled.
Hi Michael, >> >> Yes policy is described in chapter 3, page 23. I am open to separate the >> fields. >> Let me know if something like this works >> >> sev-launch-rule,flags.ks=0,policy.dbg=0,policy.ks=0,policy.nosend=0,... > > My question is, does all of it have to be sev specific? > For example, add a generic flag to block debug commands from monitor. > When blocked, and if sev happens to be enabled, you can run guest with > debug disabled. > > All the sev option should be specified during guest creation. A typical SEV flow look like this: 1) Guest owner requests cloud provider to launch a SEV quest. 2) Cloud provides requests the guest owner to provide the SEV launch parameter. These parameters includes: flags, policy, nonce, public diffie-hellman keys. 3) cloud provider launches qemu with '-sev' option. The code path looks like this: kvm_sev_guest_start() [qemu] kvm_vm_ioctl(SEV_ISSUE_CMD) [kvm] asid = allocates a new SEV aid. [kvm] psp_issue_cmd(LAUNCH_START) [kvm] // the will call the firmware to create // new cryptographic context psp_issue_cmd(ACTIVATE, asid) [kvm] // the command will bind asid to this crypto // context and loads the encryption key into // memory controller 4) As part of guest creation qemu loader copies data/code into guest memory (e.g BIOS). We need to encrypt the data/code copied into guest memory using LAUNCH_UPDATE command. sev_launch_update() kvm_vm_ioctl(SEV_ISSUE_CMD) psp_issue_cmd(LAUNCH_UPDATE, buffer) // the command will encrypt in-place 5) After qemu is done copying data into guest memory we call LAUNCH_FINISH, this command generate a measurement (basically hash of the data encrypted through LAUNCH_UPDATE). 6) Cloud provider sends the measurement to guest owner. 7) Guest owner validates the measurement. If measurement matches then we are good to launch the guest. This should ensure that bootcode was not compromised by hypervisor. Once the guest is running then memory content of VM is transparently encrypted with key loaded into memory controller. SEV guest have the concept of private and shared memory. Private memory is encrypted with the guest-specific keys, while share memory may be encrypted with hypervisor key. Certain type of memory (namely instruction pages and guest page tables) are always considered as private. The guest OS can mark individual pages of memory as encrypted using the standard x86 page table. I call it C-bit in PTE, if C-bit (bit 47) is set then page is private and if C-bit is 0 then page is shared. A page that is marked private will be automatically decrypted when read from the DRAM and encrypted when written to DRAM. Note that since C-bit is only controllable by the guest OS when it in operating in 64-bit or 32-bit PAE mode, in all other modes the SEV hardware forces the C-bit to a 1. One of field in launch_start struct is 'policy', if policy allows the debugging then hypervisor can use SEV commands (kvm_sev_debug_decrypt/kvm_sev_debug_encrypt) to read and write into guest memory. If policy does not allow the debugging then any read of guest memory from hypervisor will get encrypted data and similarly writes to guest memory from hypervisor will result guest reading the garbage.
On Wed, Sep 14, 2016 at 01:46:09PM -0500, Brijesh Singh wrote: > 7) Guest owner validates the measurement. If measurement matches then we are > good to launch the guest. This should ensure that bootcode was not > compromised by hypervisor. As hypervisor can e.g. execute said code in any order (without touching protected memory) this seems rather like adding asserts in code at random points. Frankly if one is so worried about the boot sequence, just send an already booted guest to the cloud provider. But anyway, that's beside the point. My point is that all this measurement dance is orthogonal to memory encryption. It happens to be part of the same AMD CPU, but it might not be on other CPUs, and I don't see why should command line/QOM APIs tie us to what AMD did.
diff --git a/Makefile.target b/Makefile.target index a440bcb..74ad204 100644 --- a/Makefile.target +++ b/Makefile.target @@ -136,7 +136,7 @@ ifdef CONFIG_SOFTMMU obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o obj-y += qtest.o bootdevice.o obj-y += hw/ -obj-$(CONFIG_KVM) += kvm-all.o +obj-$(CONFIG_KVM) += kvm-all.o sev.o obj-y += memory.o cputlb.o obj-y += memory_mapping.o obj-y += dump.o diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index c9c2436..7f83de0 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -40,6 +40,7 @@ #endif extern bool kvm_allowed; +extern bool kvm_sev_allowed; extern bool kvm_kernel_irqchip; extern bool kvm_split_irqchip; extern bool kvm_async_interrupts_allowed; @@ -56,6 +57,14 @@ extern bool kvm_ioeventfd_any_length_allowed; #if defined CONFIG_KVM || !defined NEED_CPU_H #define kvm_enabled() (kvm_allowed) + +/** + * kvm_sev_enabled: + * + * Returns: true if guest is running into SEV-enabled mode. + */ +#define kvm_sev_enabled() (kvm_sev_allowed) + /** * kvm_irqchip_in_kernel: * @@ -171,6 +180,7 @@ extern bool kvm_ioeventfd_any_length_allowed; #else #define kvm_enabled() (0) +#define kvm_sev_enabled() (false) #define kvm_irqchip_in_kernel() (false) #define kvm_irqchip_is_split() (false) #define kvm_async_interrupts_enabled() (false) diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h new file mode 100644 index 0000000..0ee8aff --- /dev/null +++ b/include/sysemu/sev.h @@ -0,0 +1,27 @@ +/* + * QEMU SEV support + * + * Copyright: Advanced Micro Devices, 2016 + * + * Authors: + * 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. + * + */ + +#ifndef QEMU_SEV_H +#define QEMU_SEV_H + +#include "sysemu/kvm.h" + +/** + * sev_init - initialize Secure Encrypted Virtualization on this guest + * @kvm_state - KVM handle + * Returns: 1 on error, 0 on success + */ +int sev_init(KVMState *kvm_state); + +#endif + diff --git a/kvm-all.c b/kvm-all.c index ebf35b0..e194849 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -36,6 +36,7 @@ #include "qemu/event_notifier.h" #include "trace.h" #include "hw/irq.h" +#include "sysemu/sev.h" #include "hw/boards.h" @@ -119,6 +120,7 @@ bool kvm_readonly_mem_allowed; bool kvm_vm_attributes_allowed; bool kvm_direct_msi_allowed; bool kvm_ioeventfd_any_length_allowed; +bool kvm_sev_allowed; static const KVMCapabilityInfo kvm_required_capabilites[] = { KVM_CAP_INFO(USER_MEMORY), @@ -1745,6 +1747,10 @@ static int kvm_init(MachineState *ms) kvm_state = s; + if (!sev_init(kvm_state)) { + kvm_sev_allowed = true; + } + if (kvm_eventfds_allowed) { s->memory_listener.listener.eventfd_add = kvm_mem_ioeventfd_add; s->memory_listener.listener.eventfd_del = kvm_mem_ioeventfd_del; 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; + } + + if (!strcmp(type, "unencrypted")) { + sev_info->type = UNENCRYPTED_GUEST; + } else if (!strcmp(type, "encrypted")) { + sev_info->type = PRE_ENCRYPTED_GUEST; + } 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; +} +
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" Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- Makefile.target | 2 include/sysemu/kvm.h | 10 ++ include/sysemu/sev.h | 27 +++++ kvm-all.c | 6 + sev.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 326 insertions(+), 1 deletion(-) create mode 100644 include/sysemu/sev.h create mode 100644 sev.c