|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 08/18] PVH xen: tools changes to create PVH domain
On Fri, 2013-05-24 at 18:25 -0700, Mukesh Rathor wrote:
> This patch contains tools changes for PVH. For now, only one mode is
> supported/tested:
> dom0> losetup /dev/loop1 guest.img
> dom0> In vm.cfg file: disk = ['phy:/dev/loop1,xvda,w']
>
> Chnages in V2: None
> Chnages in V3:
> - Document pvh boolean flag in xl.cfg.pod.5
> - Rename ci_pvh and bi_pvh to pvh, and domcr_is_pvh to pvh_enabled.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
> docs/man/xl.cfg.pod.5 | 3 +++
> tools/debugger/gdbsx/xg/xg_main.c | 4 +++-
> tools/libxc/xc_dom.h | 1 +
> tools/libxc/xc_dom_x86.c | 7 ++++---
> tools/libxl/libxl_create.c | 2 ++
> tools/libxl/libxl_dom.c | 18 +++++++++++++++++-
> tools/libxl/libxl_types.idl | 2 ++
> tools/libxl/libxl_x86.c | 4 +++-
> tools/libxl/xl_cmdimpl.c | 11 +++++++++++
> tools/xenstore/xenstored_domain.c | 12 +++++++-----
I think these should be split into
libxc (dombuilder) changes
libxl changes
xenstore changes
misc other (== gdbsx) changes.
Since most of the changes here are not mentioned at all in the commit
message (it was the xenstore change, which IMHO requires an explanation,
which prompted this)
> 10 files changed, 53 insertions(+), 11 deletions(-)
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index f1be43b..24f6759 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -832,7 +833,7 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
> }
>
> /* Map grant table frames into guest physmap. */
> - for ( i = 0; ; i++ )
> + for ( i = 0; !dom->pvh_enabled; i++ )
This is a bit of an odd way to do this (unless pvh_enabled somehow
changes in this loop, which I doubt). Can we just get a surrounding if
please.
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index b38d0a7..cefbf76 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -329,9 +329,23 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> struct xc_dom_image *dom;
> int ret;
> int flags = 0;
> + int is_pvh = libxl_defbool_val(info->pvh);
>
> xc_dom_loginit(ctx->xch);
>
> + if (is_pvh) {
> + char *pv_feats = "writable_descriptor_tables|auto_translated_physmap"
> + "|supervisor_mode_kernel|hvm_callback_vector";
> +
> + if (info->u.pv.features && info->u.pv.features[0] != '\0')
> + {
> + LOG(ERROR, "Didn't expect info->u.pv.features to contain
> string\n");
> + LOG(ERROR, "String: %s\n", info->u.pv.features);
> + return ERROR_FAIL;
> + }
> + info->u.pv.features = strdup(pv_feats);
What is this trying to achieve? I think the requirement for certain
features to be present if pvh is enabled needs to be handled in the
xc_dom library and not here. This field is (I think) for the user to
specify other features which they may wish to require.
> + }
> +
> dom = xc_dom_allocate(ctx->xch, state->pv_cmdline, info->u.pv.features);
> if (!dom) {
> LOGE(ERROR, "xc_dom_allocate failed");
> @@ -370,6 +384,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> }
>
> dom->flags = flags;
> + dom->pvh_enabled = is_pvh;
Not part of the flags?
> dom->console_evtchn = state->console_port;
> dom->console_domid = state->console_domid;
> dom->xenstore_evtchn = state->store_port;
> @@ -400,7 +415,8 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> LOGE(ERROR, "xc_dom_boot_image failed");
> goto out;
> }
> - if ( (ret = xc_dom_gnttab_init(dom)) != 0 ) {
> + /* PVH sets up its own grant during boot via hvm mechanisms */
> + if ( !is_pvh && (ret = xc_dom_gnttab_init(dom)) != 0 ) {
> LOGE(ERROR, "xc_dom_gnttab_init failed");
> goto out;
> }
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 8262cba..43e6d95 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -245,6 +245,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
> ("platformdata", libxl_key_value_list),
> ("poolid", uint32),
> ("run_hotplug_scripts",libxl_defbool),
> + ("pvh", libxl_defbool),
> ], dir=DIR_IN)
>
> MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
> @@ -346,6 +347,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ])),
> ("invalid", Struct(None, [])),
> ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
> + ("pvh", libxl_defbool),
I'm not quite convinced if the need for both of these bools in both
create and build, it's a bit of an odd quirk in our API which I need to
consider a bit deeper.
In any case if this one in build_info should exist it belongs in the pv
part of the preceding union since it is PV specific.
> ], dir=DIR_IN
> )
>
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index a17f6ae..424bc68 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -290,7 +290,9 @@ int libxl__arch_domain_create(libxl__gc *gc,
> libxl_domain_config *d_config,
> if (rtc_timeoffset)
> xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
>
> - if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM) {
> + if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM ||
> + libxl_defbool_val(d_config->b_info.pvh)) {
> +
> unsigned long shadow;
> shadow = (d_config->b_info.shadow_memkb + 1023) / 1024;
> xc_shadow_control(ctx->xch, domid,
> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, NULL, 0, &shadow, 0, NULL);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e13a64e..e032668 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -610,8 +610,18 @@ static void parse_config_data(const char *config_source,
> !strncmp(buf, "hvm", strlen(buf)))
> c_info->type = LIBXL_DOMAIN_TYPE_HVM;
>
> + libxl_defbool_setdefault(&c_info->pvh, false);
> + libxl_defbool_setdefault(&c_info->hap, false);
These belong in libxl__domain_create_info_setdefault() not here.
> + xlu_cfg_get_defbool(config, "pvh", &c_info->pvh, 0);
> xlu_cfg_get_defbool(config, "hap", &c_info->hap, 0);
>
> + if (libxl_defbool_val(c_info->pvh) &&
> + !libxl_defbool_val(c_info->hap)) {
> +
> + fprintf(stderr, "hap is required for PVH domain\n");
> + exit(1);
This check belongs in setdefault or one of the functions in libxl which
consumes the create_info
> + }
> +
> if (xlu_cfg_replace_string (config, "name", &c_info->name, 0)) {
> fprintf(stderr, "Domain name must be specified.\n");
> exit(1);
> @@ -918,6 +928,7 @@ static void parse_config_data(const char *config_source,
>
> b_info->u.pv.cmdline = cmdline;
> xlu_cfg_replace_string (config, "ramdisk", &b_info->u.pv.ramdisk, 0);
> + libxl_defbool_set(&b_info->pvh, libxl_defbool_val(c_info->pvh));
b_info->pvh = c_info->pvh is the right way to do this. If possible I'd
like to remove one or the other from and handle it internally to the
library. As I say I need to chew on this one a bit more.
> break;
> }
> default:
> diff --git a/tools/xenstore/xenstored_domain.c
> b/tools/xenstore/xenstored_domain.c
> index bf83d58..10c23a1 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -168,13 +168,15 @@ static int readchn(struct connection *conn, void *data,
> unsigned int len)
> static void *map_interface(domid_t domid, unsigned long mfn)
> {
> if (*xcg_handle != NULL) {
> - /* this is the preferred method */
> - return xc_gnttab_map_grant_ref(*xcg_handle, domid,
> + void *addr;
> + /* this is the preferred method */
> + addr = xc_gnttab_map_grant_ref(*xcg_handle, domid,
> GNTTAB_RESERVED_XENSTORE, PROT_READ|PROT_WRITE);
> - } else {
> - return xc_map_foreign_range(*xc_handle, domid,
> - getpagesize(), PROT_READ|PROT_WRITE, mfn);
> + if (addr)
> + return addr;
> }
> + return xc_map_foreign_range(*xc_handle, domid,
> + getpagesize(), PROT_READ|PROT_WRITE, mfn);
> }
>
> static void unmap_interface(void *interface)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |