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
