Re: [Xen-devel] [PATCH v9 2/5] xen: add parent_domid field to createdomain domctl

[Julien Grall <julien@xxxxxxx>]

Hi Tamas,

On 21/02/2020 18:49, Tamas K Lengyel wrote:
> When creating a domain that will be used as a VM fork some information is
> required to set things up properly, like the max_vcpus count. Instead of the
> toolstack having to gather this information for each fork in a separate
> hypercall we can just include the parent domain's id in the createdomain domctl
> so that Xen can copy the setting without the extra toolstack queries.
It is not entirely clear why you only want to copy max_vcpus. From my 
understanding,  when you are going to fork a domain you will want the 
domain to be nearly identical. So how do you decide what to copy?
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> ---
>   xen/common/domctl.c         | 14 ++++++++++++++
>   xen/include/public/domctl.h |  3 ++-
>   2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index a69b3b59a8..22aceb3860 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -489,6 +489,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>       case XEN_DOMCTL_createdomain:
>       {
>           domid_t        dom;
> +        domid_t        parent_dom;
>           static domid_t rover = 0;
>   
>           dom = op->domain;
> @@ -515,6 +516,19 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>               rover = dom;
>           }
>   
> +        parent_dom = op->u.createdomain.parent_domid;
> +        if ( parent_dom )
I would rather avoid to assume that parent_dom will not be 0 for a few 
reasons:
   1) Most of Xen (if not all) now avoid to assume that dom0->domain_id 
== 0.
   2) I can see usecases where it we may want to recreate dom0 setup.

So we should consider a different value to indicate whether we want to 
clone from a domain. Maybe by setting bit 16 of the parent_domid?
> +        {
> +            struct domain *pd = rcu_lock_domain_by_id(parent_dom);
> +
> +            ret = -EINVAL;
> +            if ( !pd )
> +                break;
> +
> +            op->u.createdomain.max_vcpus = pd->max_vcpus;
> +            rcu_unlock_domain(pd);
> +        }
> +
>           d = domain_create(dom, &op->u.createdomain, false);
>           if ( IS_ERR(d) )
>           {
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index fec6f6fdd1..251cc40ef6 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>   #include "hvm/save.h"
>   #include "memory.h"
>   
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
>   
>   /*
>    * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -92,6 +92,7 @@ struct xen_domctl_createdomain {
>       uint32_t max_evtchn_port;
>       int32_t max_grant_frames;
>       int32_t max_maptrack_frames;
> +    domid_t parent_domid;
By just looking at the name, it is not clear what the field is for. It 
also suggest that one domain will be linked to the other. But this is 
not the case here. I would recommend to add a comment explaining how 
this is used by Xen.
Cheers,

--
Julien Grall

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