Re: [PATCH v14 3/3] xen/tools: VM forking toolstack side

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Thu, Apr 9, 2020 at 9:43 AM Wei Liu <wl@xxxxxxx> wrote:
>
> On Mon, Apr 06, 2020 at 08:20:05AM -0700, Tamas K Lengyel wrote:
> > Add necessary bits to implement "xl fork-vm" commands. The command allows 
> > the
> > user to specify how to launch the device model allowing for a late-launch 
> > model
> > in which the user can execute the fork without the device model and decide 
> > to
> > only later launch it.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > ---
> [...]
> >
> > +int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> > +                       libxl__domain_build_state *state,
> > +                       uint32_t *domid, bool soft_reset)
>
> It would have been nice if you could split the refactoring out to a
> separate patch.

I found the toolstack to be way harder to work with then the
hypervisor code-base. Splitting the patches would have been nice but I
don't even know how to begin that since it's all such a spaghetti.

>
> >  static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
> >                               libxl_domain_build_info *b_info)
> >  {
> > @@ -1191,16 +1207,32 @@ static void initiate_domain_create(libxl__egc *egc,
> >      ret = libxl__domain_config_setdefault(gc,d_config,domid);
> >      if (ret) goto error_out;
> >
> > -    ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid,
> > -                             dcs->soft_reset);
> > -    if (ret) {
> > -        LOGD(ERROR, domid, "cannot make domain: %d", ret);
> > +    if ( !d_config->dm_restore_file )
> > +    {
> > +        ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid,
> > +                                 dcs->soft_reset);
> >          dcs->guest_domid = domid;
> > +
> > +        if (ret) {
> > +            LOGD(ERROR, domid, "cannot make domain: %d", ret);
> > +            ret = ERROR_FAIL;
> > +            goto error_out;
> > +        }
> > +    } else if ( dcs->guest_domid != INVALID_DOMID ) {
>
> Coding style issue here.
>
> There are quite a few of them.  I won't point them out one by one
> though. Let's focus on the interface first.

Do we have an automatic formatter we could just run on this code-base?
I don't know what style issue you are referring to and given that the
coding style here is different here compared to the hypervisor I find
it very hard to figure it out what other issues you spotted. Please
report them because I won't be able to spot them manually. To me it
all looks fine as-is.

> >
> > +/*
> > + * The parent domain is expected to be created with default settings for
> > + * - max_evtch_port
> > + * - max_grant_frames
> > + * - max_maptrack_frames
> > + */
> > +int libxl_domain_fork_vm(libxl_ctx *ctx, uint32_t pdomid, uint32_t 
> > max_vcpus, uint32_t *domid)
> > +{
> > +    int rc;
> > +    struct xen_domctl_createdomain create = {0};
> > +    create.flags |= XEN_DOMCTL_CDF_hvm;
> > +    create.flags |= XEN_DOMCTL_CDF_hap;
> > +    create.flags |= XEN_DOMCTL_CDF_oos_off;
> > +    create.arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
> > +    create.ssidref = SECINITSID_DOMU;
> > +    create.max_vcpus = max_vcpus;
>
>
> Some questions:
>
> Why does the caller need to specify the number of vcpus?
>
> Since the parent (source) domain is around, can you retrieve its domain
> configuration such that you know its max_vcpus and other information
> including max_evtchn_port and maptrack frames?

Because we want to avoid having to issue an extra hypercall for these.
Normally these pieces of information will be available for the user
and won't change, so we save time by not querying for it every time a
fork is created. Remember, we might be creating thousands of forks in
a very short time, so those extra hypercalls add up.

Tamas