[PATCH 1/2] implement is_hardware_domain using hardware_domain global

[PATCH 1/2] implement is_hardware_domain using hardware_domain global

[Daniel De Graaf]

This requires setting the hardware_domain variable earlier in
domain_create so that functions called from it (primarily in
arch_domain_create) observe the correct results when they call
is_hardware_domain.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/setup.c    | 2 +-
 xen/arch/x86/setup.c    | 2 +-
 xen/common/domain.c     | 5 ++++-
 xen/include/xen/sched.h | 4 ++--
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 358eafb..6b77a4c 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -761,7 +761,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     do_initcalls();
 
     /* Create initial domain 0. */
-    hardware_domain = dom0 = domain_create(0, 0, 0);
+    dom0 = domain_create(0, 0, 0);
     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
             panic("Error creating domain 0");
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 11c95fc..2e30701 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1339,7 +1339,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         panic("Could not protect TXT memory regions");
 
     /* Create initial domain 0. */
-    hardware_domain = dom0 = domain_create(0, DOMCRF_s3_integrity, 0);
+    dom0 = domain_create(0, DOMCRF_s3_integrity, 0);
     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
         panic("Error creating domain 0");
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index c4720a9..3c05711 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -237,10 +237,11 @@ struct domain *domain_create(
     else if ( domcr_flags & DOMCRF_pvh )
         d->guest_type = guest_type_pvh;
 
-    if ( is_hardware_domain(d) )
+    if ( domid == 0 )
     {
         d->is_pinned = opt_dom0_vcpus_pin;
         d->disable_migrate = 1;
+        hardware_domain = d;
     }
 
     rangeset_domain_initialise(d);
@@ -319,6 +320,8 @@ struct domain *domain_create(
 
  fail:
     d->is_dying = DOMDYING_dead;
+    if ( hardware_domain == d )
+        hardware_domain = NULL;
     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
     xfree(d->mem_event);
     xfree(d->pbuf);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b080c9e..734f7a9 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -795,9 +795,9 @@ void watchdog_domain_destroy(struct domain *d);
  * Use this check when the following are both true:
  *  - Using this feature or interface requires full access to the hardware
  *    (that is, this would not be suitable for a driver domain)
- *  - There is never a reason to deny dom0 access to this
+ *  - There is never a reason to deny the hardware domain access to this
  */
-#define is_hardware_domain(_d) ((_d)->domain_id == 0)
+#define is_hardware_domain(_d) ((_d) == hardware_domain)
 
 /* This check is for functionality specific to a control domain */
 #define is_control_domain(_d) ((_d)->is_privileged)
-- 
1.9.0

[PATCH v4 2/2] allow hardware domain != dom0

[Daniel De Graaf]

This adds a hypervisor command line option "hardware_dom=" which takes a
domain ID.  When the domain with this ID is created, it will be used
as the hardware domain.

This is intended to be used when domain 0 is a dedicated stub domain for
domain building, allowing the hardware domain to be de-privileged and
act only as a driver domain.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 docs/misc/xen-command-line.markdown | 10 ++++++
 xen/arch/x86/domain_build.c         |  4 ++-
 xen/common/domain.c                 | 62 +++++++++++++++++++++++++++++++++++--
 xen/common/rangeset.c               | 40 ++++++++++++++++++++++++
 xen/include/xen/rangeset.h          |  3 ++
 xen/include/xen/sched.h             |  6 ++++
 xen/include/xsm/dummy.h             |  6 ++++
 xen/include/xsm/xsm.h               |  6 ++++
 xen/xsm/dummy.c                     |  2 ++
 xen/xsm/flask/hooks.c               |  6 ++++
 xen/xsm/flask/policy/access_vectors |  2 ++
 11 files changed, 143 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 87de2dc..6f5064f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -590,6 +590,16 @@ Paging (HAP).
 Flag to enable 2 MB host page table support for Hardware Assisted
 Paging (HAP).
 
+### hardware\_dom
+> `= <domid>`
+
+> Default: `0`
+
+Enable late hardware domain creation using the specified domain ID.  This is
+intended to be used when domain 0 is a stub domain which builds a disaggregated
+system including a hardware domain with the specified domain ID.  This option is
+supported only when compiled with CONFIG\_XSM on x86.
+
 ### hpetbroadcast
 > `= <boolean>`
 
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index f75f6e7..10fcd43 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -1150,7 +1150,9 @@ int __init construct_dom0(
         printk(" Xen warning: dom0 kernel broken ELF: %s\n",
                elf_check_broken(&elf));
 
-    iommu_hwdom_init(hardware_domain);
+    if ( d->domain_id == hardware_domid )
+        iommu_hwdom_init(d);
+
     return 0;
 
 out:
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3c05711..11c905a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -61,6 +61,11 @@ struct domain *domain_list;
 
 struct domain *hardware_domain __read_mostly;
 
+#ifdef CONFIG_LATE_HWDOM
+domid_t hardware_domid __read_mostly;
+integer_param("hardware_dom", hardware_domid);
+#endif
+
 struct vcpu *idle_vcpu[NR_CPUS] __read_mostly;
 
 vcpu_info_t dummy_vcpu_info;
@@ -178,6 +183,53 @@ struct vcpu *alloc_vcpu(
     return v;
 }
 
+static int late_hwdom_init(struct domain *d)
+{
+#ifdef CONFIG_LATE_HWDOM
+    struct domain *dom0;
+    int rv;
+
+    if ( d != hardware_domain || d->domain_id == 0 )
+        return 0;
+
+    rv = xsm_init_hardware_domain(XSM_HOOK, d);
+    if ( rv )
+        return rv;
+
+    printk("Initialising hardware domain %d\n", hardware_domid);
+
+    dom0 = rcu_lock_domain_by_id(0);
+    ASSERT(dom0 != NULL);
+    /*
+     * Hardware resource ranges for domain 0 have been set up from
+     * various sources intended to restrict the hardware domain's
+     * access.  Apply these ranges to the actual hardware domain.
+     *
+     * Because the lists are being swapped, a side effect of this
+     * operation is that Domain 0's rangesets are cleared.  Since
+     * domain 0 should not be accessing the hardware when it constructs
+     * a hardware domain, this should not be a problem.  Both lists
+     * may be modified after this hypercall returns if a more complex
+     * device model is desired.
+     *
+     * Since late hardware domain initialization is only supported on
+     * x86, the reference to arch.ioport_caps does not need its own
+     * preprocessor conditional.
+     */
+    rangeset_swap(d->irq_caps, dom0->irq_caps);
+    rangeset_swap(d->iomem_caps, dom0->iomem_caps);
+    rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
+
+    rcu_unlock_domain(dom0);
+
+    iommu_hwdom_init(d);
+
+    return rv;
+#else
+    return 0;
+#endif
+}
+
 static unsigned int __read_mostly extra_dom0_irqs = 256;
 static unsigned int __read_mostly extra_domU_irqs = 32;
 static void __init parse_extra_guest_irqs(const char *s)
@@ -192,7 +244,7 @@ custom_param("extra_guest_irqs", parse_extra_guest_irqs);
 struct domain *domain_create(
     domid_t domid, unsigned int domcr_flags, uint32_t ssidref)
 {
-    struct domain *d, **pd;
+    struct domain *d, **pd, *old_hwdom = NULL;
     enum { INIT_xsm = 1u<<0, INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
@@ -237,10 +289,11 @@ struct domain *domain_create(
     else if ( domcr_flags & DOMCRF_pvh )
         d->guest_type = guest_type_pvh;
 
-    if ( domid == 0 )
+    if ( domid == 0 || domid == hardware_domid )
     {
         d->is_pinned = opt_dom0_vcpus_pin;
         d->disable_migrate = 1;
+        old_hwdom = hardware_domain;
         hardware_domain = d;
     }
 
@@ -316,12 +369,15 @@ struct domain *domain_create(
         spin_unlock(&domlist_update_lock);
     }
 
+    if ( (err = late_hwdom_init(d)) != 0 )
+        goto fail;
+
     return d;
 
  fail:
     d->is_dying = DOMDYING_dead;
     if ( hardware_domain == d )
-        hardware_domain = NULL;
+        hardware_domain = old_hwdom;
     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
     xfree(d->mem_event);
     xfree(d->pbuf);
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index f09c0c4..52fae1f 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -438,3 +438,43 @@ void rangeset_domain_printk(
 
     spin_unlock(&d->rangesets_lock);
 }
+
+void rangeset_swap(struct rangeset *a, struct rangeset *b)
+{
+    struct list_head tmp;
+    if (&a < &b)
+    {
+        spin_lock(&a->lock);
+        spin_lock(&b->lock);
+    }
+    else
+    {
+        spin_lock(&b->lock);
+        spin_lock(&a->lock);
+    }
+    memcpy(&tmp, &a->range_list, sizeof(tmp));
+    memcpy(&a->range_list, &b->range_list, sizeof(tmp));
+    memcpy(&b->range_list, &tmp, sizeof(tmp));
+    if ( a->range_list.next == &b->range_list )
+    {
+        a->range_list.next = &a->range_list;
+        a->range_list.prev = &a->range_list;
+    }
+    else
+    {
+        a->range_list.next->prev = &a->range_list;
+        a->range_list.prev->next = &a->range_list;
+    }
+    if ( b->range_list.next == &a->range_list )
+    {
+        b->range_list.next = &b->range_list;
+        b->range_list.prev = &b->range_list;
+    }
+    else
+    {
+        b->range_list.next->prev = &b->range_list;
+        b->range_list.prev->next = &b->range_list;
+    }
+    spin_unlock(&a->lock);
+    spin_unlock(&b->lock);
+}
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 1e16a6b..805ebde 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -73,4 +73,7 @@ void rangeset_printk(
 void rangeset_domain_printk(
     struct domain *d);
 
+/* swap contents */
+void rangeset_swap(struct rangeset *a, struct rangeset *b);
+
 #endif /* __XEN_RANGESET_H__ */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 734f7a9..44851ae 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -46,6 +46,12 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t);
 /* A global pointer to the hardware domain (usually DOM0). */
 extern struct domain *hardware_domain;
 
+#ifdef CONFIG_LATE_HWDOM
+extern domid_t hardware_domid;
+#else
+#define hardware_domid 0
+#endif
+
 #ifndef CONFIG_COMPAT
 #define BITS_PER_EVTCHN_WORD(d) BITS_PER_XEN_ULONG
 #else
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index e722155..8ca1117 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -299,6 +299,12 @@ static XSM_INLINE char *xsm_show_security_evtchn(struct domain *d, const struct
     return NULL;
 }
 
+static XSM_INLINE int xsm_init_hardware_domain(XSM_DEFAULT_ARG struct domain *d)
+{
+    XSM_ASSERT_ACTION(XSM_HOOK);
+    return xsm_default_action(action, current->domain, d);
+}
+
 static XSM_INLINE int xsm_get_pod_target(XSM_DEFAULT_ARG struct domain *d)
 {
     XSM_ASSERT_ACTION(XSM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 2cd3a3b..ef1c584 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -82,6 +82,7 @@ struct xsm_operations {
     int (*alloc_security_evtchn) (struct evtchn *chn);
     void (*free_security_evtchn) (struct evtchn *chn);
     char *(*show_security_evtchn) (struct domain *d, const struct evtchn *chn);
+    int (*init_hardware_domain) (struct domain *d);
 
     int (*get_pod_target) (struct domain *d);
     int (*set_pod_target) (struct domain *d);
@@ -311,6 +312,11 @@ static inline char *xsm_show_security_evtchn (struct domain *d, const struct evt
     return xsm_ops->show_security_evtchn(d, chn);
 }
 
+static inline int xsm_init_hardware_domain (xsm_default_t def, struct domain *d)
+{
+    return xsm_ops->init_hardware_domain(d);
+}
+
 static inline int xsm_get_pod_target (xsm_default_t def, struct domain *d)
 {
     return xsm_ops->get_pod_target(d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index b79e10f..c2804f2 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -58,6 +58,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, alloc_security_evtchn);
     set_to_dummy_if_null(ops, free_security_evtchn);
     set_to_dummy_if_null(ops, show_security_evtchn);
+    set_to_dummy_if_null(ops, init_hardware_domain);
+
     set_to_dummy_if_null(ops, get_pod_target);
     set_to_dummy_if_null(ops, set_pod_target);
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 4ce31c9..f1a4a2d 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -327,6 +327,11 @@ static char *flask_show_security_evtchn(struct domain *d, const struct evtchn *c
     return ctx;
 }
 
+static int flask_init_hardware_domain(struct domain *d)
+{
+    return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CREATE_HARDWARE_DOMAIN);
+}
+
 static int flask_grant_mapref(struct domain *d1, struct domain *d2, 
                               uint32_t flags)
 {
@@ -1500,6 +1505,7 @@ static struct xsm_operations flask_ops = {
     .alloc_security_evtchn = flask_alloc_security_evtchn,
     .free_security_evtchn = flask_free_security_evtchn,
     .show_security_evtchn = flask_show_security_evtchn,
+    .init_hardware_domain = flask_init_hardware_domain,
 
     .get_pod_target = flask_get_pod_target,
     .set_pod_target = flask_set_pod_target,
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index a0ed13d..32371a9 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -198,6 +198,8 @@ class domain2
     set_max_evtchn
 # XEN_DOMCTL_cacheflush
     cacheflush
+# Creation of the hardware domain when it is not dom0
+    create_hardware_domain
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
1.9.0

Re: [PATCH v4 2/2] allow hardware domain != dom0

[Jan Beulich]

>>> On 14.04.14 at 23:23, <dgdegra@tycho.nsa.gov> wrote:

Much better!

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -590,6 +590,16 @@ Paging (HAP).
>  Flag to enable 2 MB host page table support for Hardware Assisted
>  Paging (HAP).
>  
> +### hardware\_dom
> +> `= <domid>`
> +
> +> Default: `0`
> +
> +Enable late hardware domain creation using the specified domain ID.  This is
> +intended to be used when domain 0 is a stub domain which builds a disaggregated
> +system including a hardware domain with the specified domain ID.  This option is
> +supported only when compiled with CONFIG\_XSM on x86.

I think we ought to name the build option (XSM_ENABLE=y) here
rather than the internally used manifest constant to propagate the
requested setting.

> +static int late_hwdom_init(struct domain *d)
> +{
> +#ifdef CONFIG_LATE_HWDOM
> +    struct domain *dom0;
> +    int rv;
> +
> +    if ( d != hardware_domain || d->domain_id == 0 )
> +        return 0;
> +
> +    rv = xsm_init_hardware_domain(XSM_HOOK, d);
> +    if ( rv )
> +        return rv;
> +
> +    printk("Initialising hardware domain %d\n", hardware_domid);
> +
> +    dom0 = rcu_lock_domain_by_id(0);
> +    ASSERT(dom0 != NULL);
> +    /*
> +     * Hardware resource ranges for domain 0 have been set up from
> +     * various sources intended to restrict the hardware domain's
> +     * access.  Apply these ranges to the actual hardware domain.
> +     *
> +     * Because the lists are being swapped, a side effect of this
> +     * operation is that Domain 0's rangesets are cleared.  Since
> +     * domain 0 should not be accessing the hardware when it constructs
> +     * a hardware domain, this should not be a problem.  Both lists
> +     * may be modified after this hypercall returns if a more complex
> +     * device model is desired.
> +     *
> +     * Since late hardware domain initialization is only supported on
> +     * x86, the reference to arch.ioport_caps does not need its own
> +     * preprocessor conditional.

This is only "for now" - please either say so, or even better drop this
part of the comment and add the conditional regardless of it being
redundant right now.

> +void rangeset_swap(struct rangeset *a, struct rangeset *b)
> +{
> +    struct list_head tmp;
> +    if (&a < &b)

Coding style. Also I pretty certain you don't want to compare the
addresses of the pointers here, but the pointer themselves.

> @@ -73,4 +73,7 @@ void rangeset_printk(
>  void rangeset_domain_printk(
>      struct domain *d);
>  
> +/* swap contents */
> +void rangeset_swap(struct rangeset *a, struct rangeset *b);
> +

Please put this above the printing ones.

Jan

Re: [PATCH 1/2] implement is_hardware_domain using hardware_domain global

[Ian Campbell]

On Mon, 2014-04-14 at 17:23 -0400, Daniel De Graaf wrote:
> This requires setting the hardware_domain variable earlier in
> domain_create so that functions called from it (primarily in
> arch_domain_create) observe the correct results when they call
> is_hardware_domain.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

For the ARM side:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v4 2/2] allow hardware domain != dom0

[Andrew Cooper]

On 14/04/14 22:23, Daniel De Graaf wrote:
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3c05711..11c905a 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -61,6 +61,11 @@ struct domain *domain_list;
>  
>  struct domain *hardware_domain __read_mostly;
>  
> +#ifdef CONFIG_LATE_HWDOM
> +domid_t hardware_domid __read_mostly;
> +integer_param("hardware_dom", hardware_domid);
> +#endif
> +

Is it worth putting a custom_param() in here which clamps hardware_domid
below FIRST_RESERVED_DOMID, or allow anyone specifying
hardware_dom=0xffff to keep all the broken pieces they find?  Aliasing
the magic domids is sure to break things.

~Andrew

Re: [PATCH v4 2/2] allow hardware domain != dom0

[Daniel De Graaf]

On 04/15/2014 10:45 AM, Andrew Cooper wrote:
> On 14/04/14 22:23, Daniel De Graaf wrote:
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 3c05711..11c905a 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -61,6 +61,11 @@ struct domain *domain_list;
>>
>>   struct domain *hardware_domain __read_mostly;
>>
>> +#ifdef CONFIG_LATE_HWDOM
>> +domid_t hardware_domid __read_mostly;
>> +integer_param("hardware_dom", hardware_domid);
>> +#endif
>> +
>
> Is it worth putting a custom_param() in here which clamps hardware_domid
> below FIRST_RESERVED_DOMID, or allow anyone specifying
> hardware_dom=0xffff to keep all the broken pieces they find?  Aliasing
> the magic domids is sure to break things.
>
> ~Andrew

I'm currently of the opinion that a custom_param is overkill to prevent
users from deliberately doing bad things, but could easily write one if
others think that it would be helpful.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH v4 2/2] allow hardware domain != dom0

[Andrew Cooper]

On 15/04/2014 23:07, Daniel De Graaf wrote:
> On 04/15/2014 10:45 AM, Andrew Cooper wrote:
>> On 14/04/14 22:23, Daniel De Graaf wrote:
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index 3c05711..11c905a 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -61,6 +61,11 @@ struct domain *domain_list;
>>>
>>>   struct domain *hardware_domain __read_mostly;
>>>
>>> +#ifdef CONFIG_LATE_HWDOM
>>> +domid_t hardware_domid __read_mostly;
>>> +integer_param("hardware_dom", hardware_domid);
>>> +#endif
>>> +
>>
>> Is it worth putting a custom_param() in here which clamps hardware_domid
>> below FIRST_RESERVED_DOMID, or allow anyone specifying
>> hardware_dom=0xffff to keep all the broken pieces they find?  Aliasing
>> the magic domids is sure to break things.
>>
>> ~Andrew
>
> I'm currently of the opinion that a custom_param is overkill to prevent
> users from deliberately doing bad things, but could easily write one if
> others think that it would be helpful.
>

I suppose the answer depends on how subtle the breakage will be if the
user gets it accidentally wrong.  Given that slightly over half the
domid space is reserved and domid_t signed value, I can forsee subtle
bugs if a domid_t ever used as an array index, as well as very unsubtle
breakage if the user aliases one of the magic domids.

On the balance, a custom_param() is probably worthwhile for peace of mind.

~Andrew

Re: [PATCH v4 2/2] allow hardware domain != dom0

[Jan Beulich]

>>> On 16.04.14 at 01:37, <andrew.cooper3@citrix.com> wrote:
> On 15/04/2014 23:07, Daniel De Graaf wrote:
>> On 04/15/2014 10:45 AM, Andrew Cooper wrote:
>>> On 14/04/14 22:23, Daniel De Graaf wrote:
>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>> index 3c05711..11c905a 100644
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -61,6 +61,11 @@ struct domain *domain_list;
>>>>
>>>>   struct domain *hardware_domain __read_mostly;
>>>>
>>>> +#ifdef CONFIG_LATE_HWDOM
>>>> +domid_t hardware_domid __read_mostly;
>>>> +integer_param("hardware_dom", hardware_domid);
>>>> +#endif
>>>> +
>>>
>>> Is it worth putting a custom_param() in here which clamps hardware_domid
>>> below FIRST_RESERVED_DOMID, or allow anyone specifying
>>> hardware_dom=0xffff to keep all the broken pieces they find?  Aliasing
>>> the magic domids is sure to break things.
>>>
>>> ~Andrew
>>
>> I'm currently of the opinion that a custom_param is overkill to prevent
>> users from deliberately doing bad things, but could easily write one if
>> others think that it would be helpful.
>>
> 
> I suppose the answer depends on how subtle the breakage will be if the
> user gets it accidentally wrong.  Given that slightly over half the
> domid space is reserved and domid_t signed value, I can forsee subtle
> bugs if a domid_t ever used as an array index, as well as very unsubtle
> breakage if the user aliases one of the magic domids.
> 
> On the balance, a custom_param() is probably worthwhile for peace of mind.

Why can't we just BUG_ON() or panic() the first time domain_create()
gets to see the out of range domain ID?

Jan

Re: [PATCH v4 2/2] allow hardware domain != dom0

[Andrew Cooper]

On 16/04/14 10:08, Jan Beulich wrote:
>>>> On 16.04.14 at 01:37, <andrew.cooper3@citrix.com> wrote:
>> On 15/04/2014 23:07, Daniel De Graaf wrote:
>>> On 04/15/2014 10:45 AM, Andrew Cooper wrote:
>>>> On 14/04/14 22:23, Daniel De Graaf wrote:
>>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>>> index 3c05711..11c905a 100644
>>>>> --- a/xen/common/domain.c
>>>>> +++ b/xen/common/domain.c
>>>>> @@ -61,6 +61,11 @@ struct domain *domain_list;
>>>>>
>>>>>   struct domain *hardware_domain __read_mostly;
>>>>>
>>>>> +#ifdef CONFIG_LATE_HWDOM
>>>>> +domid_t hardware_domid __read_mostly;
>>>>> +integer_param("hardware_dom", hardware_domid);
>>>>> +#endif
>>>>> +
>>>> Is it worth putting a custom_param() in here which clamps hardware_domid
>>>> below FIRST_RESERVED_DOMID, or allow anyone specifying
>>>> hardware_dom=0xffff to keep all the broken pieces they find?  Aliasing
>>>> the magic domids is sure to break things.
>>>>
>>>> ~Andrew
>>> I'm currently of the opinion that a custom_param is overkill to prevent
>>> users from deliberately doing bad things, but could easily write one if
>>> others think that it would be helpful.
>>>
>> I suppose the answer depends on how subtle the breakage will be if the
>> user gets it accidentally wrong.  Given that slightly over half the
>> domid space is reserved and domid_t signed value, I can forsee subtle
>> bugs if a domid_t ever used as an array index, as well as very unsubtle
>> breakage if the user aliases one of the magic domids.
>>
>> On the balance, a custom_param() is probably worthwhile for peace of mind.
> Why can't we just BUG_ON() or panic() the first time domain_create()
> gets to see the out of range domain ID?
>
> Jan
>

That would also work.

~Andrew