[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 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 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?
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   = 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
Description: S/MIME Cryptographic Signature

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.