Re: REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote:
> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
> > On 08.04.16 at 19:41, <andrew.cooper3@citrix.com> wrote:
> > > The interface for the old version was sufficiently useless that build_id
> > > can't be added to it.  (Specifically, there is no ability to return
> > > varialble length data).
> > 
> > This is simply not true: The hypercall being passed a void handle,
> > everything can be arranged for without introducing a new
> > hypercall.
> 
> I'm trying to understand what your alternative suggestion is.  Could
> you please be more explicit ?
> 
> I'm reading xen/include/public/version.h.  (Sadly it doesn't have the
> API doc markup.)  AFAICT there is a XENVER_xxxx sub-op namespace which
> permits extensions to the XENVER hypercall.
> 
> But does that permit the caller to specify their buffer size ?

Only via the sub-op parameters itself. As in you cannot expand the
amount of parameters the XENVER_hypercall can do (which is two - subops
number and the pointer to arguments).
> 
> > > The new hypercall has a ration interface where you don't blindly trust
> > > that the caller passed you a pointer to a suitably-sized structure.
> 
> Ie I think ^ this is for me a key question.
> 
> > While the new one is indeed slightly neater, that's not sufficient
> > for such redundancy imo. That's the whole reason for withdrawing
> > my ack _without_ making it an explicit NAK.
> 
> I don't think I would be content with simply adding a new sub-op with
> bigger fixed-length fields.

It was variable-ish.

The new-subop (xsplice.v3) ended up 'lets-try-this-size' subop. Meaning:

/* Return value is the number of bytes written, or XEN_Exx on error.
 * Calling with empty parameter returns the size of build_id. */

#define XENVER_build_id 10
struct xen_build_id {
        uint32_t        len; /* IN: size of buf[]. */
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
        unsigned char   buf[];
#elif defined(__GNUC__)
        unsigned char   buf[1]; /* OUT: Variable length buffer with build_id. */
#endif
};

When constructing this in libxl we could do:

 	xen_build_id_t *build_id;

 	ret= xc_version(ctx->xch, XENVER_build_id, NULL);
	if ( ret != -ENOSYS && ret > 0 )
            build_id = libxl__zalloc(gc, ret + sizeof(*build_id));
            build_id->len = info->pagesize - sizeof(*build_id);
 	    ret= xc_version(ctx->xch, XENVER_build_id, build_id);
            .. parse it...
        }

For the default case, ret would be 20, which meant the structure had
to be 24 bytes. 4 bytes were for 'len' (which had the value of 20)
and the rest inside the build_id were to be filled with the hypervisor.

For glory details:
http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commitdiff;h=63681414ede6e38c1910077d7a225e0d67f0ff2e;hp=37efc2ac64b9ecf0cd49fb65aa7c7659467c9318
and
http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commitdiff;h=176c63f3c0db430c70c28fe81cb8d039ae459c66;hp=63681414ede6e38c1910077d7a225e0d67f0ff2e

(albeit we first tried with the default 1020 bytes we have on the stack).

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