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

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|