| Message ID | 1494426918-32737-1-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, May 10, 2017 at 08:05:12PM +0530, Bhupinder Thakur wrote: > libxl__device_console_add(gc, domid, &console, state, &device); > @@ -1369,14 +1377,22 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, > } > case LIBXL_DOMAIN_TYPE_PV: > { > - libxl__device_console console; > - libxl__device device; > + libxl__device_console console, vuart; > + libxl__device device, vuart_device; > > for (i = 0; i < d_config->num_vfbs; i++) { > libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]); > libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]); > } > > + if (d_config->b_info.vuart) > + { > + init_console_info(gc, &vuart, 0); > + vuart.backend_domid = state->console_domid; > + libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device); > + libxl__device_console_dispose(&vuart); > + } > + This is strange. You have code snippet for both PV and HVM. Why? > init_console_info(gc, &console, 0); > console.backend_domid = state->console_domid; > libxl__device_console_add(gc, domid, &console, state, &device); > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > index 5e96676..bb25672 100644 > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -26,6 +26,9 @@ static 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); > > + if (device->kind == LIBXL__DEVICE_KIND_VUART) > + return GCSPRINTF("%s/vuart/%d", dom_path, device->devid); > + > return GCSPRINTF("%s/device/%s/%d", dom_path, > libxl__device_kind_to_string(device->kind), > device->devid); > @@ -151,13 +154,19 @@ retry_transaction: > if (rc) goto out; > > if (!libxl_only) { > - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), > - frontend_path); > - if (rc) goto out; > + if (fents || ro_fents) > + { > + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), > + frontend_path); > + if (rc) goto out; > + } > > - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), > - backend_path); > - if (rc) goto out; > + if (bents) > + { > + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), > + backend_path); > + if (rc) goto out; > + } What is this for? If there is no fe or be entries you skip the path creation altogether. But why? This doesn't seem to be related to your patch. At least explain this a bit in the commit message?
Hi Wei, On 11 May 2017 at 16:40, Wei Liu <wei.liu2@citrix.com> wrote: > On Wed, May 10, 2017 at 08:05:12PM +0530, Bhupinder Thakur wrote: >> libxl__device_console_add(gc, domid, &console, state, &device); >> @@ -1369,14 +1377,22 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, >> } >> case LIBXL_DOMAIN_TYPE_PV: >> { >> - libxl__device_console console; >> - libxl__device device; >> + libxl__device_console console, vuart; >> + libxl__device device, vuart_device; >> >> for (i = 0; i < d_config->num_vfbs; i++) { >> libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]); >> libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]); >> } >> >> + if (d_config->b_info.vuart) >> + { >> + init_console_info(gc, &vuart, 0); >> + vuart.backend_domid = state->console_domid; >> + libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device); >> + libxl__device_console_dispose(&vuart); >> + } >> + > > This is strange. You have code snippet for both PV and HVM. Why? Yes I believe I should have added only for PV guest but to keep the code consistent for both type of guests, I added for both. I will remove it for HVM. > >> init_console_info(gc, &console, 0); >> console.backend_domid = state->console_domid; >> libxl__device_console_add(gc, domid, &console, state, &device); >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> index 5e96676..bb25672 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -26,6 +26,9 @@ static 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); >> >> + if (device->kind == LIBXL__DEVICE_KIND_VUART) >> + return GCSPRINTF("%s/vuart/%d", dom_path, device->devid); >> + >> return GCSPRINTF("%s/device/%s/%d", dom_path, >> libxl__device_kind_to_string(device->kind), >> device->devid); >> @@ -151,13 +154,19 @@ retry_transaction: >> if (rc) goto out; >> >> if (!libxl_only) { >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), >> - frontend_path); >> - if (rc) goto out; >> + if (fents || ro_fents) >> + { >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), >> + frontend_path); >> + if (rc) goto out; >> + } >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), >> - backend_path); >> - if (rc) goto out; >> + if (bents) >> + { >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), >> + backend_path); >> + if (rc) goto out; >> + } > > What is this for? > > If there is no fe or be entries you skip the path creation altogether. > But why? This doesn't seem to be related to your patch. For vuart, I am adding only a front end node but the libxl__device_generic_add() creates the backend path also,even though there is no backend node. To remove that hanging be path, I added this check. > > At least explain this a bit in the commit message? I will add more details in the commit message. Regards, Bhupinder
On Fri, May 12, 2017 at 03:02:29PM +0530, Bhupinder Thakur wrote: [...] > >> @@ -151,13 +154,19 @@ retry_transaction: > >> if (rc) goto out; > >> > >> if (!libxl_only) { > >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), > >> - frontend_path); > >> - if (rc) goto out; > >> + if (fents || ro_fents) > >> + { > >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), > >> + frontend_path); > >> + if (rc) goto out; > >> + } > >> > >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), > >> - backend_path); > >> - if (rc) goto out; > >> + if (bents) > >> + { > >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), > >> + backend_path); > >> + if (rc) goto out; > >> + } > > > > What is this for? > > > > If there is no fe or be entries you skip the path creation altogether. > > But why? This doesn't seem to be related to your patch. > For vuart, I am adding only a front end node but the > libxl__device_generic_add() creates the backend path also,even though > there is no backend node. To remove that hanging be path, I added this > check. > > > > At least explain this a bit in the commit message? > I will add more details in the commit message. > Preferable it should be in a separate patch. That would make review easier. But there is another question: how do you know if Dom0 is servicing a DomU? How do you construct a libxl__device struct should you want to manipulate vuart? Wei. > Regards, > Bhupinder
Hi Wei, > [...] >> >> @@ -151,13 +154,19 @@ retry_transaction: >> >> if (rc) goto out; >> >> >> >> if (!libxl_only) { >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), >> >> - frontend_path); >> >> - if (rc) goto out; >> >> + if (fents || ro_fents) >> >> + { >> >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), >> >> + frontend_path); >> >> + if (rc) goto out; >> >> + } >> >> >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), >> >> - backend_path); >> >> - if (rc) goto out; >> >> + if (bents) >> >> + { >> >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), >> >> + backend_path); >> >> + if (rc) goto out; >> >> + } >> > >> > What is this for? >> > >> > If there is no fe or be entries you skip the path creation altogether. >> > But why? This doesn't seem to be related to your patch. >> For vuart, I am adding only a front end node but the >> libxl__device_generic_add() creates the backend path also,even though >> there is no backend node. To remove that hanging be path, I added this >> check. >> > >> > At least explain this a bit in the commit message? >> I will add more details in the commit message. >> > > Preferable it should be in a separate patch. That would make review > easier. > > But there is another question: how do you know if Dom0 is servicing a > DomU? How do you construct a libxl__device struct should you want to > manipulate vuart? > Can you please elaborate more on this question? I am adding the vuart console node at the same place where the PV console node is added. I believe, the check that I have added should be a generic check valid for any device creation. Regards, Bhupinder
On Mon, May 22, 2017 at 04:33:15PM +0530, Bhupinder Thakur wrote: > Hi Wei, > > > > [...] > >> >> @@ -151,13 +154,19 @@ retry_transaction: > >> >> if (rc) goto out; > >> >> > >> >> if (!libxl_only) { > >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), > >> >> - frontend_path); > >> >> - if (rc) goto out; > >> >> + if (fents || ro_fents) > >> >> + { > >> >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), > >> >> + frontend_path); > >> >> + if (rc) goto out; > >> >> + } > >> >> > >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), > >> >> - backend_path); > >> >> - if (rc) goto out; > >> >> + if (bents) > >> >> + { > >> >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), > >> >> + backend_path); > >> >> + if (rc) goto out; > >> >> + } > >> > > >> > What is this for? > >> > > >> > If there is no fe or be entries you skip the path creation altogether. > >> > But why? This doesn't seem to be related to your patch. > >> For vuart, I am adding only a front end node but the > >> libxl__device_generic_add() creates the backend path also,even though > >> there is no backend node. To remove that hanging be path, I added this > >> check. > >> > > >> > At least explain this a bit in the commit message? > >> I will add more details in the commit message. > >> > > > > Preferable it should be in a separate patch. That would make review > > easier. > > > > But there is another question: how do you know if Dom0 is servicing a > > DomU? How do you construct a libxl__device struct should you want to > > manipulate vuart? > > > Can you please elaborate more on this question? I am adding the vuart > console node at the same place where the PV console node is added. I > believe, the check that I have added should be a generic check valid > for any device creation. > Sorry if that question confuses you. Let me try to rephrase. Nowadays you can use xl block-list / network-list etc do list respective devices for a DomU. The underlying libxl functions normally rely on xenstore for device information. Now AIUI (but I could be wrong!) you don't have an backend entry for vuart in xenstore. In a hypothetical situation, we want to add a xl console-list to list all consoles for a DomU, where should libxl get relevant information from? Wei. > Regards, > Bhupinder
Hi Wei, Thanks for your explanation. >> >> >> @@ -151,13 +154,19 @@ retry_transaction: >> >> >> if (rc) goto out; >> >> >> >> >> >> if (!libxl_only) { >> >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), >> >> >> - frontend_path); >> >> >> - if (rc) goto out; >> >> >> + if (fents || ro_fents) >> >> >> + { >> >> >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), >> >> >> + frontend_path); >> >> >> + if (rc) goto out; >> >> >> + } >> >> >> >> >> >> - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), >> >> >> - backend_path); >> >> >> - if (rc) goto out; >> >> >> + if (bents) >> >> >> + { >> >> >> + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), >> >> >> + backend_path); >> >> >> + if (rc) goto out; >> >> >> + } >> >> > >> >> > What is this for? >> >> > >> >> > If there is no fe or be entries you skip the path creation altogether. >> >> > But why? This doesn't seem to be related to your patch. >> >> For vuart, I am adding only a front end node but the >> >> libxl__device_generic_add() creates the backend path also,even though >> >> there is no backend node. To remove that hanging be path, I added this >> >> check. >> >> > >> >> > At least explain this a bit in the commit message? >> >> I will add more details in the commit message. >> >> >> > >> > Preferable it should be in a separate patch. That would make review >> > easier. >> > >> > But there is another question: how do you know if Dom0 is servicing a >> > DomU? How do you construct a libxl__device struct should you want to >> > manipulate vuart? >> > >> Can you please elaborate more on this question? I am adding the vuart >> console node at the same place where the PV console node is added. I >> believe, the check that I have added should be a generic check valid >> for any device creation. >> > > Sorry if that question confuses you. Let me try to rephrase. > > Nowadays you can use xl block-list / network-list etc do list respective > devices for a DomU. The underlying libxl functions normally rely on > xenstore for device information. > > Now AIUI (but I could be wrong!) you don't have an backend entry for > vuart in xenstore. In a hypothetical situation, we want to add a xl > console-list to list all consoles for a DomU, where should libxl get > relevant information from? > I did not create a backend entry because I thought domU won't need it. But as you have mentioned, there are other consumers of the backend information (like xl) so I will add the backend information for vuart in xenstore. In that case, the checks that I have added in libxl__device_generic_add() are not required because there is no use case where a device is created with backend or frontend as null. Regards, Bhupinder
diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c index 853be15..870cb9c 100644 --- a/tools/libxl/libxl_console.c +++ b/tools/libxl/libxl_console.c @@ -344,6 +344,39 @@ out: return rc; } +int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid, + libxl__device_console *console, + libxl__domain_build_state *state, + libxl__device *device) +{ + flexarray_t *ro_front; + int rc; + + ro_front = flexarray_make(gc, 16, 1); + + device->backend_devid = console->devid; + device->backend_domid = console->backend_domid; + device->backend_kind = LIBXL__DEVICE_KIND_VUART; + device->devid = console->devid; + device->domid = domid; + device->kind = LIBXL__DEVICE_KIND_VUART; + + flexarray_append(ro_front, "port"); + flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port)); + flexarray_append(ro_front, "ring-ref"); + flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_gfn)); + flexarray_append(ro_front, "limit"); + flexarray_append(ro_front, GCSPRINTF("%d", LIBXL_XENCONSOLE_LIMIT)); + flexarray_append(ro_front, "type"); + flexarray_append(ro_front, "xenconsoled"); + + rc = libxl__device_generic_add(gc, XBT_NULL, device, + NULL, + NULL, + libxl__xs_kvs_of_flexarray(gc, ro_front)); + return rc; +} + int libxl__init_console_from_channel(libxl__gc *gc, libxl__device_console *console, int dev_num, diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 29daa35..39da2d0 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1332,10 +1332,18 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, switch (d_config->c_info.type) { case LIBXL_DOMAIN_TYPE_HVM: { - libxl__device_console console; - libxl__device device; + libxl__device_console console, vuart; + libxl__device device, vuart_device; libxl_device_vkb vkb; + if (d_config->b_info.vuart) + { + init_console_info(gc, &vuart, 0); + vuart.backend_domid = state->console_domid; + libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device); + libxl__device_console_dispose(&vuart); + } + init_console_info(gc, &console, 0); console.backend_domid = state->console_domid; libxl__device_console_add(gc, domid, &console, state, &device); @@ -1369,14 +1377,22 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, } case LIBXL_DOMAIN_TYPE_PV: { - libxl__device_console console; - libxl__device device; + libxl__device_console console, vuart; + libxl__device device, vuart_device; for (i = 0; i < d_config->num_vfbs; i++) { libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]); libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]); } + if (d_config->b_info.vuart) + { + init_console_info(gc, &vuart, 0); + vuart.backend_domid = state->console_domid; + libxl__device_vuart_add(gc, domid, &vuart, state, &vuart_device); + libxl__device_console_dispose(&vuart); + } + init_console_info(gc, &console, 0); console.backend_domid = state->console_domid; libxl__device_console_add(gc, domid, &console, state, &device); diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c index 5e96676..bb25672 100644 --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -26,6 +26,9 @@ static 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); + if (device->kind == LIBXL__DEVICE_KIND_VUART) + return GCSPRINTF("%s/vuart/%d", dom_path, device->devid); + return GCSPRINTF("%s/device/%s/%d", dom_path, libxl__device_kind_to_string(device->kind), device->devid); @@ -151,13 +154,19 @@ retry_transaction: if (rc) goto out; if (!libxl_only) { - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), - frontend_path); - if (rc) goto out; + if (fents || ro_fents) + { + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path), + frontend_path); + if (rc) goto out; + } - rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), - backend_path); - if (rc) goto out; + if (bents) + { + rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path), + backend_path); + if (rc) goto out; + } } /* xxx much of this function lacks error checks! */ @@ -170,14 +179,20 @@ retry_transaction: * historically contained other information, such as the * vnc-port, which we don't want the guest fiddling with. */ - if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0) + if ((device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0) || + (device->kind == LIBXL__DEVICE_KIND_VUART)) xs_set_permissions(ctx->xsh, t, frontend_path, ro_frontend_perms, ARRAY_SIZE(ro_frontend_perms)); else xs_set_permissions(ctx->xsh, t, frontend_path, frontend_perms, ARRAY_SIZE(frontend_perms)); - xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path), - backend_path, strlen(backend_path)); + /* + * Create backend node only if there are entries to be populated in the + * backend node. + */ + if (bents) + xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path), + backend_path, strlen(backend_path)); if (fents) libxl__xs_writev_perms(gc, t, frontend_path, fents, frontend_perms, ARRAY_SIZE(frontend_perms)); @@ -192,8 +207,13 @@ retry_transaction: xs_mkdir(ctx->xsh, t, backend_path); xs_set_permissions(ctx->xsh, t, backend_path, backend_perms, ARRAY_SIZE(backend_perms)); - xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path), - frontend_path, strlen(frontend_path)); + /* + * Create frontend node only if there are entries populated in the + * frontend node. + */ + if (fents || ro_fents) + xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path), + frontend_path, strlen(frontend_path)); libxl__xs_writev(gc, t, backend_path, bents); } @@ -800,7 +820,8 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs) dev->domid = domid; dev->kind = kind; dev->devid = atoi(devs[j]); - if (dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE) { + if (dev->backend_kind == LIBXL__DEVICE_KIND_CONSOLE || + dev->backend_kind == LIBXL__DEVICE_KIND_VUART) { /* Currently console devices can be destroyed * synchronously by just removing xenstore entries, * this is what libxl__device_destroy does. diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 4e2c247..7a22db0 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1202,6 +1202,10 @@ _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid, libxl__device_console *console, libxl__domain_build_state *state, libxl__device *device); +_hidden int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid, + libxl__device_console *console, + libxl__domain_build_state *state, + libxl__device *device); /* Returns 1 if device exists, 0 if not, ERROR_* (<0) on error. */ _hidden int libxl__device_exists(libxl__gc *gc, xs_transaction_t t, diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl index 7dc4d0f..c463c33 100644 --- a/tools/libxl/libxl_types_internal.idl +++ b/tools/libxl/libxl_types_internal.idl @@ -26,6 +26,7 @@ libxl__device_kind = Enumeration("device_kind", [ (9, "VUSB"), (10, "QUSB"), (11, "9PFS"), + (12, "VUART"), ]) libxl__console_backend = Enumeration("console_backend", [
Add a new vuart console node to xenstore. This node is added at /local/domain/$DOMID/vuart/0. The node contains information such as the ring-ref, event channel, buffer limit and type of console. Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- tools/libxl/libxl_console.c | 33 ++++++++++++++++++++++++++ tools/libxl/libxl_create.c | 24 +++++++++++++++---- tools/libxl/libxl_device.c | 45 ++++++++++++++++++++++++++---------- tools/libxl/libxl_internal.h | 4 ++++ tools/libxl/libxl_types_internal.idl | 1 + 5 files changed, 91 insertions(+), 16 deletions(-)