Re: [Xen-devel] [PATCH v2 4/6] libxl: allow creation of domains with a specified or random domid

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Jan 13, 2020 at 11:55 AM Durrant, Paul <pdurrant@xxxxxxxxxxxx> wrote:
>
> > -----Original Message-----
> > From: jandryuk@xxxxxxxxx <jandryuk@xxxxxxxxx>
> > Sent: 13 January 2020 16:16
> > To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> > Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Anthony PERARD
> > <anthony.perard@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Wei
> > Liu <wl@xxxxxxx>
> > Subject: Re: [Xen-devel] [PATCH v2 4/6] libxl: allow creation of domains
> > with a specified or random domid
> >
> > On Thu, Jan 9, 2020 at 6:50 AM Paul Durrant <pdurrant@xxxxxxxxxx> wrote:
> > >
> > > This patch adds a 'domid' field to libxl_domain_create_info and then
> > > modifies do_domain_create() to use that value if it is valid. Any valid
> > > domid will be checked against the retired domid list before being passed
> > > to libxl__domain_make().
> > > If the domid value is invalid then Xen will choose the domid, as before,
> > > unless the value is the new special RANDOM_DOMID value added to the API.
> > > This value instructs libxl__domain_make() to select a random domid
> > value,
> > > check it for validity, verify it does not match a retired domain, and
> > then
> > > pass it to Xen's XEN_DOMCTL_createdomain operation. If Xen determines
> > that
> > > it co-incides with an existing domain, a new random value will be
> > > selected and the operation will be re-tried.
> > >
> > > NOTE: libxl__logv() is also modified to only log valid domid values in
> > >       messages rather than any domid, valid or otherwise, that is not
> > >       INVALID_DOMID.
> > >
> > > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > > ---
> > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > Cc: Wei Liu <wl@xxxxxxx>
> > > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > >
> > > v2:
> > >  - Re-worked to use a value from libxl_domain_create_info
> > > ---
> > >  tools/libxl/libxl.h          |  9 +++++++++
> > >  tools/libxl/libxl_create.c   | 32 +++++++++++++++++++++++++++++++-
> > >  tools/libxl/libxl_internal.c |  2 +-
> > >  tools/libxl/libxl_types.idl  |  1 +
> > >  4 files changed, 42 insertions(+), 2 deletions(-)
> > >
> >
> > <snip>
> >
> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > index 1835a5502c..ee76dee364 100644
> > > --- a/tools/libxl/libxl_create.c
> > > +++ b/tools/libxl/libxl_create.c
> > > @@ -600,9 +600,39 @@ int libxl__domain_make(libxl__gc *gc,
> > libxl_domain_config *d_config,
> > >              goto out;
> > >          }
> > >
> > > -        ret = xc_domain_create(ctx->xch, domid, &create);
> > > +        if (libxl_domid_valid_guest(info->domid)) {
> > > +            *domid = info->domid;
> > > +
> > > +            if (libxl__is_retired_domid(gc, *domid)) {
> > > +                LOGED(ERROR, *domid, "domain id is retired");
> > > +                rc = ERROR_FAIL;
> > > +                goto out;
> > > +            }
> > > +        } else if (info->domid == RANDOM_DOMID) {
> > > +            *domid = 0; /* Zero-out initial value */
> > > +        }
> > > +
> > > +        for (;;) {
> > > +            if (info->domid == RANDOM_DOMID) {
> > > +                /* Randomize lower order bytes */
> > > +                ret = libxl__random_bytes(gc, (void *)domid,
> > > +                                          sizeof(uint16_t));
> >
> > Casting to void * assumes little endian.
>
> I think that's a fairly safe assumption as far as Xen goes...
>
> > Using a temporary uint16_t
>
> ...but, yes, that might be neater.
>
> > would avoid that assumption.  Also, masking down to 0x7fff would clear
> > the top bit which is never valid.
>
> That seems like a bit of a layering violation and the check in 
> libxl_domid_valid_guest() is going to cause a pretty fast turn round the loop 
> if the top bit is set so masking is not going to gain that much.

Yeah, there isn't a define or constant exposed for 0x7fff, so masking
is a little dirty.  Since about ~half of random 16bit numbers will
have the high bit set, we'll have to read a second one.  My natural
instinct is to avoid those extra reads :)

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel