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:36:11 -0400 Message-ID: <5074284B.1050402@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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5187726707256683994==" Return-path: In-Reply-To: <1349778775.21847.132.camel@zakaz.uk.xensource.com> 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. --===============5187726707256683994== Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha1; boundary="------------ms080806030800010400010107" This is a cryptographically signed message in MIME format. --------------ms080806030800010400010107 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 chang= e > 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? > >> 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 *v= tpm) >> +{ >> + 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 to= > 75-80 characters please. > > There are various helper macros like GCSPRINTF which can help to reduce= > the length of lines. Also you might find the LOG* macros useful instead= > 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 be= > 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_vtpminf= o *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 uuid= ?? (%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, m= ultidev); >> + 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 *vtpm= ) >> +{ >> + 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 the= > 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 di= d > 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 >> > --------------ms080806030800010400010107 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 KoZIhvcNAQkFMQ8XDTEyMTAwOTEzMzYxMVowIwYJKoZIhvcNAQkEMRYEFCXQoBS5LZBmK9eg PtEnGzrStFJBMGwGCSqGSIb3DQEJDzFfMF0wCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBAjAK BggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZIhvcNAwICAUAwBwYFKw4DAgcwDQYI KoZIhvcNAwICASgwDQYJKoZIhvcNAQEBBQAEgYAnnBk4lXZOgBgkbnbRGb8wSPyDwLjWm/AR V3lpPeEgplIZBAJcmstAbBdK/e60AagPMcuDJV6716kwzlYfuZRyPeGxxQSee3XUPOBar6/5 Svxemu3447pCYfE/DfqP/L+WlGcuNFH2utRmdIQpnc4RaH/6pj+gvp9Dvf4RA+im5wAAAAAA AA== --------------ms080806030800010400010107-- --===============5187726707256683994== 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 --===============5187726707256683994==--