Re: [PATCH vtpm v2 11/12] add vtpm support to libxl

[Matthew Fioravante <matthew.fioravante@jhuapl.edu>]

[-- Attachment #1.1: Type: text/plain, Size: 8600 bytes --]

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 #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 1459 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel