[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 7/18 V2]: PVH xen: tools changes to create PVH domain



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@xxxxxxxxxx>'

> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.