Re: [PATCH RFC 08/31] tools/stubs: Expose host featureset to userspace

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-01-05 at 16:19 +0000, Andrew Cooper wrote:
> On 05/01/16 16:09, Ian Campbell wrote:
> > On Tue, 2016-01-05 at 15:59 +0000, Andrew Cooper wrote:
> > > On 05/01/16 15:36, Ian Campbell wrote:
> > > > On Wed, 2015-12-16 at 21:24 +0000, Andrew Cooper wrote:
> > > > > diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> > > > > index c613545..4d7af3d 100644
> > > > > --- a/tools/libxc/xc_misc.c
> > > > > +++ b/tools/libxc/xc_misc.c
> > > > > @@ -718,6 +718,33 @@ int xc_hvm_inject_trap(
> > > > >      return rc;
> > > > >  }
> > > > >  
> > > > > +int xc_get_featureset(xc_interface *xch, uint32_t index,
> > > > > +                      uint32_t *nr_features, uint32_t
> > > > > *featureset)
> > > > This looks like a valid binding to the hypercall iface, so once
> > > > that is
> > > > agreed this LGTM.
> > > > 
> > > > > diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > > > b/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > > > index b7de615..a47473b 100644
> > > > > --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > > > +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> > > > > @@ -1220,6 +1220,41 @@ CAMLprim value
> > > > > stub_xc_domain_deassign_device(value xch, value domid, value desc
> > > > >  	CAMLreturn(Val_unit);
> > > > >  }
> > > > >  
> > > > > +CAMLprim value stub_xc_get_featureset(value xch, value idx)
> > > > > +{
> > > > > +	CAMLparam2(xch, idx);
> > > > > +	CAMLlocal1(bitmap_val);
> > > > > +
> > > > > +	/* Safe, because of the global ocaml lock. */
> > > > > +	static uint32_t fs_len;
> > > > > +
> > > > > +	if (fs_len == 0)
> > > > > +	{
> > > > > +		int ret = xc_get_featureset(_H(xch), 0, &fs_len,
> > > > > NULL);
> > > > > +
> > > > > +		if (ret || (fs_len == 0))
> > > > > +			failwith_xc(_H(xch));
> > > > > +	}
> > > > This confuses me because I had thought when reading the previous
> > > > patch
> > > > that
> > > > the output nr_features would depend on the specific index, is that
> > > > not
> > > > the
> > > > case? Maybe this is just a hypercall docs fix?
> > > nr_features is compile-time-constant in Xen, and every featureset
> > > handed
> > > back will be of that length.
> > IOW if the featureset given to the hypercall is NULL then index is
> > ignored?
> > Worth documenting.
> 
> From the previous patch,
> 
> +struct xen_sysctl_featureset {
> +#define XEN_SYSCTL_featureset_host      0
> +    uint32_t index;       /* IN: Which featureset to query? */
> +    uint32_t nr_features; /* IN/OUT: Number of entries in/written to
> +                           * 'features', or the maximum number of features if
> +                           * the guest handle is NULL. */
> +    XEN_GUEST_HANDLE_64(uint32) features; /* OUT: */
> 
> 
> This is the usual documentation for this behaviour.  Is that enough?

It's not clear from this that the returned nr_features is independent of
the index (i..e a caller might infer they need to call it again and again
for each index).

I'm not sure if that is true of other similar hypercall's implementation or
documentation though.


> > 
> > > It is expected to grow given new changes to Xen, but won't change at
> > > runtime.
> > And we never expect any index to need a different length to all the
> > others?
> 
> All bitmaps returned use bits from the same numberspace, which are going
> to have the same maximum potential length.

Good, thanks.

Ian.

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