|
[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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |