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

From: "Durrant, Paul" <pdurrant@amazon.co.uk>
To: "jandryuk@gmail.com" <jandryuk@gmail.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Wei Liu <wl@xen.org>
Subject: Re: [Xen-devel] [PATCH v2 4/6] libxl: allow creation of domains with a specified or random domid
Date: Tue, 14 Jan 2020 10:05:00 +0000	[thread overview]
Message-ID: <2a82b63d1ecd42e1ae1fd9be37422055@EX13D32EUC003.ant.amazon.com> (raw)
In-Reply-To: <CAKf6xpsz9ee4=15CApvX1XOkXRrZj2qHkgnpf7g836UBLi4-Ww@mail.gmail.com>

> -----Original Message-----
> From: jandryuk@gmail.com <jandryuk@gmail.com>
> Sent: 13 January 2020 19:35
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel <xen-devel@lists.xenproject.org>; Anthony PERARD
> <anthony.perard@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei
> Liu <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH v2 4/6] libxl: allow creation of domains
> with a specified or random domid
> 
> On Mon, Jan 13, 2020 at 11:55 AM Durrant, Paul <pdurrant@amazon.co.uk>
> wrote:
> >
> > > -----Original Message-----
> > > From: jandryuk@gmail.com <jandryuk@gmail.com>
> > > Sent: 13 January 2020 16:16
> > > To: Durrant, Paul <pdurrant@amazon.co.uk>
> > > Cc: xen-devel <xen-devel@lists.xenproject.org>; Anthony PERARD
> > > <anthony.perard@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>;
> Wei
> > > Liu <wl@xen.org>
> > > 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@amazon.com>
> 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@amazon.com>
> > > > ---
> > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > Cc: Wei Liu <wl@xen.org>
> > > > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > > >
> > > > 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 :)
> 

Perhaps I should try adding a DOMID_MASK definition somewhere.

  Paul

> Regards,
> Jason
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel