diff mbox

[v3,01/15] xen: add xenstore domain flag to hypervisor

Message ID 1452258526-4797-2-git-send-email-jgross@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Juergen Gross Jan. 8, 2016, 1:08 p.m. UTC
In order to be able to have full support of a xenstore domain in Xen
add a "Xenstore-domain" flag to the hypervisor. This flag must be
specified at domain creation time and is returned by
XEN_DOMCTL_getdomaininfo.

It will allow the domain to retrieve domain information by issuing the
XEN_DOMCTL_getdomaininfo itself in order to be able to check for
domains having been destroyed. At the same time this flag will inhibit
the domain to be migrated, as this wouldn't be a very wise thing to do.

In case of a later support of a rebootable Dom0 this flag will allow to
recognize a xenstore domain already being present to connect to.

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com>
---
V3: changed comment as requested by Ian Campbell
---
 xen/common/domain.c         |  6 ++++++
 xen/common/domctl.c         | 14 +++++++++-----
 xen/include/public/domctl.h |  6 ++++++
 xen/include/xen/sched.h     |  5 +++++
 xen/include/xsm/dummy.h     |  6 ++++++
 xen/include/xsm/xsm.h       |  1 +
 6 files changed, 33 insertions(+), 5 deletions(-)

Comments

Jan Beulich Jan. 8, 2016, 2:07 p.m. UTC | #1
>>> On 08.01.16 at 14:08, <JGross@suse.com> wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>          hardware_domain = d;
>      }
>  
> +    if ( domcr_flags & DOMCRF_xs_domain )
> +    {
> +        d->is_xenstore = 1;
> +        d->disable_migrate = 1;
> +    }

It only occurred to me now: Wouldn't it be worth denying the domain
creation request if there already is a xenstore domain?

Jan
Juergen Gross Jan. 8, 2016, 2:22 p.m. UTC | #2
On 08/01/16 15:07, Jan Beulich wrote:
>>>> On 08.01.16 at 14:08, <JGross@suse.com> wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>>          hardware_domain = d;
>>      }
>>  
>> +    if ( domcr_flags & DOMCRF_xs_domain )
>> +    {
>> +        d->is_xenstore = 1;
>> +        d->disable_migrate = 1;
>> +    }
> 
> It only occurred to me now: Wouldn't it be worth denying the domain
> creation request if there already is a xenstore domain?

Hmm, the only cases where this would make sense would be:

- some bug in dom0 resulting in parallel calls of init-xenstore-domain
- an out-of-tree tool in dom0 (or another domain capable of creating
  domains) creating a domain with the xenstore flag set

I'm really not sure what would be best here. Should the hypervisor
really be responsible for this decision?

In case it is desired, I can modify the patch accordingly (the
modification would be trivial).


Juergen
Andrew Cooper Jan. 8, 2016, 2:25 p.m. UTC | #3
On 08/01/16 14:22, Juergen Gross wrote:
> On 08/01/16 15:07, Jan Beulich wrote:
>>>>> On 08.01.16 at 14:08, <JGross@suse.com> wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>>>          hardware_domain = d;
>>>      }
>>>  
>>> +    if ( domcr_flags & DOMCRF_xs_domain )
>>> +    {
>>> +        d->is_xenstore = 1;
>>> +        d->disable_migrate = 1;
>>> +    }
>> It only occurred to me now: Wouldn't it be worth denying the domain
>> creation request if there already is a xenstore domain?
> Hmm, the only cases where this would make sense would be:
>
> - some bug in dom0 resulting in parallel calls of init-xenstore-domain
> - an out-of-tree tool in dom0 (or another domain capable of creating
>   domains) creating a domain with the xenstore flag set
>
> I'm really not sure what would be best here. Should the hypervisor
> really be responsible for this decision?
>
> In case it is desired, I can modify the patch accordingly (the
> modification would be trivial).

For now, it might be better to knobble it in Xen to weed out bugs.

However, given future plans for restartable or multiple xenstores, I
could easily see it becoming a valid option.

~Andrew
Jan Beulich Jan. 8, 2016, 4:03 p.m. UTC | #4
>>> On 08.01.16 at 15:22, <JGross@suse.com> wrote:
> On 08/01/16 15:07, Jan Beulich wrote:
>>>>> On 08.01.16 at 14:08, <JGross@suse.com> wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned 
> int domcr_flags,
>>>          hardware_domain = d;
>>>      }
>>>  
>>> +    if ( domcr_flags & DOMCRF_xs_domain )
>>> +    {
>>> +        d->is_xenstore = 1;
>>> +        d->disable_migrate = 1;
>>> +    }
>> 
>> It only occurred to me now: Wouldn't it be worth denying the domain
>> creation request if there already is a xenstore domain?
> 
> Hmm, the only cases where this would make sense would be:
> 
> - some bug in dom0 resulting in parallel calls of init-xenstore-domain

Or human (admin) error?

Jan

> - an out-of-tree tool in dom0 (or another domain capable of creating
>   domains) creating a domain with the xenstore flag set
> 
> I'm really not sure what would be best here. Should the hypervisor
> really be responsible for this decision?
> 
> In case it is desired, I can modify the patch accordingly (the
> modification would be trivial).
> 
> 
> Juergen
Juergen Gross Jan. 8, 2016, 4:10 p.m. UTC | #5
On 08/01/16 17:03, Jan Beulich wrote:
>>>> On 08.01.16 at 15:22, <JGross@suse.com> wrote:
>> On 08/01/16 15:07, Jan Beulich wrote:
>>>>>> On 08.01.16 at 14:08, <JGross@suse.com> wrote:
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned 
>> int domcr_flags,
>>>>          hardware_domain = d;
>>>>      }
>>>>  
>>>> +    if ( domcr_flags & DOMCRF_xs_domain )
>>>> +    {
>>>> +        d->is_xenstore = 1;
>>>> +        d->disable_migrate = 1;
>>>> +    }
>>>
>>> It only occurred to me now: Wouldn't it be worth denying the domain
>>> creation request if there already is a xenstore domain?
>>
>> Hmm, the only cases where this would make sense would be:
>>
>> - some bug in dom0 resulting in parallel calls of init-xenstore-domain
> 
> Or human (admin) error?

I don't think so. init-xenstore-domain checks whether xenstored or
another xenstore domain is already running and will fail in this case.
Multiple xenstore domains are possible only if init-xenstore-domain
is running multiple times in parallel and at least two instances
succeed in passing the check for another xenstored domain before
one instance did create the domain.

The only way to achieve this via admin error would be to duplicate
the start script/service and configure both to run in parallel.
I think this scenario is plausible only for an action done on
purpose.


Juergen
David Vrabel Jan. 8, 2016, 4:13 p.m. UTC | #6
On 08/01/16 14:07, Jan Beulich wrote:
>>>> On 08.01.16 at 14:08, <JGross@suse.com> wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -318,6 +318,12 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>>          hardware_domain = d;
>>      }
>>  
>> +    if ( domcr_flags & DOMCRF_xs_domain )
>> +    {
>> +        d->is_xenstore = 1;
>> +        d->disable_migrate = 1;
>> +    }
> 
> It only occurred to me now: Wouldn't it be worth denying the domain
> creation request if there already is a xenstore domain?

Running multiple xenstore domains, one for each isolated set of domains
managed by their own toolstack domain with their own backends seems like
a valid (future) use case.

David
Ian Campbell Jan. 15, 2016, 3:50 p.m. UTC | #7
On Fri, 2016-01-08 at 14:08 +0100, Juergen Gross wrote:
> In order to be able to have full support of a xenstore domain in Xen
> add a "Xenstore-domain" flag to the hypervisor. This flag must be
> specified at domain creation time and is returned by
> XEN_DOMCTL_getdomaininfo.
> 
> It will allow the domain to retrieve domain information by issuing the
> XEN_DOMCTL_getdomaininfo itself in order to be able to check for
> domains having been destroyed. At the same time this flag will inhibit
> the domain to be migrated, as this wouldn't be a very wise thing to do.
> 
> In case of a later support of a rebootable Dom0 this flag will allow to
> recognize a xenstore domain already being present to connect to.
> 
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>
diff mbox

Patch

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 1017efb..2979c1b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -318,6 +318,12 @@  struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         hardware_domain = d;
     }
 
+    if ( domcr_flags & DOMCRF_xs_domain )
+    {
+        d->is_xenstore = 1;
+        d->disable_migrate = 1;
+    }
+
     rangeset_domain_initialise(d);
     init_status |= INIT_rangeset;
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 1fab587..dc1ea06 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -183,10 +183,11 @@  void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->cpu_time = cpu_time;
 
     info->flags = (info->nr_online_vcpus ? flags : 0) |
-        ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying    : 0) |
-        (d->is_shut_down                ? XEN_DOMINF_shutdown : 0) |
-        (d->controller_pause_count > 0  ? XEN_DOMINF_paused   : 0) |
-        (d->debugger_attached           ? XEN_DOMINF_debugged : 0) |
+        ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying     : 0) |
+        (d->is_shut_down                ? XEN_DOMINF_shutdown  : 0) |
+        (d->controller_pause_count > 0  ? XEN_DOMINF_paused    : 0) |
+        (d->debugger_attached           ? XEN_DOMINF_debugged  : 0) |
+        (d->is_xenstore                 ? XEN_DOMINF_xs_domain : 0) |
         d->shutdown_code << XEN_DOMINF_shutdownshift;
 
     switch ( d->guest_type )
@@ -551,7 +552,8 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                | XEN_DOMCTL_CDF_pvh_guest
                | XEN_DOMCTL_CDF_hap
                | XEN_DOMCTL_CDF_s3_integrity
-               | XEN_DOMCTL_CDF_oos_off)) )
+               | XEN_DOMCTL_CDF_oos_off
+               | XEN_DOMCTL_CDF_xs_domain)) )
             break;
 
         dom = op->domain;
@@ -593,6 +595,8 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             domcr_flags |= DOMCRF_s3_integrity;
         if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_oos_off )
             domcr_flags |= DOMCRF_oos_off;
+        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
+            domcr_flags |= DOMCRF_xs_domain;
 
         d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
                           &op->u.createdomain.config);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 7a56b3f..2d8076c 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -63,6 +63,9 @@  struct xen_domctl_createdomain {
  /* Is this a PVH guest (as opposed to an HVM or PV guest)? */
 #define _XEN_DOMCTL_CDF_pvh_guest     4
 #define XEN_DOMCTL_CDF_pvh_guest      (1U<<_XEN_DOMCTL_CDF_pvh_guest)
+ /* Is this a xenstore domain? */
+#define _XEN_DOMCTL_CDF_xs_domain     5
+#define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
     uint32_t flags;
     struct xen_arch_domainconfig config;
 };
@@ -97,6 +100,9 @@  struct xen_domctl_getdomaininfo {
 /* domain is PVH */
 #define _XEN_DOMINF_pvh_guest 7
 #define XEN_DOMINF_pvh_guest  (1U<<_XEN_DOMINF_pvh_guest)
+/* domain is a xenstore domain */
+#define _XEN_DOMINF_xs_domain 8
+#define XEN_DOMINF_xs_domain  (1U<<_XEN_DOMINF_xs_domain)
  /* XEN_DOMINF_shutdown guest-supplied code.  */
 #define XEN_DOMINF_shutdownmask 255
 #define XEN_DOMINF_shutdownshift 16
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index e1428f7..82b6dd1 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -374,6 +374,8 @@  struct domain
     bool_t           auto_node_affinity;
     /* Is this guest fully privileged (aka dom0)? */
     bool_t           is_privileged;
+    /* Is this a xenstore domain (not dom0)? */
+    bool_t           is_xenstore;
     /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
     bool_t           is_pinned;
     /* Non-migratable and non-restoreable? */
@@ -533,6 +535,9 @@  struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
  /* DOMCRF_pvh: Create PV domain in HVM container. */
 #define _DOMCRF_pvh             5
 #define DOMCRF_pvh              (1U<<_DOMCRF_pvh)
+ /* DOMCRF_xs_domain: xenstore domain */
+#define _DOMCRF_xs_domain       6
+#define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
 
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 55b84f0..1d13826 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -71,6 +71,10 @@  static always_inline int xsm_default_action(
         if ( src->is_privileged )
             return 0;
         return -EPERM;
+    case XSM_XS_PRIV:
+        if ( src->is_xenstore || src->is_privileged )
+            return 0;
+        return -EPERM;
     default:
         LINKER_BUG_ON(1);
         return -EPERM;
@@ -123,6 +127,8 @@  static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd)
     case XEN_DOMCTL_bind_pt_irq:
     case XEN_DOMCTL_unbind_pt_irq:
         return xsm_default_action(XSM_DM_PRIV, current->domain, d);
+    case XEN_DOMCTL_getdomaininfo:
+        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
     default:
         return xsm_default_action(XSM_PRIV, current->domain, d);
     }
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 2c365cd..3afed70 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -35,6 +35,7 @@  enum xsm_default {
     XSM_DM_PRIV,  /* Device model can perform on its target domain */
     XSM_TARGET,   /* Can perform on self or your target domain */
     XSM_PRIV,     /* Privileged - normally restricted to dom0 */
+    XSM_XS_PRIV,  /* Xenstore domain - can do some privileged operations */
     XSM_OTHER     /* Something more complex */
 };
 typedef enum xsm_default xsm_default_t;