From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Mukesh Rathor <mukesh.rathor@oracle.com>
Cc: "Xen-devel@lists.xensource.com" <Xen-devel@lists.xensource.com>
Subject: Re: [PATCH 7/18 V2]: PVH xen: tools changes to create PVH domain
Date: Mon, 18 Mar 2013 10:22:25 -0400 [thread overview]
Message-ID: <20130318142225.GH24560@phenom.dumpdata.com> (raw)
In-Reply-To: <20130315173423.0ae6987d@mantra.us.oracle.com>
On Fri, Mar 15, 2013 at 05:34:23PM -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
Changes.
Two nit-picks below.
Otherwise 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>'
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
> 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 | 14 ++++++++------
> xen/include/public/domctl.h | 3 +++
> 10 files changed, 54 insertions(+), 12 deletions(-)
You are missing the changes to docs/man/xl.cfg.pod.5 to document
the 'pvh' boolean flag.
>
> diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
> index 64c7484..5736b86 100644
> --- a/tools/debugger/gdbsx/xg/xg_main.c
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -81,6 +81,7 @@ int xgtrc_on = 0;
> struct xen_domctl domctl; /* just use a global domctl */
>
> static int _hvm_guest; /* hvm guest? 32bit HVMs have 64bit context */
> +static int _pvh_guest; /* PV guest in HVM container */
> static domid_t _dom_id; /* guest domid */
> static int _max_vcpu_id; /* thus max_vcpu_id+1 VCPUs */
> static int _dom0_fd; /* fd of /dev/privcmd */
> @@ -309,6 +310,7 @@ xg_attach(int domid, int guest_bitness)
>
> _max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id;
> _hvm_guest = (domctl.u.getdomaininfo.flags & XEN_DOMINF_hvm_guest);
> + _pvh_guest = (domctl.u.getdomaininfo.flags & XEN_DOMINF_pvh_guest);
> return _max_vcpu_id;
> }
>
> @@ -369,7 +371,7 @@ _change_TF(vcpuid_t which_vcpu, int guest_bitness, int setit)
> int sz = sizeof(anyc);
>
> /* first try the MTF for hvm guest. otherwise do manually */
> - if (_hvm_guest) {
> + if (_hvm_guest || _pvh_guest) {
> domctl.u.debug_op.vcpu = which_vcpu;
> domctl.u.debug_op.op = setit ? XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON :
> XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF;
> diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
> index 779b9d4..e8a5260 100644
> --- a/tools/libxc/xc_dom.h
> +++ b/tools/libxc/xc_dom.h
> @@ -130,6 +130,7 @@ struct xc_dom_image {
> domid_t console_domid;
> domid_t xenstore_domid;
> xen_pfn_t shared_info_mfn;
> + int domcr_is_pvh;
How about 'pvh_enabled'? Could you also attach a little blurb in
a comment ? Perhaps saying:
/* PVH means no P2M, which means no page-tables for the guest
in the toolstack (it is all done in the hypervisor via
HAP. Also grant setup is done the HVM-ish way. */
>
> xc_interface *xch;
> domid_t guest_domid;
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index eb9ac07..ca1bc95 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -355,7 +355,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
> pgpfn = (addr - dom->parms.virt_base) >> PAGE_SHIFT_X86;
> l1tab[l1off] =
> pfn_to_paddr(xc_dom_p2m_guest(dom, pgpfn)) | L1_PROT;
> - if ( (addr >= dom->pgtables_seg.vstart) &&
> + if ( (!dom->domcr_is_pvh) &&
> + (addr >= dom->pgtables_seg.vstart) &&
> (addr < dom->pgtables_seg.vend) )
> l1tab[l1off] &= ~_PAGE_RW; /* page tables are r/o */
> if ( l1off == (L1_PAGETABLE_ENTRIES_X86_64 - 1) )
> @@ -672,7 +673,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> rc = x86_compat(dom->xch, dom->guest_domid, dom->guest_type);
> if ( rc )
> return rc;
> - if ( xc_dom_feature_translated(dom) )
> + if ( xc_dom_feature_translated(dom) && !dom->domcr_is_pvh )
> {
> dom->shadow_enabled = 1;
> rc = x86_shadow(dom->xch, dom->guest_domid);
> @@ -786,7 +787,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->domcr_is_pvh; i++ )
> {
> rc = xc_domain_add_to_physmap(dom->xch, dom->guest_domid,
> XENMAPSPACE_grant_table,
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index efeebf2..7f96dbd 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -405,6 +405,8 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info,
> flags |= XEN_DOMCTL_CDF_hvm_guest;
> flags |= libxl_defbool_val(info->hap) ? XEN_DOMCTL_CDF_hap : 0;
> flags |= libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
> + } else if ( libxl_defbool_val(info->ci_pvh) ) {
> + flags |= XEN_DOMCTL_CDF_hap;
> }
> *domid = -1;
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index de555ee..4b23cf4 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -322,9 +322,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->bi_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);
> + }
> +
> dom = xc_dom_allocate(ctx->xch, state->pv_cmdline, info->u.pv.features);
> if (!dom) {
> LOGE(ERROR, "xc_dom_allocate failed");
> @@ -363,6 +377,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> }
>
> dom->flags = flags;
> + dom->domcr_is_pvh = is_pvh;
> dom->console_evtchn = state->console_port;
> dom->console_domid = state->console_domid;
> dom->xenstore_evtchn = state->store_port;
> @@ -392,7 +407,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 5b080ed..ae11309 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -244,6 +244,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
> ("platformdata", libxl_key_value_list),
> ("poolid", uint32),
> ("run_hotplug_scripts",libxl_defbool),
> + ("ci_pvh", libxl_defbool),
No need for the 'ci'. When you start searching in the code it is obvious
that you using either b_info or c_info.
> ], dir=DIR_IN)
>
> MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
> @@ -343,6 +344,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ])),
> ("invalid", Struct(None, [])),
> ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
> + ("bi_pvh", libxl_defbool),
Ditto. Just make it 'pvh'
> ], dir=DIR_IN
> )
>
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index a17f6ae..3caba5c 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.bi_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 a98705e..788aa4a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -633,8 +633,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->ci_pvh, false);
> + libxl_defbool_setdefault(&c_info->hap, false);
> + xlu_cfg_get_defbool(config, "pvh", &c_info->ci_pvh, 0);
> xlu_cfg_get_defbool(config, "hap", &c_info->hap, 0);
>
> + if (libxl_defbool_val(c_info->ci_pvh) &&
> + !libxl_defbool_val(c_info->hap)) {
> +
> + fprintf(stderr, "hap is required for PVH domain\n");
> + exit(1);
> + }
> +
> if (xlu_cfg_replace_string (config, "name", &c_info->name, 0)) {
> fprintf(stderr, "Domain name must be specified.\n");
> exit(1);
> @@ -939,6 +949,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->bi_pvh, libxl_defbool_val(c_info->ci_pvh));
> break;
> }
> default:
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index bf83d58..6b7b986 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)
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 113b8dc..a6241ef 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -89,6 +89,9 @@ struct xen_domctl_getdomaininfo {
> /* Being debugged. */
> #define _XEN_DOMINF_debugged 6
> #define XEN_DOMINF_debugged (1U<<_XEN_DOMINF_debugged)
> + /* domain is PVH */
> +#define _XEN_DOMINF_pvh_guest 7
> +#define XEN_DOMINF_pvh_guest (1U<<_XEN_DOMINF_pvh_guest)
> /* XEN_DOMINF_shutdown guest-supplied code. */
> #define XEN_DOMINF_shutdownmask 255
> #define XEN_DOMINF_shutdownshift 16
> --
> 1.7.2.3
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
prev parent reply other threads:[~2013-03-18 14:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-16 0:34 [PATCH 7/18 V2]: PVH xen: tools changes to create PVH domain Mukesh Rathor
2013-03-18 14:22 ` Konrad Rzeszutek Wilk [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130318142225.GH24560@phenom.dumpdata.com \
--to=konrad.wilk@oracle.com \
--cc=Xen-devel@lists.xensource.com \
--cc=mukesh.rathor@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.