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

[Eduardo Habkost <ehabkost@redhat.com>]

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.

-- 
Eduardo