|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH vtpm v2 11/12] add vtpm support to libxl
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 change
> 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 *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 = vtpm->devid;
>> + device->backend_domid = vtpm->backend_domid;
>> + device->backend_kind = LIBXL__DEVICE_KIND_VTPM;
>> + device->devid = vtpm->devid;
>> + device->domid = domid;
>> + device->kind = 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 = libxl__device_vtpm_setdefault(gc, vtpm);
>> + if (rc) goto out;
>> +
>> + front = 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 = ERROR_NOMEM;
>> + goto out;
>> + }
>> + back = flexarray_make(16, 1);
>> + if (!back) {
>> + rc = ERROR_NOMEM;
>> + goto out;
>> + }
>> +
>> + if(vtpm->devid == -1) {
>> + if (!(dompath = libxl__xs_get_dompath(gc, domid))) {
>> + rc = ERROR_FAIL;
>> + goto out_free;
>> + }
>> + l = 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 = 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_vtpminfo
>> *vtpminfo)
>> +{
>> [...]
>> + val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/uuid",
>> vtpminfo->backend));
>> + if(val == 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 = CONTAINER_OF(multidev, *dcs, multidev);
>> + STATE_AO_GC(dcs->ao);
>> + int domid = dcs->guest_domid;
>> +
>> + libxl_domain_config* const d_config = 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 = 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 = libxl_device_vtpm_list(ctx, domid, &nb);
>> + if (!vtpms)
>> + return ERROR_FAIL;
>> +
>> + memset(vtpm, 0, sizeof (libxl_device_vtpm));
>> + rc = 1;
>> + for (i = 0; i < nb; ++i) {
>> + if(!libxl_uuid_compare(uuid, &vtpms[i].uuid)) {
>> + vtpm->backend_domid = vtpms[i].backend_domid;
>> + vtpm->devid = vtpms[i].devid;
>> + libxl_uuid_copy(&vtpm->uuid, &vtpms[i].uuid);
>> + rc = 0;
>> + break;
>> + }
>> + }
>> +
>> + for (i=0; i<nb; 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 did
> 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[] = {
>> "Destroy a domain's virtual block device",
>> "<Domain> <DevId>",
>> },
>> + { "vtpm-attach",
>> + &main_vtpmattach, 0, 1,
>> + "Create a new virtual TPM device",
>> + "<Domain> [uuid=<uuid>] [backend=<BackDomain>]",
>> + },
>> + { "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",
>> + "<Domain(s)>",
>> + },
>> + { "vtpm-detach",
>> + &main_vtpmdetach, 0, 1,
>> + "Destroy a domain's virtual TPM device",
>> + "<Domain> <DevId|uuid>",
>> + },
>> { "uptime",
>> &main_uptime, 0, 0,
>> "Print uptime for all/some domains",
>> --
>> 1.7.4.4
>>
>
Attachment:
smime.p7s _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |