| Message ID | 08d22ed5ffef1d947b819606aafa6414a16bed0b.1582310142.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
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@intel.com> > --- > 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,
On Fri, Feb 21, 2020 at 2:03 PM Julien Grall <julien@xen.org> wrote: > > 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? All parameters of the parent domain need to be copied, but during createdomain domctl only max_vcpus is required. The rest are copied during XENMEM_sharing_op_fork. > > > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > --- > > 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. That's an interesting thought, I don't expect that will be a usecase but it's interesting. Currently I don't think it would work, so I consider that to be out-of-scope. > > 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? I can add a XEN_DOMCTL_CDF_fork flag, but I think that's an overkill. > > > + { > > + 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. No, during createdomain the domains won't get linked. Only once the XENMEM_sharing_op_fork finishes do the domains get linked. I explain this in the patch message, I can copy that as a comment into the header if you prefer. Tamas
Hi Tamas, On 21/02/2020 21:35, Tamas K Lengyel wrote: > On Fri, Feb 21, 2020 at 2:03 PM Julien Grall <julien@xen.org> wrote: >> >> 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? > > All parameters of the parent domain need to be copied, but during > createdomain domctl only max_vcpus is required. The rest are copied > during XENMEM_sharing_op_fork. I don't believe max_vcpus is the only field required here. Most of the information hold in the structure are required at creation time so the domain is configured correctly. For instance, on Arm, the version of interrupt controller can only be configured at creation time. For x86, I am pretty sure the emuflags have to be correct at creation time as well. So it feels weird to me that you only need to copy max_vcpus here. Because if you are able to fill up the other fields of the structure, then most likely you have the max_vcpus in hand as well. The current suggestion is too restrictive and only save you one hypercall. IMHO, if we are going to update createdomain, then all the fields but parent_domid should be ignored when the latter is valid. The fields can then be filled up and copied back to the toolstack so it can consumed the information. > >> >>> >>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> >>> --- >>> 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. > > That's an interesting thought, I don't expect that will be a usecase > but it's interesting. Maybe not in the context VM fork. But ... > Currently I don't think it would work, so I > consider that to be out-of-scope. ... this is a generic hypercall and therefore we should be open to other usecase. I am not asking to check whether we can recreate a domain based on dom0. I am only asking to be mindful with your interface and not put ourself in a corner just because this is out-of-scope for you. The more important bit for me my point 1). I.e not assuming that 0 is an invalid value for domid. > >> >> 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? > > I can add a XEN_DOMCTL_CDF_fork flag, but I think that's an overkill. See above. If you are going to modify a common interface then you need to bear in mind how this can be used. > >> >>> + { >>> + 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. > > No, during createdomain the domains won't get linked. Only once the > XENMEM_sharing_op_fork finishes do the domains get linked. I explain > this in the patch message, I can copy that as a comment into the > header if you prefer. Bear in mind that developpers don't want to play the blame game everytime they want to understand an interfaces. So you either want to use a meaningful name or a comment in your code. Maybe "clone_domid" would be clearer here. Yet it is not clear that only "max_vcpus" is going to be copied. Cheers,
On Fri, Feb 21, 2020 at 3:34 PM Julien Grall <julien@xen.org> wrote: > > Hi Tamas, > > On 21/02/2020 21:35, Tamas K Lengyel wrote: > > On Fri, Feb 21, 2020 at 2:03 PM Julien Grall <julien@xen.org> wrote: > >> > >> 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? > > > > All parameters of the parent domain need to be copied, but during > > createdomain domctl only max_vcpus is required. The rest are copied > > during XENMEM_sharing_op_fork. > > I don't believe max_vcpus is the only field required here. Most of the > information hold in the structure are required at creation time so the > domain is configured correctly. For instance, on Arm, the version of > interrupt controller can only be configured at creation time. For x86, I > am pretty sure the emuflags have to be correct at creation time as well. > > So it feels weird to me that you only need to copy max_vcpus here. > Because if you are able to fill up the other fields of the structure, > then most likely you have the max_vcpus in hand as well. Look at patch 5 and see how libxl statically define most of these values and why we don't need to copy them. > > The current suggestion is too restrictive and only save you one > hypercall. IMHO, if we are going to update createdomain, then all the > fields but parent_domid should be ignored when the latter is valid. The > fields can then be filled up and copied back to the toolstack so it can > consumed the information. > > > > >> > >>> > >>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > >>> --- > >>> 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. > > > > That's an interesting thought, I don't expect that will be a usecase > > but it's interesting. > > Maybe not in the context VM fork. But ... > > > Currently I don't think it would work, so I > > consider that to be out-of-scope. > > ... this is a generic hypercall and therefore we should be open to other > usecase. I am not asking to check whether we can recreate a domain based > on dom0. I am only asking to be mindful with your interface and not put > ourself in a corner just because this is out-of-scope for you. > > The more important bit for me my point 1). I.e not assuming that 0 is an > invalid value for domid. > > > > >> > >> 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? > > > > I can add a XEN_DOMCTL_CDF_fork flag, but I think that's an overkill. > > See above. If you are going to modify a common interface then you need > to bear in mind how this can be used. > > > > >> > >>> + { > >>> + 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. > > > > No, during createdomain the domains won't get linked. Only once the > > XENMEM_sharing_op_fork finishes do the domains get linked. I explain > > this in the patch message, I can copy that as a comment into the > > header if you prefer. > > Bear in mind that developpers don't want to play the blame game > everytime they want to understand an interfaces. So you either want to > use a meaningful name or a comment in your code. > > Maybe "clone_domid" would be clearer here. Yet it is not clear that only > "max_vcpus" is going to be copied. Julien, you have valid points but at this time I won't be able to refactor this series any more. This patch series was first posted in September, it's now almost March. So at this point I'm just going to say drop this patch and we'll live with the limitation that VM forking only works with single vCPU domains until at some later time someone needs it to work with guests that have more then 1 vCPUs. If that's also unacceptable for whatever reason, then we'll live with the feature not being merged upstream. Cheers, Tamas
On Fri, 21 Feb 2020 at 22:53, Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote: > > On Fri, Feb 21, 2020 at 3:34 PM Julien Grall <julien@xen.org> wrote: > > > > Hi Tamas, > > > > On 21/02/2020 21:35, Tamas K Lengyel wrote: > > > On Fri, Feb 21, 2020 at 2:03 PM Julien Grall <julien@xen.org> wrote: > > >> > > >> 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? > > > > > > All parameters of the parent domain need to be copied, but during > > > createdomain domctl only max_vcpus is required. The rest are copied > > > during XENMEM_sharing_op_fork. > > > > I don't believe max_vcpus is the only field required here. Most of the > > information hold in the structure are required at creation time so the > > domain is configured correctly. For instance, on Arm, the version of > > interrupt controller can only be configured at creation time. For x86, I > > am pretty sure the emuflags have to be correct at creation time as well. > > > > So it feels weird to me that you only need to copy max_vcpus here. > > Because if you are able to fill up the other fields of the structure, > > then most likely you have the max_vcpus in hand as well. > > Look at patch 5 and see how libxl statically define most of these > values and why we don't need to copy them. I looked at patch 5, this is an example of the implementation. You limit yourself quite a bit and that's your choice. But I am afraid this does not mean the interface with the hypervisor should do the same. [...] > Julien, > you have valid points but at this time I won't be able to refactor > this series any more. This patch series was first posted in September, > it's now almost March. So at this point I'm just going to say drop > this patch and we'll live with the limitation that VM forking only > works with single vCPU domains until at some later time someone needs > it to work with guests that have more then 1 vCPUs. To be honest, I don't have a vested interest for the VM forking. So the limitation of 1 vCPU is fine with me. Anyone who will want to support more than 1 vCPU with forking will have to come up with a proper interface. If you don't want to invest time on it that's fine, the rest of the code is still useful to have. Cheers,
On Fri, Feb 21, 2020 at 4:18 PM Julien Grall <julien.grall.oss@gmail.com> wrote: > > On Fri, 21 Feb 2020 at 22:53, Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote: > > > > On Fri, Feb 21, 2020 at 3:34 PM Julien Grall <julien@xen.org> wrote: > > > > > > Hi Tamas, > > > > > > On 21/02/2020 21:35, Tamas K Lengyel wrote: > > > > On Fri, Feb 21, 2020 at 2:03 PM Julien Grall <julien@xen.org> wrote: > > > >> > > > >> 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? > > > > > > > > All parameters of the parent domain need to be copied, but during > > > > createdomain domctl only max_vcpus is required. The rest are copied > > > > during XENMEM_sharing_op_fork. > > > > > > I don't believe max_vcpus is the only field required here. Most of the > > > information hold in the structure are required at creation time so the > > > domain is configured correctly. For instance, on Arm, the version of > > > interrupt controller can only be configured at creation time. For x86, I > > > am pretty sure the emuflags have to be correct at creation time as well. > > > > > > So it feels weird to me that you only need to copy max_vcpus here. > > > Because if you are able to fill up the other fields of the structure, > > > then most likely you have the max_vcpus in hand as well. > > > > Look at patch 5 and see how libxl statically define most of these > > values and why we don't need to copy them. > > I looked at patch 5, this is an example of the implementation. > You limit yourself quite a bit and that's your choice. But I am afraid > this does not mean the interface with the hypervisor should do the > same. > > [...] > > > Julien, > > you have valid points but at this time I won't be able to refactor > > this series any more. This patch series was first posted in September, > > it's now almost March. So at this point I'm just going to say drop > > this patch and we'll live with the limitation that VM forking only > > works with single vCPU domains until at some later time someone needs > > it to work with guests that have more then 1 vCPUs. > > To be honest, I don't have a vested interest for the VM forking. So > the limitation > of 1 vCPU is fine with me. > > Anyone who will want to support more than 1 vCPU with forking will have to > come up with a proper interface. If you don't want to invest time on it that's > fine, the rest of the code is still useful to have. The toolstack can always just decide to do the extra hypercall query for the max_vcpus and make things work that way. In our usecase we have single vCPU domains so doing that extra query is something I want to avoid. 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. Right, but when I said this wasn't safe, I did mean it... What happens when parent and the current domain have different gnttab or evtchn limits, or different emulation settings? If you want to fork a domain safely, you either need to have no parameters passed by the toolstack (and let Xen copy appropriate values), or cross check every provided parameter and bail early on a mismatch. ~Andrew
On Mon, Feb 24, 2020 at 8:45 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > 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. > > Right, but when I said this wasn't safe, I did mean it... > > What happens when parent and the current domain have different gnttab or > evtchn limits, or different emulation settings? > > If you want to fork a domain safely, you either need to have no > parameters passed by the toolstack (and let Xen copy appropriate > values), or cross check every provided parameter and bail early on a > mismatch. If you are using the toolstack code we add in patch 5 that doesn't happen. So, for the situation you describe to happen: 1) you have to custom compile Xen with the EXPERT setting enable this experimental feature 2) write your own toolstack code 3) screw up doing so. This is such an unlikely scenario that I'm not really bothered by it. Tamas
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 ) + { + 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; struct xen_arch_domainconfig arch; };
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. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/common/domctl.c | 14 ++++++++++++++ xen/include/public/domctl.h | 3 ++- 2 files changed, 16 insertions(+), 1 deletion(-)