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

[Eduardo Habkost <ehabkost@redhat.com>]

(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?

-- 
Eduardo