|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Embedded-pv-devel] [PATCH RFC 1/6] libxl: implementation of PV rtc device interface
On Thu, May 19, 2016 at 05:37:30PM +0300, Iurii Mykhalskyi wrote:
> PV rtc device interface is implemented in libxl and xl with
> full support for device control. No JSON parser for domain
> configuration yet.
I'm not sure I follow the last sentence. What JSON parser do you need?
Is there a specification for vrtc that I can reference? I don't see one
in xen/include/public/io.
> +
>
>
> /******************************************************************************/
>
> @@ -4131,12 +4402,14 @@ out:
> * libxl_device_disk_destroy
> * libxl_device_nic_remove
> * libxl_device_nic_destroy
> - * libxl_device_vtpm_remove
> - * libxl_device_vtpm_destroy
> * libxl_device_vkb_remove
> * libxl_device_vkb_destroy
> * libxl_device_vfb_remove
> * libxl_device_vfb_destroy
> + * libxl_device_vtpm_remove
> + * libxl_device_vtpm_destroy
Unrelated change here.
> + * libxl_device_vrtc_remove
> + * libxl_device_vrtc_destroy
> */
> #define DEFINE_DEVICE_REMOVE(type, removedestroy, f) \
> int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> @@ -4188,6 +4461,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
> DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
> DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>
> +/* vrtc */
> +DEFINE_DEVICE_REMOVE(vrtc, remove, 0)
> +DEFINE_DEVICE_REMOVE(vrtc, destroy, 1)
> +
> /* channel/console hotunplug is not implemented. There are 2 possibilities:
> * 1. add support for secondary consoles to xenconsoled
> * 2. dynamically add/remove qemu chardevs via qmp messages. */
> @@ -4201,6 +4478,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
> * libxl_device_disk_add
> * libxl_device_nic_add
> * libxl_device_vtpm_add
> + * libxl_device_vrtc_add
> */
>
> #define DEFINE_DEVICE_ADD(type) \
> @@ -4232,6 +4510,9 @@ DEFINE_DEVICE_ADD(nic)
> /* vtpm */
> DEFINE_DEVICE_ADD(vtpm)
>
> +/* vrtc */
> +DEFINE_DEVICE_ADD(vrtc)
> +
> #undef DEFINE_DEVICE_ADD
>
>
> /******************************************************************************/
> @@ -6747,6 +7028,8 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx,
> uint32_t domid,
>
> MERGE(vtpm, vtpms, COMPARE_DEVID, {});
>
> + MERGE(vrtc, vrtcs, COMPARE_DEVID, {});
> +
> MERGE(pci, pcidevs, COMPARE_PCI, {});
>
> /* Take care of removable device. We maintain invariant in the
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index fa5aedd..9243b86 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1435,6 +1435,23 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx
> *ctx, uint32_t domid, int *n
> int libxl_device_vtpm_getinfo(libxl_ctx *ctx, uint32_t domid,
> libxl_device_vtpm *vtpm, libxl_vtpminfo
> *vtpminfo);
>
> +/* RTC */
> +int libxl_device_vrtc_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vrtc
> *vrtc,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vrtc_remove(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vrtc *vrtc,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vrtc_destroy(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vrtc *vrtc,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_vrtc *libxl_device_vrtc_list(libxl_ctx *ctx, uint32_t domid,
> int *num);
> +int libxl_device_vrtc_getinfo(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_vrtc *vrtc, libxl_vrtcinfo
> *vrtcinfo);
> +
> /* Keyboard */
> int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb
> *vkb,
> const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1c0579c..1206c34 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -734,6 +734,8 @@ static void domcreate_launch_dm(libxl__egc *egc,
> libxl__multidev *aodevs,
>
> static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev
> *multidev,
> int ret);
> +static void domcreate_attach_vrtcs(libxl__egc *egc, libxl__multidev
> *multidev,
> + int ret);
> static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
> int ret);
> static void domcreate_attach_dtdev(libxl__egc *egc,
> @@ -1392,12 +1394,45 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
> if (d_config->num_vtpms > 0) {
> /* Attach vtpms */
> libxl__multidev_begin(ao, &dcs->multidev);
> - dcs->multidev.callback = domcreate_attach_pci;
> + dcs->multidev.callback = domcreate_attach_vrtcs;
> libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
> libxl__multidev_prepared(egc, &dcs->multidev, 0);
> return;
> }
>
> + domcreate_attach_vrtcs(egc, multidev, 0);
> + return;
> +
> +error_out:
> + assert(ret);
> + domcreate_complete(egc, dcs, ret);
> +}
> +
> +static void domcreate_attach_vrtcs(libxl__egc *egc,
> + libxl__multidev *multidev,
> + int ret)
> +{
> + 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 vtpm devices");
> + goto error_out;
> + }
> +
> + /* Plug vrtc devices */
> + if (d_config->num_vrtcs > 0) {
> + /* Attach vrtcs */
> + libxl__multidev_begin(ao, &dcs->multidev);
> + dcs->multidev.callback = domcreate_attach_pci;
> + libxl__add_vrtcs(egc, ao, domid, d_config, &dcs->multidev);
> + libxl__multidev_prepared(egc, &dcs->multidev, 0);
> + return;
> + }
> +
> domcreate_attach_pci(egc, multidev, 0);
> return;
>
> @@ -1419,7 +1454,7 @@ static void domcreate_attach_pci(libxl__egc *egc,
> libxl__multidev *multidev,
> libxl_domain_config *const d_config = dcs->guest_config;
>
> if (ret) {
> - LOG(ERROR, "unable to add vtpm devices");
> + LOG(ERROR, "unable to add vrtc devices");
> goto error_out;
> }
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 8bb5e93..72f09a0 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -544,6 +544,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
> * libxl__add_disks
> * libxl__add_nics
> * libxl__add_vtpms
> + * libxl__add_vrtcs
> */
>
> #define DEFINE_DEVICES_ADD(type) \
> @@ -563,6 +564,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
> DEFINE_DEVICES_ADD(disk)
> DEFINE_DEVICES_ADD(nic)
> DEFINE_DEVICES_ADD(vtpm)
> +DEFINE_DEVICES_ADD(vrtc)
>
> #undef DEFINE_DEVICES_ADD
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5b0b50a..2a423b5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -476,7 +476,8 @@ typedef struct {
> #define QEMU_BACKEND(dev) (\
> (dev)->backend_kind == LIBXL__DEVICE_KIND_QDISK || \
> (dev)->backend_kind == LIBXL__DEVICE_KIND_VFB || \
> - (dev)->backend_kind == LIBXL__DEVICE_KIND_VKBD)
> + (dev)->backend_kind == LIBXL__DEVICE_KIND_VKBD || \
> + (dev)->backend_kind == LIBXL__DEVICE_KIND_VRTC)
>
So it is going to be backed by QEMU?
> #define XC_PCI_BDF "0x%x, 0x%x, 0x%x, 0x%x"
> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> @@ -1186,6 +1187,7 @@ _hidden int libxl__device_disk_setdefault(libxl__gc *gc,
> _hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic
> *nic,
> uint32_t domid);
> _hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm
> *vtpm);
> +_hidden int libxl__device_vrtc_setdefault(libxl__gc *gc, libxl_device_vrtc
> *vrtc);
> _hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb
> *vfb);
> _hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb
> *vkb);
> _hidden int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci
> *pci);
> @@ -2549,6 +2551,8 @@ struct libxl__multidev {
> * xenstore entry afterwards. We have both JSON and xenstore entry,
> * it's a valid state.
> */
> +
> +/* AO operation to connect a disk device */
Unrelated change.
All these add comments should go to a separate patch. But I don't think
they are absolutely needed.
> _hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
> libxl_device_disk *disk,
> libxl__ao_device *aodev);
> @@ -2558,9 +2562,15 @@ _hidden void libxl__device_nic_add(libxl__egc *egc,
> uint32_t domid,
> libxl_device_nic *nic,
> libxl__ao_device *aodev);
>
> +/* AO operation to connect a tpm device */
> _hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> - libxl_device_vtpm *vtpm,
> - libxl__ao_device *aodev);
> + libxl_device_vtpm *vtpm,
> + libxl__ao_device *aodev);
> +
Unrelated change.
> +/* AO operation to connect an rtc device */
> +_hidden void libxl__device_vrtc_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_vrtc *vrtc,
> + libxl__ao_device *aodev);
>
> /* Internal function to connect a vkb device */
> _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
> @@ -3278,6 +3288,9 @@ _hidden void libxl__add_vtpms(libxl__egc *egc,
> libxl__ao *ao, uint32_t domid,
> libxl_domain_config *d_config,
> libxl__multidev *multidev);
>
> +_hidden void libxl__add_vrtcs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> + libxl_domain_config *d_config,
> + libxl__multidev *multidev);
Indentation.
I skim-read this patch. The structure and arrangement looks plausible.
Please fix the coding style issues and remove unrelated changes. Please
also provide some more references in commit message.
You also need to provide a LIBXL_HAVE macro in libxl.h.
I think my comments here apply to all patches in this series.
Please don't hesitate to ask if my comments are not clear.
Wei.
_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |