From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Fioravante Subject: Re: [PATCH vtpm v2 11/12] add vtpm support to libxl Date: Tue, 09 Oct 2012 09:42:14 -0400 Message-ID: <507429B6.6020008@jhuapl.edu> References: <1349460132-13853-1-git-send-email-matthew.fioravante@jhuapl.edu> <1349460132-13853-3-git-send-email-matthew.fioravante@jhuapl.edu> <1349778775.21847.132.camel@zakaz.uk.xensource.com> <5074284B.1050402@jhuapl.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8615713508135530041==" Return-path: In-Reply-To: <5074284B.1050402@jhuapl.edu> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org This is a cryptographically signed message in MIME format. --===============8615713508135530041== Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha1; boundary="------------ms060507000301080509020503" This is a cryptographically signed message in MIME format. --------------ms060507000301080509020503 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 10/09/2012 09:36 AM, Matthew Fioravante wrote: > On 10/09/2012 06:32 AM, Ian Campbell wrote: >> IIRC you are reworking the vtpm stuff to only support the stub domaun >> model and not the process model -- does this mean this patch will chan= ge >> or is this already only doing stub stuff? > Since I'm not removing the process model, its possible that this. the > tpm mini-os drivers, and the linux vtpm drivers might change slightly. > Would you prefer I held off on these last 2 patches until the changes > are finalized? Sorry typo *since I am removing the process model* >>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >>> index 1606eb1..17094ca 100644 >>> --- a/tools/libxl/libxl.c >>> +++ b/tools/libxl/libxl.c >>> @@ -1726,6 +1726,246 @@ out: >>> } >>> >>> /*******************************************************************= ***********/ >>> +int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *= vtpm) >>> +{ >>> + if(libxl_uuid_is_nil(&vtpm->uuid)) { >>> + libxl_uuid_generate(&vtpm->uuid); >>> + } >>> + return 0; >>> +} >>> + >>> +static int libxl__device_from_vtpm(libxl__gc *gc, uint32_t domid, >>> + libxl_device_vtpm *vtpm, >>> + libxl__device *device) >>> +{ >>> + device->backend_devid =3D vtpm->devid; >>> + device->backend_domid =3D vtpm->backend_domid; >>> + device->backend_kind =3D LIBXL__DEVICE_KIND_VTPM; >>> + device->devid =3D vtpm->devid; >>> + device->domid =3D domid; >>> + device->kind =3D LIBXL__DEVICE_KIND_VTPM; >>> + >>> + return 0; >>> +} >>> + >>> +void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, >>> + libxl_device_vtpm *vtpm, >>> + libxl__ao_device *aodev) >>> +{ >>> + STATE_AO_GC(aodev->ao); >>> + flexarray_t *front; >>> + flexarray_t *back; >>> + libxl__device *device; >>> + char *dompath, **l; >>> + unsigned int nb, rc; >>> + >>> + rc =3D libxl__device_vtpm_setdefault(gc, vtpm); >>> + if (rc) goto out; >>> + >>> + front =3D flexarray_make(16, 1); >> Sorry but in the meantime the flexarray interface has changed, it now >> accepts a gc -- see commit 25991:5c6b72b62bd7. > Will fix >> >>> + if (!front) { >>> + rc =3D ERROR_NOMEM; >>> + goto out; >>> + } >>> + back =3D flexarray_make(16, 1); >>> + if (!back) { >>> + rc =3D ERROR_NOMEM; >>> + goto out; >>> + } >>> + >>> + if(vtpm->devid =3D=3D -1) { >>> + if (!(dompath =3D libxl__xs_get_dompath(gc, domid))) { >>> + rc =3D ERROR_FAIL; >>> + goto out_free; >>> + } >>> + l =3D libxl__xs_directory(gc, XBT_NULL, libxl__sprintf(gc, "= %s/device/vtpm", dompath), &nb); >> You have some very long lines in this patch. Can you try and keep it t= o >> 75-80 characters please. >> >> There are various helper macros like GCSPRINTF which can help to reduc= e >> the length of lines. Also you might find the LOG* macros useful instea= d >> of the more verbose LIBXL__LOG*. > noted >> [...] >> >>> + flexarray_append(back, "instance"); /* MAYBE CAN GET RID OF THIS= */ >>> + flexarray_append(back, "0"); >>> + flexarray_append(back, "pref_instance"); /* MAYBE CAN GET RID OF= THIS */ >>> + flexarray_append(back, "0"); >>> + flexarray_append(back, "resume"); >>> + flexarray_append(back, "False"); >>> + flexarray_append(back, "ready"); /* MAYBE CAN GET RID OF THIS */= >>> + flexarray_append(back, "1"); >> Can we decide now or is this a future work thing? > These will probably be removed from the driver now that the process > model is gone. >> Not a lot of existing code uses it but we have flexarray_append_pair >> which can clarify the pairing of keys and values if you would like to >> use it. > Probably makes code a little more readable, ill look into it. >>> +static void libxl__device_vtpm_from_xs_fe(libxl__gc *gc, >>> + const char* fe_path, >>> + libxl_device_vtpm *vtpm) >>> >> Other devices have from_xs_be not fe. This is because we "trust" the b= e >> to be less malicious. > will rework >>> +{ >>> [...] >>> + tmp =3D libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/uuid"= , fe_path)); >>> + if(tmp) { >>> + libxl_uuid_from_string(&(vtpm->uuid), tmp); >>> + } >>> +} >>> [...] >>> +int libxl_device_vtpm_getinfo(libxl_ctx *ctx, uint32_t domid, >>> + libxl_device_vtpm *vtpm, libxl_vtpmin= fo *vtpminfo) >>> +{ >>> [...] >>> + val =3D libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/uuid= ", vtpminfo->backend)); >>> + if(val =3D=3D NULL) { >>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/uuid does not exist!\n"= , vtpminfo->backend); >>> + goto err; >>> + } >>> + if(libxl_uuid_from_string(&(vtpminfo->uuid), val)) { >>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/uuid is a malformed uui= d?? (%s) Probably a bug!\n", vtpminfo->backend, val); >>> + goto err; >> This is fatal here but not in from_xs_fe? >> >>> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev = *multidev, int ret) { >> brace on next line please. >> >>> + libxl__domain_create_state *dcs =3D CONTAINER_OF(multidev, *dcs, = multidev); >>> + STATE_AO_GC(dcs->ao); >>> + int domid =3D dcs->guest_domid; >>> + >>> + libxl_domain_config* const d_config =3D dcs->guest_config; >>> + >>> + if(ret) { >>> + LOG(ERROR, "unable to add nic devices"); >>> + goto error_out; >>> + } >> Four space indents above please. >> >>> + /* Plug vtpm devices */ >>> + if (d_config->num_vtpms > 0) { >>> + /* Attach vtpms */ >>> + libxl__multidev_begin(ao, &dcs->multidev); >>> + dcs->multidev.callback =3D domcreate_attach_pci; >>> + libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev); >>> + libxl__multidev_prepared(egc, &dcs->multidev, 0); >>> + return; >>> + } >> This indent is ok. >> >>> + >>> + domcreate_attach_pci(egc, multidev, 0); >>> + return; >>> + >>> +error_out: >>> + assert(ret); >>> + domcreate_complete(egc, dcs, ret); >> But here we've gone back to 3 spaces again. >> >>> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c >>> index 55cd299..73a158a 100644 >>> --- a/tools/libxl/libxl_utils.c >>> +++ b/tools/libxl/libxl_utils.c >>> @@ -463,6 +463,35 @@ int libxl_pipe(libxl_ctx *ctx, int pipes[2]) >>> return 0; >>> } >>> >>> +int libxl_uuid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid, >>> + libxl_uuid* uuid, libxl_device_vtpm *vtp= m) >>> +{ >>> + libxl_device_vtpm *vtpms; >>> + int nb, i; >>> + int rc; >>> + >>> + vtpms =3D libxl_device_vtpm_list(ctx, domid, &nb); >>> + if (!vtpms) >>> + return ERROR_FAIL; >>> + >>> + memset(vtpm, 0, sizeof (libxl_device_vtpm)); >>> + rc =3D 1; >>> + for (i =3D 0; i < nb; ++i) { >>> + if(!libxl_uuid_compare(uuid, &vtpms[i].uuid)) { >>> + vtpm->backend_domid =3D vtpms[i].backend_domid; >>> + vtpm->devid =3D vtpms[i].devid; >>> + libxl_uuid_copy(&vtpm->uuid, &vtpms[i].uuid); >>> + rc =3D 0; >>> + break; >>> + } >>> + } >>> + >>> + for (i=3D0; i>> + libxl_device_vtpm_dispose(&vtpms[i]); >>> + free(vtpms); >> I think I saw this a few times (probably copied from elsewhere) but th= e >> modern alternative is to define libxl_THING_list_free and use that to >> free the result of libxl_THING_list. >> >> We just didn't go back and change all the existing instances when we d= id >> this. >> >> >>> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c >>> index 85ea768..7c018eb 100644 >>> --- a/tools/libxl/xl_cmdtable.c >>> +++ b/tools/libxl/xl_cmdtable.c >>> @@ -338,6 +338,21 @@ struct cmd_spec cmd_table[] =3D { >>> "Destroy a domain's virtual block device", >>> " ", >>> }, >>> + { "vtpm-attach", >>> + &main_vtpmattach, 0, 1, >>> + "Create a new virtual TPM device", >>> + " [uuid=3D] [backend=3D]", >>> + }, >>> + { "vtpm-list", >>> + &main_vtpmlist, 0, 0, >> I think you want the first 0 to be 1 since you do support dry run in >> this command > agreed, must have been a typo >>> + "List virtual TPM devices for a domain", >>> + "", >>> + }, >>> + { "vtpm-detach", >>> + &main_vtpmdetach, 0, 1, >>> + "Destroy a domain's virtual TPM device", >>> + " ", >>> + }, >>> { "uptime", >>> &main_uptime, 0, 0, >>> "Print uptime for all/some domains", >>> -- >>> 1.7.4.4 >>> > --------------ms060507000301080509020503 Content-Type: application/pkcs7-signature; name="smime.p7s" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="smime.p7s" Content-Description: S/MIME Cryptographic Signature MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIIDyjCC A8YwggMvoAMCAQICBD/xyf0wDQYJKoZIhvcNAQEFBQAwLzELMAkGA1UEBhMCVVMxDzANBgNV BAoTBkpIVUFQTDEPMA0GA1UECxMGQklTRENBMB4XDTEwMDYxMTE4MjIwNloXDTEzMDYxMTE4 NTIwNlowZjELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkpIVUFQTDEPMA0GA1UECxMGUGVvcGxl MTUwFgYDVQQLEw9WUE5Hcm91cC1CSVNEQ0EwGwYDVQQDExRNYXR0aGV3IEUgRmlvcmF2YW50 ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAnpbwVSP6o1Nb5lcW7dd3yTo9iBJdi7qz 4nANOMFPK7JOy5npKN1iiousl28U/scUJES55gPwAWYJK3uVyQAsA4adgDKi5DoD1UHDQEwp bY7iHLJeq0NPr4BqYNqnCFPbE6HC8zSJrr4qKn+gVUQT39SIFqdiIPJwZL8FYTRQ/zsCAwEA AaOCAbYwggGyMAsGA1UdDwQEAwIHgDArBgNVHRAEJDAigA8yMDEwMDYxMTE4MjIwNlqBDzIw MTIwNzE3MjI1MjA2WjAbBg0rBgEEAbMlCwMBAQEBBAoWCGZpb3JhbWUxMBsGDSsGAQQBsyUL AwEBAQIEChIIMDAxMDQyNjEwWAYJYIZIAYb6ax4BBEsMSVRoZSBwcml2YXRlIGtleSBjb3Jy ZXNwb25kaW5nIHRvIHRoaXMgY2VydGlmaWNhdGUgbWF5IGhhdmUgYmVlbiBleHBvcnRlZC4w KAYDVR0RBCEwH4EdTWF0dGhldy5GaW9yYXZhbnRlQGpodWFwbC5lZHUwUgYDVR0fBEswSTBH oEWgQ6RBMD8xCzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZKSFVBUEwxDzANBgNVBAsTBkJJU0RD QTEOMAwGA1UEAxMFQ1JMNTYwHwYDVR0jBBgwFoAUCDUpmxH52EU2CyWmF2EJMB1yqeswHQYD VR0OBBYEFO6LYxg6r9wHZ+zdQtBHn1dZ/YTNMAkGA1UdEwQCMAAwGQYJKoZIhvZ9B0EABAww ChsEVjcuMQMCBLAwDQYJKoZIhvcNAQEFBQADgYEAJO9HQh4YNChVLzuZqK5ARJARD8JoujGZ fdo75quvg2jXFQe2sEjvLnxJZgm/pv8fdZakq48CWwjYHKuvIp7sDjTEsQfo+y7SpN/N2NvJ WU5SqfK1VgYtNLRRoGJUB5Q1aZ+Dg95g3kqpyfpUMISJL8IKVLtJVfN4fggFVUYZ9wwxggGr MIIBpwIBATA3MC8xCzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZKSFVBUEwxDzANBgNVBAsTBkJJ U0RDQQIEP/HJ/TAJBgUrDgMCGgUAoIHLMBgGCSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJ KoZIhvcNAQkFMQ8XDTEyMTAwOTEzNDIxNFowIwYJKoZIhvcNAQkEMRYEFBKaXg3PQfrqIR+W Szl3ib8ZNhS5MGwGCSqGSIb3DQEJDzFfMF0wCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBAjAK BggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZIhvcNAwICAUAwBwYFKw4DAgcwDQYI KoZIhvcNAwICASgwDQYJKoZIhvcNAQEBBQAEgYBL9w/kvh7VX3+1aSJKchPp2efYsruOc08S EBrNXKT0sm90uMod7bOxLkDbBiOApZG9VBb5GkWAM4niqhEatDxn7wwU7LJ1+3v2fKdmh/qt NHWzBNSaqDGwlWzjWsVeUPCBKuQ1ddBdfV3WZsgtZSDD1wahIZNb3WdSUOTQAake+wAAAAAA AA== --------------ms060507000301080509020503-- --===============8615713508135530041== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8615713508135530041==--