Re: [Xen-devel] [PATCH] libxl: write IO ABI for disk frontends

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

> @@ -910,6 +911,54 @@ static void domcreate_rebuild_done(libxl__egc *egc,
>          goto error_out;
>      }
>  
> +#if defined (__i386__) || (__x86_64__)
> +    /*
> +     * Pass along the disk navtive protocol to disks so that the protocol can

                              native

> +     * be written to frontend Xenstore node. This is a workaround for old PV
> +     * kernels before 2.6.26.
> +     *
> +     * Newer blkfront will write that node itself. In that case frontend just
> +     * rewrites the node as it sees fit.
> +     *
> +     * Note that Xend actually propagate this vaule to all frontends 
> including

                                                 value

> +     * console and vifs. As now this is only needed for disk frontend so here
> +     * we minimize the impact.
> +     *
> +     * Presumably ARM guests don't have this problem, so this snippet is only
> +     * compiled for X86 target.

Yes, this isn't required on ARM.

Is there a reason why this is only done for PV guests? I'd expect at
least some older versions of the PV on HVM drivers to also need this
node. What did Xend do in this regard?

I think it would be preferable to have the logic for determining the
protocol in libxc, rather than exposing the guest width and putting the
logic in libxl. Mostly because on domain build it is also libxc which
makes this determination and keeping it in the same library makes sense
to me.

What I'd actually prefer is to use the value of
dom->arch_hooks.native_protocol in libxl__build_pv when building a new
domain and have xc_domain_restore include the protocol as an output when
restoring so that libxl can use that in the restore case. This might be
more plumbing than we are willing to do at this stage of the release
though, so if you remove the bits which expose the protocol in the libxl
API (see below) and make this purely internal functionality of libxl+
libxc we could live with an explicit query for the protocol.

> +     */
> +
> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV) {
> +        int i;
> +        uint32_t guest_width;
> +        const char *protocol = NULL;
> +
> +        ret = xc_domain_getwidth(CTX->xch, domid, &guest_width);
> +        if (ret) {
> +            ret = ERROR_FAIL;
> +            goto error_out;
> +        }
> +
> +        switch (guest_width) {
> +        case 32: /* 32 bit guest */
> +            protocol = XEN_IO_PROTO_ABI_X86_32;
> +            break;
> +        case 64: /* 64 bit guest */
> +            protocol = XEN_IO_PROTO_ABI_X86_64;
> +            break;
> +        default:
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                       "invalid address size for domain: %u",
> +                       guest_width);
> +            ret = ERROR_FAIL;
> +            goto error_out;
> +        }
> +
> +        for (i = 0; i < d_config->num_disks; i++)
> +            d_config->disks[i].native_protocol = strdup(protocol);

If you are going to expose native_protocol to the libxl user then you
need to allow for them to have set it.

However I don't think we want/need to expose this to the user,
device_disk_add() can just query and write the string in one go. Or it
could be queried once for the domain early on and remembered in some
handy place (either a suitable datastructure or in xenstore)

Did Xend only do this for disks, or did it do it for all devices?

> +    }
> +#endif
> +
>      store_libxl_entry(gc, domid, &d_config->b_info);
>  
>      libxl__multidev_begin(ao, &dcs->multidev);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 6cb6de6..22622a9 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -274,7 +274,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("disable_migrate", libxl_defbool),
>      ("cpuid",           libxl_cpuid_policy_list),
>      ("blkdev_start",    string),
> -    
> +
>      ("device_model_version", libxl_device_model_version),
>      ("device_model_stubdomain", libxl_defbool),
>      # if you set device_model you must set device_model_version too
> @@ -318,9 +318,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("keymap",           string),
>                                         ("sdl",              libxl_sdl_info),
>                                         ("spice",            
> libxl_spice_info),
> -                                       
> +
>                                         ("gfx_passthru",     libxl_defbool),
> -                                       
> +
>                                         ("serial",           string),
>                                         ("boot",             string),
>                                         ("usb",              libxl_defbool),
> @@ -371,6 +371,7 @@ libxl_device_disk = Struct("device_disk", [
>      ("removable", integer),
>      ("readwrite", integer),
>      ("is_cdrom", integer),
> +    ("native_protocol", string),
>      ])
>  
>  libxl_device_nic = Struct("device_nic", [



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