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 :|
