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

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

On Thu, Apr 9, 2020 at 11:30 AM Tamas K Lengyel
<tamas.k.lengyel@xxxxxxxxx> wrote:
>
> On Thu, Apr 9, 2020 at 11:11 AM Wei Liu <wl@xxxxxxx> wrote:
> >
> > On Thu, Apr 09, 2020 at 10:59:55AM -0600, Tamas K Lengyel wrote:
> > [...]
> > > >
> > > > >
> > > > > > >
> > > > > > > +/*
> > > > > > > + * 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.
> > > >
> > > > I see. Speed is a big concern to you.
> > > >
> > > > What I was referring to doesn't require issuing hypercalls but requires
> > > > calling libxl_retrieve_domain_configuration. That's likely to be even
> > > > slower than making a hypercall.
> > >
> > > Right. We only want to parse the domain config if the device model is
> > > being launched.
> > >
> > > >
> > > > I'm afraid the current incarnation of libxl_domain_fork_vm cannot become
> > > > supported (as in Xen's support statement) going forward, because it is
> > > > leaky.
> > >
> > > What do you mean by leaky?
> >
> > It requires the caller to specify the number of max_vcpus. The reason
> > for doing that is because you want to avoid extra hypercall(s). There is
> > nothing that stops someone from coming along in the future claiming some
> > other parameters are required because of the same concern you have
> > today. It is an optimisation, not a must-have in terms of functionality.
> >
> > To me the number shouldn't be specified by the caller in the first
> > place. It is like forking a process somehow requires you to specify how
> > many threads it will have.
>
> I agree. It's not how I wanted to have the interface work but
> unfortunately this was the least "ugly" of the possible solutions
> given the circumstances.

By the way, it would be trivial to query the parent in case max_vcpus
is not provided by the user. But I would still like to keep the option
available to skip that step if the number is known already. Let me
know if you would like me to add that.

Thanks,
Tamas