From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk5aw-0005fv-Mz for qemu-devel@nongnu.org; Wed, 14 Sep 2016 04:31:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bk5as-00066u-1y for qemu-devel@nongnu.org; Wed, 14 Sep 2016 04:31:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk5ar-00066e-OS for qemu-devel@nongnu.org; Wed, 14 Sep 2016 04:30:57 -0400 Date: Wed, 14 Sep 2016 09:30:51 +0100 From: "Daniel P. Berrange" Message-ID: <20160914083051.GC28399@redhat.com> Reply-To: "Daniel P. Berrange" References: <147377800565.11859.4411044563640180545.stgit@brijesh-build-machine> <147377806784.11859.11149856529336910514.stgit@brijesh-build-machine> <20160913155807.GA2850@thinpad.lan.raisama.net> <6411b07f-4edd-390c-acca-5342ab1187ba@amd.com> <20160913220044.GY24695@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160913220044.GY24695@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [RFC PATCH v1 06/22] sev: add initial SEV support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Brijesh Singh , crosthwaite.peter@gmail.com, armbru@redhat.com, mst@redhat.com, p.fedin@samsung.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, rth@twiddle.net 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 :|