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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

> 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.


> +    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*.

[...]

> +    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?

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.

> +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.

> +{
> [...]
> +   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

> +      "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
> 



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