|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 4/7] vTPM: add vTPM device for HVM virtual machine
On Tue, Mar 10, 2015 at 08:13:59AM -0400, Quan Xu wrote:
> refactor libxl__device_vtpm_add to call the right helpers
> libxl__device_vtpm_add_{pv,hvm}. For HVM virtual machine,
> it does not support hot-plug and hot-unplug, as it requires
> SeaBios to initalize ACPI and virtual MMIO space for TPM
> TIS when virtual machine starts.
>
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> ---
> tools/libxl/libxl.c | 176
> +++++++++++++++++++++++++++++++++++++++++--
> tools/libxl/libxl.h | 7 +-
> tools/libxl/libxl_create.c | 32 +++++---
> tools/libxl/libxl_device.c | 38 +++++++++-
> tools/libxl/libxl_dm.c | 12 +++
> tools/libxl/libxl_internal.h | 6 +-
> tools/libxl/xl_cmdimpl.c | 4 +-
> 7 files changed, 253 insertions(+), 22 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 18561fb..c2d4baa 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1904,6 +1904,25 @@ out:
> return;
> }
>
> +static int libxl__frontend_device_nextid(libxl__gc *gc, uint32_t domid, char
> *device)
Line too line.
> +{
> + char *dompath, **l;
> + unsigned int nb;
> + int nextid = -1;
> +
> + if (!(dompath = libxl__xs_get_dompath(gc, domid)))
> + return nextid;
> +
> + l = libxl__xs_directory(gc, XBT_NULL,
> + GCSPRINTF("/local/domain/0/frontend/%s/%u", device, domid), &nb);
Please don't use hardcode /local/domain/0. And I suspect this will be
wrong mostly of the time since this function is not Dom0 only.
> + if (l == NULL || nb == 0)
> + nextid = 0;
> + else
> + nextid = strtoul(l[nb - 1], NULL, 10) + 1;
> +
> + return nextid;
> +}
And this function looks very much like libxl__device_nextid. The only
difference is the xenstore path, right? Please factor out the common
bits of both function to a helper.
> +
> /* common function to get next device id */
> static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
> {
> @@ -1957,9 +1976,9 @@ static int libxl__device_from_vtpm(libxl__gc *gc,
> uint32_t domid,
> return 0;
> }
>
> -void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> - libxl_device_vtpm *vtpm,
> - libxl__ao_device *aodev)
> +void libxl__device_vtpm_add_pv(libxl__egc *egc, uint32_t domid,
> + libxl_device_vtpm *vtpm,
> + libxl__ao_device *aodev)
> {
> STATE_AO_GC(aodev->ao);
> flexarray_t *front;
> @@ -2073,6 +2092,134 @@ out:
> return;
> }
>
> +void libxl__device_vtpm_add_hvm(libxl__egc *egc, uint32_t domid,
> + libxl_device_vtpm *vtpm,
> + libxl__ao_device *aodev)
> +{
[...]
> + libxl_device_vtpm_dispose(&vtpm_saved);
> + libxl_domain_config_dispose(&d_config);
> + aodev->rc = rc;
> + if (rc)
> + aodev->callback(egc, aodev);
> + return;
> +}
> +
This looks repetitive as well. I think the only difference between _pv
and _hvm is the xenstore nodes they write to. You need to do some
refactoring as well.
> libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid,
> int *num)
> {
> GC_INIT(ctx);
> @@ -4179,11 +4326,28 @@ DEFINE_DEVICE_ADD(disk)
> /* nic */
> DEFINE_DEVICE_ADD(nic)
>
> -/* vtpm */
> -DEFINE_DEVICE_ADD(vtpm)
> -
> #undef DEFINE_DEVICE_ADD
>
> +#define DEFINE_VTPM_ADD_PV(type) \
> + int libxl_device_##type##_add_pv(libxl_ctx *ctx, \
> + uint32_t domid, libxl_device_##type *type, \
> + const libxl_asyncop_how *ao_how) \
> + { \
> + AO_CREATE(ctx, domid, ao_how); \
> + libxl__ao_device *aodev; \
> + \
> + GCNEW(aodev); \
> + libxl__prepare_ao_device(ao, aodev); \
> + aodev->callback = device_addrm_aocomplete; \
> + aodev->update_json = true; \
> + libxl__device_##type##_add_pv(egc, domid, type, aodev); \
> + \
> + return AO_INPROGRESS; \
> + }
> +
> +/* vtpm */
> +DEFINE_VTPM_ADD_PV(vtpm)
No need to define a macro for this, since there is only one user of
this. But after looking further down I think you're doing it wrong.
There should still be a unified libxl_device_vtpm_add function that
works for both PV and HVM (if this is the case because your change).
> +
>
> /******************************************************************************/
>
> /*
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index c3451bd..38d3542 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1183,9 +1183,10 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx,
> uint32_t domid,
> libxl_channelinfo *channelinfo);
>
> /* Virtual TPMs */
> -int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm
> *vtpm,
> - const libxl_asyncop_how *ao_how)
> - LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vtpm_add_pv(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vtpm *vtpm,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> int libxl_device_vtpm_remove(libxl_ctx *ctx, uint32_t domid,
> libxl_device_vtpm *vtpm,
> const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index ffb124a..b88e1cb 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -901,6 +901,14 @@ static void initiate_domain_create(libxl__egc *egc,
> d_config->nics[i].devid = ++last_devid;
> }
>
> + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) {
> + for (i = 0; i < d_config->num_vtpms; i++) {
> + ret = libxl__device_vtpm_setdefault(gc, &d_config->vtpms[i]);
> + if (ret)
> + goto error_out;
> + }
> + }
> +
Why do you not need to call libxl__device_vtpm_setdefault for PV guest?
> if (restore_fd >= 0) {
> LOG(DEBUG, "restoring, not running bootloader");
> domcreate_bootloader_done(egc, &dcs->bl, 0);
> @@ -1244,6 +1252,13 @@ static void domcreate_launch_dm(libxl__egc *egc,
> libxl__multidev *multidev,
> libxl__device_vkb_add(gc, domid, &vkb);
> libxl_device_vkb_dispose(&vkb);
>
> + /*
> + * Plug vtpm devices. dcs->multidev.callback is NULL, no need to call
> + * libxl__multidev_prepared().
> + */
> + libxl__multidev_begin(ao, &dcs->multidev);
> + libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
> +
Why is vtpms added here instead of its usual location? I.e. the place
you stubbed out in previous patch. That is ...
> dcs->dmss.dm.guest_domid = domid;
> if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
> libxl__spawn_stub_dm(egc, &dcs->dmss);
> @@ -1361,15 +1376,14 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
> goto error_out;
> }
>
> - /*
> - * Plug vtpm devices only for PV guest. The xenstore directory is very
> - * different for PV guest and HVM guest, but it is still call it for
> - * creating HVM guest, and xl should create xenstore directory before
> - * spawning QEMU. So try to make it only for PV guest.
> - */
> - if (d_config->num_vtpms > 0 &&
> - d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
> -
> + /*
> + * Plug vtpm devices only for PV guest. The xenstore directory is very
> + * different for PV guest and HVM guest, but it is still call it for
> + * creating HVM guest, and xl should create xenstore directory before
> + * spawning QEMU. So try to make it only for PV guest.
> + */
> + if (d_config->num_vtpms > 0 &&
> + d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
... here?
BTW this hunk doesn't have meaningful changes...
> /* Attach vtpms */
> libxl__multidev_begin(ao, &dcs->multidev);
> dcs->multidev.callback = domcreate_attach_pci;
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 4b51ded..b1a71fe 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -26,6 +26,13 @@ char *libxl__device_frontend_path(libxl__gc *gc,
> libxl__device *device)
> if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
> return GCSPRINTF("%s/console", dom_path);
>
> + /* vTPM for HVM virtual machine is a special case */
> + else if (device->backend_kind == LIBXL__DEVICE_KIND_VTPM &&
> + device->domid == 0)
> + return GCSPRINTF("/local/domain/0/frontend/%s/%u/%d",
> + libxl__device_kind_to_string(device->backend_kind),
> + device->backend_domid, device->devid);
> +
You need to define xenstore protocol first.
And, why is it special? I don't know because you haven't defined what
the protocol looks like.
> return GCSPRINTF("%s/device/%s/%d", dom_path,
> libxl__device_kind_to_string(device->kind),
> device->devid);
> @@ -560,10 +567,39 @@ void libxl__multidev_prepared(libxl__egc *egc,
>
> DEFINE_DEVICES_ADD(disk)
> DEFINE_DEVICES_ADD(nic)
> -DEFINE_DEVICES_ADD(vtpm)
>
> #undef DEFINE_DEVICES_ADD
>
> +#define DEFINE_VTPM_ADD(_type) \
> + void libxl__add_##_type##s(libxl__egc *egc, libxl__ao *ao, \
> + uint32_t domid, \
> + libxl_domain_config *d_config, \
> + libxl__multidev *multidev) \
> + { \
> + AO_GC; \
> + int i; \
> + for (i = 0; i < d_config->num_##_type##s; i++) { \
> + libxl__ao_device *aodev = libxl__multidev_prepare(multidev); \
> + switch(d_config->c_info.type) { \
> + case LIBXL_DOMAIN_TYPE_PV: \
> + libxl__device_##_type##_add_pv(egc, domid, \
> + &d_config->_type##s[i], \
> + aodev); \
> + break; \
> + case LIBXL_DOMAIN_TYPE_HVM: \
> + libxl__device_##_type##_add_hvm(egc, domid, \
> + &d_config->_type##s[i], \
> + aodev); \
> + break; \
> + default: \
> + break; \
> + } \
> + } \
> + }
> +
> +DEFINE_VTPM_ADD(vtpm)
> +
It's fine by me that you just hand code this function without using this
macro.
But, I don't think the function body as-is is correct. There should not
be a loop. This function should only handle one device.
> +#undef DEFINE_VTPM_ADD
>
> /******************************************************************************/
>
> int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3e191c3..183910c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -414,6 +414,7 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
> const libxl_device_nic *nics = guest_config->nics;
> const int num_disks = guest_config->num_disks;
> const int num_nics = guest_config->num_nics;
> + const int num_vtpms = guest_config->num_vtpms;
> const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
> const libxl_sdl_info *sdl = dm_sdl(guest_config);
> const char *keymap = dm_keymap(guest_config);
> @@ -747,6 +748,17 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
> abort();
> }
>
> + /* Add vTPM parameters for HVM virtual machine */
> + if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> + num_vtpms > 0) {
> + flexarray_vappend(dm_args, "-tpmdev",
> + libxl__sprintf(gc, "xenstubdoms,id=xenvtpm%d",
> + guest_domid), NULL);
Please use GCSPRINTF.
> + flexarray_vappend(dm_args,"-device",
> + libxl__sprintf(gc, "tpm-tis,tpmdev=xenvtpm%d",
> + guest_domid), NULL);
> + }
> +
> ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);
> flexarray_append(dm_args, "-m");
> flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size));
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4361421..c6c82dd 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2384,10 +2384,14 @@ _hidden void libxl__device_nic_add(libxl__egc *egc,
> uint32_t domid,
> libxl_device_nic *nic,
> libxl__ao_device *aodev);
>
> -_hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> +_hidden void libxl__device_vtpm_add_pv(libxl__egc *egc, uint32_t domid,
> libxl_device_vtpm *vtpm,
> libxl__ao_device *aodev);
>
> +void libxl__device_vtpm_add_hvm(libxl__egc *egc, uint32_t domid,
> + libxl_device_vtpm *vtpm,
> + libxl__ao_device *aodev);
> +
> /* Internal function to connect a vkb device */
> _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
> libxl_device_vkb *vkb);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 3c9f146..e01d341 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -6490,8 +6490,8 @@ int main_vtpmattach(int argc, char **argv)
> return 0;
> }
>
> - if (libxl_device_vtpm_add(ctx, domid, &vtpm, 0)) {
> - fprintf(stderr, "libxl_device_vtpm_add failed.\n");
> + if (libxl_device_vtpm_add_pv(ctx, domid, &vtpm, 0)) {
> + fprintf(stderr, "libxl_device_vtpm_add_pv failed.\n");
How do you add vtpm for hvm guest?
Wei.
> return 1;
> }
> libxl_device_vtpm_dispose(&vtpm);
> --
> 1.8.3.2
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |