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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Apr 24, 2013 at 09:57:28AM +0100, Ian Campbell wrote:
[...]
> > +     *
> > +     * 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?

My question, does Linux prior to 2.6.26 support PV on HVM?

> 
> 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.
> 

I did use arch_hooks.native_protocol in my first implementation but
later found out that it is impossible to get this value when doing
restore. I also tried to avoid altering existing restore API as you
pointed out we are now close to release. So I just came up with exposing
guest width to libxl. :-(

> > +     */
> > +
> > +    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)
> 

I think querying when adding disk is a good idea.

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

It does this for all devices, but only very old disk frontend relies on
this AFAICT.


Wei.

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