[RFC,v1,06/22] sev: add initial SEV support

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

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.