Re: [Xen-devel] [RFC PATCH 7/16]: PVH xen: User space changes to create a PVH domain

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Sat, 12 Jan 2013, Mukesh Rathor wrote:
> In this patch, I introduce xl/xc changes to create a PVH domain. For
> now, only one mode is supported/testd:
>    > losetup /dev/loop1 guest.img
>    > In vm.cfg file: disk = ['phy:/dev/loop1,xvda,w']  
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> 
> diff -r 5af39353f3f9 -r 956e77de73e4 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Fri Jan 11 16:27:46 2013 -0800
> +++ b/tools/libxl/libxl_dom.c Fri Jan 11 16:29:49 2013 -0800
> @@ -270,7 +270,8 @@ int libxl__build_pre(libxl__gc *gc, uint
>      if (rtc_timeoffset)
>          xc_domain_set_time_offset(ctx->xch, domid, rtc_timeoffset);
>  
> -    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM ||
> +        libxl_defbool_val(info->bi_pvh) ) {
>          unsigned long shadow;
>          shadow = (info->shadow_memkb + 1023) / 1024;
>          xc_shadow_control(ctx->xch, domid, 
> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, NULL, 0, &shadow, 0, NULL);
> @@ -368,9 +369,23 @@ int libxl__build_pv(libxl__gc *gc, uint3
>      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) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xc_dom_allocate failed");
> @@ -409,6 +424,7 @@ int libxl__build_pv(libxl__gc *gc, uint3
>      }
>  
>      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;
> @@ -438,7 +454,8 @@ int libxl__build_pv(libxl__gc *gc, uint3
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_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 ) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xc_dom_gnttab_init failed");
>          goto out;
>      }

I have the same problem on ARM and I have just managed to fix it.
Provided that gnttab_create_shared_page and gnttab_shared_gmfn are
implemented correctly for PVH guests, I think that you just need to call
xc_dom_gnttab_hvm_seed instead of xc_dom_gnttab_init.

Keep an eye on the ARM patch series that I am about to send: it is going
to conflict with this patch, but it should sort out this issue for PVH
too.


> diff -r 5af39353f3f9 -r 956e77de73e4 tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl     Fri Jan 11 16:27:46 2013 -0800
> +++ b/tools/libxl/libxl_types.idl     Fri Jan 11 16:29:49 2013 -0800
> @@ -243,6 +243,7 @@ libxl_domain_create_info = Struct("domai
>      ("platformdata", libxl_key_value_list),
>      ("poolid",       uint32),
>      ("run_hotplug_scripts",libxl_defbool),
> +    ("ci_pvh",       libxl_defbool),
>      ], dir=DIR_IN)
>  
>  MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
> @@ -339,6 +340,7 @@ libxl_domain_build_info = Struct("domain
>                                        ])),
>                   ("invalid", Struct(None, [])),
>                   ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
> +    ("bi_pvh",       libxl_defbool),
>      ], dir=DIR_IN
>  )
>  
> diff -r 5af39353f3f9 -r 956e77de73e4 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c        Fri Jan 11 16:27:46 2013 -0800
> +++ b/tools/libxl/xl_cmdimpl.c        Fri Jan 11 16:29:49 2013 -0800
> @@ -615,8 +615,18 @@ static void parse_config_data(const char
>          !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);
> @@ -916,6 +926,7 @@ static void parse_config_data(const char
>  
>          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:

I think that it is time to add a set of arch-specific options in the IDL
and in the config file, so that we don't have to parse PVH and HAP on
ARM, where they are meaningless.


> diff -r 5af39353f3f9 -r 956e77de73e4 tools/xenstore/xenstored_domain.c
> --- a/tools/xenstore/xenstored_domain.c       Fri Jan 11 16:27:46 2013 -0800
> +++ b/tools/xenstore/xenstored_domain.c       Fri Jan 11 16:29:49 2013 -0800
> @@ -168,13 +168,15 @@ static int readchn(struct connection *co
>  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)

Even though I think I managed to fix this problem for ARM and PVH, I
still welcome this change.

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