[RFC 0/6] Hyperlaunch domain roles and capabilities

[RFC 0/6] Hyperlaunch domain roles and capabilities

[Daniel P. Smith]

A goal of the hyperlaunch effort was to solidify the concept of the different
types of domains the hypervisor has some notion around. The initial approach
was to formalize these types as roles enforced through the XSM framework. In
this RFC, a simpler approach is taken to lay a foundation of domain roles and
assignable capabilities.

The approach in this series is to collapse the relevant bools in struct domain
into a pair of bit flag entries that represent roles and capabilities that a
domain is assigned.

Daniel P. Smith (6):
  dom0: replace explict zero checks
  roles: provide abstraction for the possible domain roles
  roles: add a role for xenstore domain
  capabilities: introduce console io as a domain capability
  capabilities: add dom0 cpu faulting disable
  capabilities: convert attach debugger into a capability

 xen/arch/arm/domain_build.c     |  6 ++-
 xen/arch/x86/cpu-policy.c       |  2 +-
 xen/arch/x86/cpu/common.c       | 82 ++++++++++++++++-----------------
 xen/arch/x86/hvm/svm/svm.c      |  8 ++--
 xen/arch/x86/hvm/vmx/realmode.c |  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c     |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c      | 10 ++--
 xen/arch/x86/setup.c            |  6 +++
 xen/arch/x86/traps.c            |  6 ++-
 xen/common/domain.c             | 21 +++++++--
 xen/common/domctl.c             |  6 ++-
 xen/common/sched/arinc653.c     |  2 +-
 xen/common/sched/core.c         |  4 +-
 xen/include/xen/sched.h         | 58 +++++++++++++++++++----
 xen/include/xsm/dummy.h         |  6 +--
 xen/xsm/flask/hooks.c           | 12 ++---
 16 files changed, 150 insertions(+), 83 deletions(-)

-- 
2.20.1

[RFC 1/6] dom0: replace explict zero checks

[Daniel P. Smith]

A legacy concept is that the initial domain will have a domain id of zero. As a
result there are places where a check that a domain is the inital domain is
determined by an explicit check that the domid is zero.

This commit seeks to abstract this check into a function call and replace all
check locations with the function call.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/domain.c         | 4 ++--
 xen/common/sched/arinc653.c | 2 +-
 xen/common/sched/core.c     | 4 ++--
 xen/include/xen/sched.h     | 7 +++++++
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 304aa04fa6..8fb3c052f5 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -309,7 +309,7 @@ static int late_hwdom_init(struct domain *d)
     struct domain *dom0;
     int rv;
 
-    if ( d != hardware_domain || d->domain_id == 0 )
+    if ( d != hardware_domain || is_initial_domain(d) )
         return 0;
 
     rv = xsm_init_hardware_domain(XSM_HOOK, d);
@@ -612,7 +612,7 @@ struct domain *domain_create(domid_t domid,
     d->is_privileged = flags & CDF_privileged;
 
     /* Sort out our idea of is_hardware_domain(). */
-    if ( domid == 0 || domid == hardware_domid )
+    if ( is_initial_domain(d) || domid == hardware_domid )
     {
         if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
             panic("The value of hardware_dom must be a valid domain ID\n");
diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
index a82c0d7314..31e8270af3 100644
--- a/xen/common/sched/arinc653.c
+++ b/xen/common/sched/arinc653.c
@@ -404,7 +404,7 @@ a653sched_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
      * Add every one of dom0's units to the schedule, as long as there are
      * slots available.
      */
-    if ( unit->domain->domain_id == 0 )
+    if ( is_initial_domain(unit->domain) )
     {
         entry = sched_priv->num_schedule_entries;
 
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 022f548652..210ad30f94 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -585,7 +585,7 @@ int sched_init_vcpu(struct vcpu *v)
          */
         sched_set_affinity(unit, cpumask_of(0), cpumask_of(0));
     }
-    else if ( d->domain_id == 0 && opt_dom0_vcpus_pin )
+    else if ( is_initial_domain(d) && opt_dom0_vcpus_pin )
     {
         /*
          * If dom0_vcpus_pin is specified, dom0 vCPUs are pinned 1:1 to
@@ -594,7 +594,7 @@ int sched_init_vcpu(struct vcpu *v)
         sched_set_affinity(unit, cpumask_of(processor), &cpumask_all);
     }
 #ifdef CONFIG_X86
-    else if ( d->domain_id == 0 )
+    else if ( is_initial_domain(d) )
     {
         /*
          * In absence of dom0_vcpus_pin instead, the hard and soft affinity of
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 854f3e32c0..a9276a7bed 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1058,6 +1058,13 @@ void scheduler_disable(void);
 void watchdog_domain_init(struct domain *d);
 void watchdog_domain_destroy(struct domain *d);
 
+static always_inline bool is_initial_domain(const struct domain *d)
+{
+    static int init_domain_id = 0;
+
+    return d->domain_id == init_domain_id;
+}
+
 /*
  * Use this check when the following are both true:
  *  - Using this feature or interface requires full access to the hardware
-- 
2.20.1

[RFC 2/6] roles: provide abstraction for the possible domain roles

[Daniel P. Smith]

The existing concepts such as unbounded domain, ie. all powerful, control
domain and hardware domain are, effectively, roles the domains provide for the
system. Currently, these are represented with booleans within `struct domain`
or global domid variables that are compared against. This patch begins to
formalize these roles by replacing the `is_control` and `is_console`, along
with expanding the check against the global `hardware_domain` with a single
encapsulating role attribute in `struct domain`.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/arm/domain_build.c |  2 ++
 xen/arch/x86/setup.c        |  2 ++
 xen/common/domain.c         | 14 +++++++++++++-
 xen/include/xen/sched.h     | 16 +++++++++-------
 xen/include/xsm/dummy.h     |  4 ++--
 xen/xsm/flask/hooks.c       | 12 ++++++------
 6 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 39b4ee03a5..51b4daefe1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -4201,6 +4201,8 @@ void __init create_dom0(void)
     if ( IS_ERR(dom0) )
         panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
 
+    dom0->role |= ROLE_UNBOUNDED_DOMAIN;
+
     if ( alloc_dom0_vcpu0(dom0) == NULL )
         panic("Error creating domain 0 vcpu0\n");
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2dbe9857aa..4e20edc3bf 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const module_t *image,
     if ( IS_ERR(d) )
         panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
 
+    d->role |= ROLE_UNBOUNDED_DOMAIN;
+
     init_dom0_cpuid_policy(d);
 
     if ( alloc_dom0_vcpu0(d) == NULL )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8fb3c052f5..0ff1d52e3d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
     setup_io_bitmap(dom0);
 #endif
 
+    /*
+     * "dom0" may have been created under the unbounded role, demote it from
+     * that role, reducing it to the control domain role and any other roles it
+     * may have been given.
+     */
+    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
+    dom0->role |= ROLE_CONTROL_DOMAIN;
+
     rcu_unlock_domain(dom0);
 
     iommu_hwdom_init(d);
@@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
     }
 
     /* Sort out our idea of is_control_domain(). */
-    d->is_privileged = flags & CDF_privileged;
+    if ( flags & CDF_privileged )
+        d->role |= ROLE_CONTROL_DOMAIN;
+    else
+        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
 
     /* Sort out our idea of is_hardware_domain(). */
     if ( is_initial_domain(d) || domid == hardware_domid )
@@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
 
         old_hwdom = hardware_domain;
         hardware_domain = d;
+        d->role |= ROLE_HARDWARE_DOMAIN;
     }
 
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index a9276a7bed..695f240326 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -467,8 +467,10 @@ struct domain
 #endif
     /* is node-affinity automatically computed? */
     bool             auto_node_affinity;
-    /* Is this guest fully privileged (aka dom0)? */
-    bool             is_privileged;
+#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
+#define ROLE_CONTROL_DOMAIN    (1U<<1)
+#define ROLE_HARDWARE_DOMAIN   (1U<<2)
+    uint8_t          role;
     /* Can this guest access the Xen console? */
     bool             is_console;
     /* Is this guest being debugged by dom0? */
@@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
 
 static always_inline bool is_initial_domain(const struct domain *d)
 {
-    static int init_domain_id = 0;
-
-    return d->domain_id == init_domain_id;
+    return d->role & ROLE_UNBOUNDED_DOMAIN;
 }
 
 /*
@@ -1076,7 +1076,8 @@ static always_inline bool is_hardware_domain(const struct domain *d)
     if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
         return false;
 
-    return evaluate_nospec(d == hardware_domain);
+    return evaluate_nospec(((d->role & ROLE_HARDWARE_DOMAIN) ||
+        is_initial_domain(d)) && (d == hardware_domain));
 }
 
 /* This check is for functionality specific to a control domain */
@@ -1085,7 +1086,8 @@ static always_inline bool is_control_domain(const struct domain *d)
     if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
         return false;
 
-    return evaluate_nospec(d->is_privileged);
+    return evaluate_nospec((d->role & ROLE_CONTROL_DOMAIN) ||
+        is_initial_domain(d));
 }
 
 #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 8671af1ba4..18f1ddd127 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -108,7 +108,7 @@ static XSM_INLINE int cf_check xsm_set_system_active(void)
 {
     struct domain *d = current->domain;
 
-    ASSERT(d->is_privileged);
+    ASSERT(d->role & ROLE_CONTROL_DOMAIN);
 
     if ( d->domain_id != DOMID_IDLE )
     {
@@ -116,7 +116,7 @@ static XSM_INLINE int cf_check xsm_set_system_active(void)
         return -EPERM;
     }
 
-    d->is_privileged = false;
+    d->role &= ~ROLE_CONTROL_DOMAIN;
 
     return 0;
 }
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78225f68c1..0a31719f43 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -193,7 +193,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
     default:
         if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
         {
-            if ( d->is_privileged )
+            if ( d->role & ROLE_CONTROL_DOMAIN )
                 dsec->sid = SECINITSID_DOM0;
             else if ( pv_shim )
                 dsec->sid = SECINITSID_DOMU;
@@ -213,7 +213,7 @@ static int cf_check flask_set_system_active(void)
 
     dsec = d->ssid;
 
-    ASSERT(d->is_privileged);
+    ASSERT(d->role & ROLE_CONTROL_DOMAIN);
     ASSERT(dsec->sid == SECINITSID_XENBOOT);
     ASSERT(dsec->self_sid == SECINITSID_XENBOOT);
 
@@ -224,11 +224,11 @@ static int cf_check flask_set_system_active(void)
     }
 
     /*
-     * While is_privileged has no significant meaning under flask, set to false
-     * as is_privileged is not only used for a privilege check but also as a
-     * type of domain check, specifically if the domain is the control domain.
+     * While domain roles have no significant meaning under flask, mask out
+     * control domain role as it is not only used for a privilege check but
+     * also as a type of domain check.
      */
-    d->is_privileged = false;
+    d->role &= ~ROLE_CONTROL_DOMAIN;
 
     dsec->self_sid = dsec->sid = SECINITSID_XEN;
 
-- 
2.20.1

[RFC 3/6] roles: add a role for xenstore domain

[Daniel P. Smith]

Expand the possible roles for a domain to include a role for the Xenstore
domain.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/domain.c     | 3 +++
 xen/include/xen/sched.h | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0ff1d52e3d..dbf055c559 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -633,6 +633,9 @@ struct domain *domain_create(domid_t domid,
         d->role |= ROLE_HARDWARE_DOMAIN;
     }
 
+    if ( d->options & XEN_DOMCTL_CDF_xs_domain )
+        d->role |= ROLE_XENSTORE_DOMAIN;
+
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 695f240326..ec0f9baff6 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -470,6 +470,7 @@ struct domain
 #define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
 #define ROLE_CONTROL_DOMAIN    (1U<<1)
 #define ROLE_HARDWARE_DOMAIN   (1U<<2)
+#define ROLE_XENSTORE_DOMAIN   (1U<<3)
     uint8_t          role;
     /* Can this guest access the Xen console? */
     bool             is_console;
@@ -1165,7 +1166,7 @@ static inline bool is_vcpu_online(const struct vcpu *v)
 
 static inline bool is_xenstore_domain(const struct domain *d)
 {
-    return d->options & XEN_DOMCTL_CDF_xs_domain;
+    return d->role & ROLE_XENSTORE_DOMAIN;
 }
 
 static always_inline bool is_iommu_enabled(const struct domain *d)
-- 
2.20.1

[RFC 4/6] capabilities: introduce console io as a domain capability

[Daniel P. Smith]

The field `is_console` suggests that the field represents a state of being or
posession, not that it reflects the privilege to access the console. In this
patch the field is renamed to capabilities to encapsulate the capabilities a
domain has been granted. The first capability being the ability to read/write
the Xen console.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/arm/domain_build.c |  4 +++-
 xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
 xen/include/xsm/dummy.h     |  2 +-
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 51b4daefe1..ad7432b029 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -4076,7 +4076,9 @@ void __init create_domUs(void)
             panic("Error creating domain %s (rc = %ld)\n",
                   dt_node_name(node), PTR_ERR(d));
 
-        d->is_console = true;
+        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )
+            printk("failed setting console_io on %pd\n", d);
+
         dt_device_set_used_by(node, d->domain_id);
 
         rc = construct_domU(d, node);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ec0f9baff6..b04fbe0565 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -472,8 +472,8 @@ struct domain
 #define ROLE_HARDWARE_DOMAIN   (1U<<2)
 #define ROLE_XENSTORE_DOMAIN   (1U<<3)
     uint8_t          role;
-    /* Can this guest access the Xen console? */
-    bool             is_console;
+#define CAP_CONSOLE_IO  (1U<<0)
+    uint8_t          capabilities;
     /* Is this guest being debugged by dom0? */
     bool             debugger_attached;
     /*
@@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu *v)
     return is_hvm_domain(v->domain);
 }
 
+static always_inline bool domain_has_cap(
+    const struct domain *d, uint8_t cap)
+{
+    return d->capabilities & cap;
+}
+
+static always_inline bool domain_set_cap(
+    struct domain *d, uint8_t cap)
+{
+    switch ( cap )
+    {
+    case CAP_CONSOLE_IO:
+        d->capabilities |= cap;
+        break;
+    default:
+        return false;
+    }
+
+    return domain_has_cap(d, cap);
+}
+
 static always_inline bool hap_enabled(const struct domain *d)
 {
     /* sanitise_domain_config() rejects HAP && !HVM */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 18f1ddd127..067ff1d111 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -268,7 +268,7 @@ static XSM_INLINE int cf_check xsm_console_io(
     XSM_DEFAULT_ARG struct domain *d, int cmd)
 {
     XSM_ASSERT_ACTION(XSM_OTHER);
-    if ( d->is_console )
+    if ( domain_has_cap(d, CAP_CONSOLE_IO) )
         return xsm_default_action(XSM_HOOK, d, NULL);
 #ifdef CONFIG_VERBOSE_DEBUG
     if ( cmd == CONSOLEIO_write )
-- 
2.20.1

[RFC 5/6] capabilities: add dom0 cpu faulting disable

[Daniel P. Smith]

This encapsulates disableing cpu faulting for PV dom0 as a capability.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/cpu-policy.c |  2 +-
 xen/arch/x86/cpu/common.c | 82 +++++++++++++++++++--------------------
 xen/arch/x86/setup.c      |  4 ++
 xen/include/xen/sched.h   |  8 +++-
 4 files changed, 52 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 1f954d4e59..42c3193938 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -912,7 +912,7 @@ void __init init_dom0_cpuid_policy(struct domain *d)
      * If the domain is getting unfiltered CPUID, don't let the guest kernel
      * play with CPUID faulting either, as Xen's CPUID path won't cope.
      */
-    if ( !opt_dom0_cpuid_faulting && is_control_domain(d) && is_pv_domain(d) )
+    if ( domain_has_cap(d, CAP_DISABLE_CPU_FAULT) )
         p->platform_info.cpuid_faulting = false;
 
     recalculate_cpuid_policy(d);
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cfcdaace12..937581e353 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -164,48 +164,46 @@ static void set_cpuid_faulting(bool enable)
 
 void ctxt_switch_levelling(const struct vcpu *next)
 {
-	const struct domain *nextd = next ? next->domain : NULL;
-	bool enable_cpuid_faulting;
-
-	if (cpu_has_cpuid_faulting ||
-	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
-		/*
-		 * No need to alter the faulting setting if we are switching
-		 * to idle; it won't affect any code running in idle context.
-		 */
-		if (nextd && is_idle_domain(nextd))
-			return;
-		/*
-		 * We *should* be enabling faulting for PV control domains.
-		 *
-		 * The domain builder has now been updated to not depend on
-		 * seeing host CPUID values.  This makes it compatible with
-		 * PVH toolstack domains, and lets us enable faulting by
-		 * default for all PV domains.
-		 *
-		 * However, as PV control domains have never had faulting
-		 * enforced on them before, there might plausibly be other
-		 * dependenices on host CPUID data.  Therefore, we have left
-		 * an interim escape hatch in the form of
-		 * `dom0=no-cpuid-faulting` to restore the older behaviour.
-		 */
-		enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting ||
-		                                  !is_control_domain(nextd) ||
-		                                  !is_pv_domain(nextd)) &&
-		                        (is_pv_domain(nextd) ||
-		                         next->arch.msrs->
-		                         misc_features_enables.cpuid_faulting);
-
-		if (cpu_has_cpuid_faulting)
-			set_cpuid_faulting(enable_cpuid_faulting);
-		else
-			amd_set_cpuid_user_dis(enable_cpuid_faulting);
-
-		return;
-	}
-
-	if (ctxt_switch_masking)
-		alternative_vcall(ctxt_switch_masking, next);
+    const struct domain *nextd = next ? next->domain : NULL;
+    bool enable_cpuid_faulting;
+
+    if ( cpu_has_cpuid_faulting ||
+         boot_cpu_has(X86_FEATURE_CPUID_USER_DIS) ) {
+        /*
+        * No need to alter the faulting setting if we are switching
+        * to idle; it won't affect any code running in idle context.
+        */
+        if (nextd && is_idle_domain(nextd))
+            return;
+        /*
+        * We *should* be enabling faulting for PV control domains.
+        *
+        * The domain builder has now been updated to not depend on
+        * seeing host CPUID values.  This makes it compatible with
+        * PVH toolstack domains, and lets us enable faulting by
+        * default for all PV domains.
+        *
+        * However, as PV control domains have never had faulting
+        * enforced on them before, there might plausibly be other
+        * dependenices on host CPUID data.  Therefore, we have left
+        * an interim escape hatch in the form of
+        * `dom0=no-cpuid-faulting` to restore the older behaviour.
+        */
+        enable_cpuid_faulting = nextd &&
+            domain_has_cap(nextd, CAP_DISABLE_CPU_FAULT) &&
+            (is_pv_domain(nextd) ||
+            next->arch.msrs->misc_features_enables.cpuid_faulting);
+
+        if (cpu_has_cpuid_faulting)
+            set_cpuid_faulting(enable_cpuid_faulting);
+        else
+            amd_set_cpuid_user_dis(enable_cpuid_faulting);
+
+        return;
+    }
+
+    if (ctxt_switch_masking)
+        alternative_vcall(ctxt_switch_masking, next);
 }
 
 bool_t opt_cpu_info;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4e20edc3bf..d65144da01 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image,
 
     d->role |= ROLE_UNBOUNDED_DOMAIN;
 
+    if ( !opt_dom0_cpuid_faulting &&
+         !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) )
+        printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d);
+
     init_dom0_cpuid_policy(d);
 
     if ( alloc_dom0_vcpu0(d) == NULL )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b04fbe0565..ebfe65cd73 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -472,7 +472,8 @@ struct domain
 #define ROLE_HARDWARE_DOMAIN   (1U<<2)
 #define ROLE_XENSTORE_DOMAIN   (1U<<3)
     uint8_t          role;
-#define CAP_CONSOLE_IO  (1U<<0)
+#define CAP_CONSOLE_IO         (1U<<0)
+#define CAP_DISABLE_CPU_FAULT  (1U<<1)
     uint8_t          capabilities;
     /* Is this guest being debugged by dom0? */
     bool             debugger_attached;
@@ -1160,6 +1161,11 @@ static always_inline bool domain_set_cap(
     case CAP_CONSOLE_IO:
         d->capabilities |= cap;
         break;
+    case CAP_DISABLE_CPU_FAULT:
+        /* Disabling cpu faulting is only allowed for a PV control domain. */
+        if ( is_pv_domain(d) && is_control_domain(d) )
+            d->capabilities |= cap;
+        break;
     default:
         return false;
     }
-- 
2.20.1

[RFC 6/6] capabilities: convert attach debugger into a capability

[Daniel P. Smith]

Expresses the ability to attach a debugger as a capability that a domain can be
provisioned.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/hvm/svm/svm.c      |  8 ++++----
 xen/arch/x86/hvm/vmx/realmode.c |  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c     |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c      | 10 +++++-----
 xen/arch/x86/traps.c            |  6 ++++--
 xen/common/domctl.c             |  6 ++++--
 xen/include/xen/sched.h         |  9 ++++++---
 7 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 27170213ae..9872804d39 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -999,7 +999,7 @@ static void noreturn cf_check svm_do_resume(void)
 {
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
-    bool debug_state = (v->domain->debugger_attached ||
+    bool debug_state = (domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) ||
                         v->domain->arch.monitor.software_breakpoint_enabled ||
                         v->domain->arch.monitor.debug_exception_enabled);
     bool_t vcpu_guestmode = 0;
@@ -1335,7 +1335,7 @@ static void cf_check svm_inject_event(const struct x86_event *event)
         }
         /* fall through */
     case X86_EXC_BP:
-        if ( curr->domain->debugger_attached )
+        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
         {
             /* Debug/Int3: Trap to debugger. */
             domain_pause_for_debugger();
@@ -2732,7 +2732,7 @@ void svm_vmexit_handler(void)
 
     case VMEXIT_ICEBP:
     case VMEXIT_EXCEPTION_DB:
-        if ( !v->domain->debugger_attached )
+        if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
         {
             unsigned int trap_type;
 
@@ -2769,7 +2769,7 @@ void svm_vmexit_handler(void)
         if ( insn_len == 0 )
              break;
 
-        if ( v->domain->debugger_attached )
+        if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
         {
             /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
             __update_guest_eip(regs, insn_len);
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index ff44ddcfa6..f761026a9d 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -121,7 +121,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
 
     if ( rc == X86EMUL_EXCEPTION )
     {
-        if ( unlikely(curr->domain->debugger_attached) &&
+        if ( unlikely(domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH)) &&
              ((hvmemul_ctxt->ctxt.event.vector == X86_EXC_DB) ||
               (hvmemul_ctxt->ctxt.event.vector == X86_EXC_BP)) )
         {
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 13719cc923..9474869018 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1912,7 +1912,7 @@ void cf_check vmx_do_resume(void)
         hvm_asid_flush_vcpu(v);
     }
 
-    debug_state = v->domain->debugger_attached
+    debug_state = domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH)
                   || v->domain->arch.monitor.software_breakpoint_enabled
                   || v->domain->arch.monitor.singlestep_enabled;
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7ec44018d4..5069e3cbf3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2041,7 +2041,7 @@ static void cf_check vmx_inject_event(const struct x86_event *event)
             break;
         /* fall through */
     case X86_EXC_BP:
-        if ( curr->domain->debugger_attached )
+        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
         {
             /* Debug/Int3: Trap to debugger. */
             domain_pause_for_debugger();
@@ -2121,7 +2121,7 @@ static void cf_check vmx_set_info_guest(struct vcpu *v)
      * immediately vmexit and hence make no progress.
      */
     __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
-    if ( v->domain->debugger_attached &&
+    if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) &&
          (v->arch.user_regs.eflags & X86_EFLAGS_TF) &&
          (intr_shadow & VMX_INTR_SHADOW_STI) )
     {
@@ -4283,7 +4283,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                 }
             }
 
-            if ( !v->domain->debugger_attached )
+            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
             {
                 unsigned long insn_len = 0;
                 int rc;
@@ -4307,7 +4307,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             break;
         case X86_EXC_BP:
             HVMTRACE_1D(TRAP, vector);
-            if ( !v->domain->debugger_attached )
+            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
             {
                 unsigned long insn_len;
                 int rc;
@@ -4647,7 +4647,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                               HVM_MONITOR_SINGLESTEP_BREAKPOINT,
                               0, 0, 0);
 
-            if ( v->domain->debugger_attached )
+            if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
                 domain_pause_for_debugger();
         }
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4229bda159..041ced35ea 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1214,7 +1214,8 @@ void do_int3(struct cpu_user_regs *regs)
         return;
     }
 
-    if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
+    if ( guest_kernel_mode(curr, regs) &&
+         domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
     {
         curr->arch.gdbsx_vcpu_event = X86_EXC_BP;
         domain_pause_for_debugger();
@@ -1995,7 +1996,8 @@ void do_debug(struct cpu_user_regs *regs)
     v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
     v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
 
-    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
+    if ( guest_kernel_mode(v, regs) &&
+         domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
     {
         domain_pause_for_debugger();
         return;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 505e29c0dc..895ddf0600 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -99,7 +99,8 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
         ((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) |
+        (domain_has_cap(d, CAP_DEBUGGER_ATTACH) ?
+                                          XEN_DOMINF_debugged  : 0) |
         (is_xenstore_domain(d)          ? XEN_DOMINF_xs_domain : 0) |
         (is_hvm_domain(d)               ? XEN_DOMINF_hvm_guest : 0) |
         d->shutdown_code << XEN_DOMINF_shutdownshift;
@@ -643,7 +644,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         else
         {
             domain_pause(d);
-            d->debugger_attached = !!op->u.setdebugging.enable;
+            if ( !!op->u.setdebugging.enable )
+                domain_set_cap(d, CAP_DEBUGGER_ATTACH);
             domain_unpause(d); /* causes guest to latch new status */
         }
         break;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ebfe65cd73..47eadb5008 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -474,9 +474,8 @@ struct domain
     uint8_t          role;
 #define CAP_CONSOLE_IO         (1U<<0)
 #define CAP_DISABLE_CPU_FAULT  (1U<<1)
-    uint8_t          capabilities;
-    /* Is this guest being debugged by dom0? */
-    bool             debugger_attached;
+#define CAP_DEBUGGER_ATTACH    (1U<<2)
+    uint16_t         capabilities;
     /*
      * Set to true at the very end of domain creation, when the domain is
      * unpaused for the first time by the systemcontroller.
@@ -1166,6 +1165,10 @@ static always_inline bool domain_set_cap(
         if ( is_pv_domain(d) && is_control_domain(d) )
             d->capabilities |= cap;
         break;
+    case CAP_DEBUGGER_ATTACH:
+        if ( !is_control_domain(d) )
+            d->capabilities |= cap;
+        break;
     default:
         return false;
     }
-- 
2.20.1

Re: [RFC 1/6] dom0: replace explict zero checks

[Stefano Stabellini]

On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> A legacy concept is that the initial domain will have a domain id of zero. As a
> result there are places where a check that a domain is the inital domain is
> determined by an explicit check that the domid is zero.
> 
> This commit seeks to abstract this check into a function call and replace all
> check locations with the function call.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Thanks for the patch, this is a good cleanup!


> ---
>  xen/common/domain.c         | 4 ++--
>  xen/common/sched/arinc653.c | 2 +-
>  xen/common/sched/core.c     | 4 ++--
>  xen/include/xen/sched.h     | 7 +++++++
>  4 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 304aa04fa6..8fb3c052f5 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -309,7 +309,7 @@ static int late_hwdom_init(struct domain *d)
>      struct domain *dom0;
>      int rv;
>  
> -    if ( d != hardware_domain || d->domain_id == 0 )
> +    if ( d != hardware_domain || is_initial_domain(d) )
>          return 0;
>  
>      rv = xsm_init_hardware_domain(XSM_HOOK, d);
> @@ -612,7 +612,7 @@ struct domain *domain_create(domid_t domid,
>      d->is_privileged = flags & CDF_privileged;
>  
>      /* Sort out our idea of is_hardware_domain(). */
> -    if ( domid == 0 || domid == hardware_domid )
> +    if ( is_initial_domain(d) || domid == hardware_domid )
>      {
>          if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
>              panic("The value of hardware_dom must be a valid domain ID\n");
> diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
> index a82c0d7314..31e8270af3 100644
> --- a/xen/common/sched/arinc653.c
> +++ b/xen/common/sched/arinc653.c
> @@ -404,7 +404,7 @@ a653sched_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
>       * Add every one of dom0's units to the schedule, as long as there are
>       * slots available.
>       */
> -    if ( unit->domain->domain_id == 0 )
> +    if ( is_initial_domain(unit->domain) )
>      {
>          entry = sched_priv->num_schedule_entries;
>  
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 022f548652..210ad30f94 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -585,7 +585,7 @@ int sched_init_vcpu(struct vcpu *v)
>           */
>          sched_set_affinity(unit, cpumask_of(0), cpumask_of(0));
>      }
> -    else if ( d->domain_id == 0 && opt_dom0_vcpus_pin )
> +    else if ( is_initial_domain(d) && opt_dom0_vcpus_pin )
>      {
>          /*
>           * If dom0_vcpus_pin is specified, dom0 vCPUs are pinned 1:1 to
> @@ -594,7 +594,7 @@ int sched_init_vcpu(struct vcpu *v)
>          sched_set_affinity(unit, cpumask_of(processor), &cpumask_all);
>      }
>  #ifdef CONFIG_X86
> -    else if ( d->domain_id == 0 )
> +    else if ( is_initial_domain(d) )
>      {
>          /*
>           * In absence of dom0_vcpus_pin instead, the hard and soft affinity of
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 854f3e32c0..a9276a7bed 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>  void watchdog_domain_init(struct domain *d);
>  void watchdog_domain_destroy(struct domain *d);
>  
> +static always_inline bool is_initial_domain(const struct domain *d)

I know you used always_inline only because is_hardware_domain just below
also uses it, but I wonder if we need it.

Also, Robero, it looks like always_inline is missing from
docs/misra/C-language-toolchain.rst? Or do we plan to get rid of it?


> +{
> +    static int init_domain_id = 0;

I take it is done this way because you plan to make it configurable?


> +    return d->domain_id == init_domain_id;
> +}
> +
>  /*
>   * Use this check when the following are both true:
>   *  - Using this feature or interface requires full access to the hardware
> -- 
> 2.20.1
> 

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Stefano Stabellini]

On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> The existing concepts such as unbounded domain, ie. all powerful, control
> domain and hardware domain are, effectively, roles the domains provide for the
> system. Currently, these are represented with booleans within `struct domain`
> or global domid variables that are compared against. This patch begins to
> formalize these roles by replacing the `is_control` and `is_console`, along
> with expanding the check against the global `hardware_domain` with a single
> encapsulating role attribute in `struct domain`.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

This is definitely heading in the right direction


> ---
>  xen/arch/arm/domain_build.c |  2 ++
>  xen/arch/x86/setup.c        |  2 ++
>  xen/common/domain.c         | 14 +++++++++++++-
>  xen/include/xen/sched.h     | 16 +++++++++-------
>  xen/include/xsm/dummy.h     |  4 ++--
>  xen/xsm/flask/hooks.c       | 12 ++++++------
>  6 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 39b4ee03a5..51b4daefe1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -4201,6 +4201,8 @@ void __init create_dom0(void)
>      if ( IS_ERR(dom0) )
>          panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
>  
> +    dom0->role |= ROLE_UNBOUNDED_DOMAIN;

I am not a fan of "UNBOUNDED". Maybe "PRIMARY"? "PRIVILEGED"? "SUPER"?
"ROOT"?

I also recognize I am not good at naming things so I'll stop here and
let other provide better feedback :-)


Also, do we actually need unbounded given that it gets replaced with
control_domain pretty soon?

I am asking because I think that at least from a safety perspective it
would be a problem to run a domain as "unbounded". In a safety system,
we wouldn't want anything to be unbounded, not even temporarily at boot.
If "unbounded" is removed before running dom0, then of course it is no
problem because actually dom0 never gets to run with "unbounded" set.

(I am currently thinking of solving the privilege issue by using XSM and
removing most privileges from Dom0.) 


>      if ( alloc_dom0_vcpu0(dom0) == NULL )
>          panic("Error creating domain 0 vcpu0\n");
>  
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 2dbe9857aa..4e20edc3bf 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const module_t *image,
>      if ( IS_ERR(d) )
>          panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
>  
> +    d->role |= ROLE_UNBOUNDED_DOMAIN;
> +
>      init_dom0_cpuid_policy(d);
>  
>      if ( alloc_dom0_vcpu0(d) == NULL )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 8fb3c052f5..0ff1d52e3d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
>      setup_io_bitmap(dom0);
>  #endif
>  
> +    /*
> +     * "dom0" may have been created under the unbounded role, demote it from
> +     * that role, reducing it to the control domain role and any other roles it
> +     * may have been given.
> +     */
> +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
> +    dom0->role |= ROLE_CONTROL_DOMAIN;

I think we need a better definition of the three roles to understand
what this mean.


>      rcu_unlock_domain(dom0);
>  
>      iommu_hwdom_init(d);
> @@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
>      }
>  
>      /* Sort out our idea of is_control_domain(). */
> -    d->is_privileged = flags & CDF_privileged;
> +    if ( flags & CDF_privileged )
> +        d->role |= ROLE_CONTROL_DOMAIN;
> +    else
> +        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
>  
>      /* Sort out our idea of is_hardware_domain(). */
>      if ( is_initial_domain(d) || domid == hardware_domid )
> @@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
>  
>          old_hwdom = hardware_domain;
>          hardware_domain = d;
> +        d->role |= ROLE_HARDWARE_DOMAIN;
>      }
>  
>      TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index a9276a7bed..695f240326 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -467,8 +467,10 @@ struct domain
>  #endif
>      /* is node-affinity automatically computed? */
>      bool             auto_node_affinity;
> -    /* Is this guest fully privileged (aka dom0)? */
> -    bool             is_privileged;
> +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
> +#define ROLE_CONTROL_DOMAIN    (1U<<1)
> +#define ROLE_HARDWARE_DOMAIN   (1U<<2)

This is a great step in the right direction but I think at least a short
in-code comment to explain the difference between the three


> +    uint8_t          role;
>      /* Can this guest access the Xen console? */
>      bool             is_console;
>      /* Is this guest being debugged by dom0? */
> @@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
>  
>  static always_inline bool is_initial_domain(const struct domain *d)
>  {
> -    static int init_domain_id = 0;
> -
> -    return d->domain_id == init_domain_id;
> +    return d->role & ROLE_UNBOUNDED_DOMAIN;
>  }

As far as I can tell this is the only functional change in this patch:
given that dom0 loses unbounded soon after boot, the "is_initial_domain"
checks will start to fail?

We have a few of them in the code and I couldn't rule out that at least
these 3 could happen at runtime:

xen/common/sched/core.c:    else if ( is_initial_domain(d) && opt_dom0_vcpus_pin )
xen/common/sched/core.c:    else if ( is_initial_domain(d) )
xen/common/sched/arinc653.c:    if ( is_initial_domain(unit->domain) )

Maybe they need to be changed to control_domain checks?


>  /*
> @@ -1076,7 +1076,8 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>      if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>          return false;
>  
> -    return evaluate_nospec(d == hardware_domain);
> +    return evaluate_nospec(((d->role & ROLE_HARDWARE_DOMAIN) ||
> +        is_initial_domain(d)) && (d == hardware_domain));
>  }
>  
>  /* This check is for functionality specific to a control domain */
> @@ -1085,7 +1086,8 @@ static always_inline bool is_control_domain(const struct domain *d)
>      if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>          return false;
>  
> -    return evaluate_nospec(d->is_privileged);
> +    return evaluate_nospec((d->role & ROLE_CONTROL_DOMAIN) ||
> +        is_initial_domain(d));
>  }
>  
>  #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 8671af1ba4..18f1ddd127 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -108,7 +108,7 @@ static XSM_INLINE int cf_check xsm_set_system_active(void)
>  {
>      struct domain *d = current->domain;
>  
> -    ASSERT(d->is_privileged);
> +    ASSERT(d->role & ROLE_CONTROL_DOMAIN);
>  
>      if ( d->domain_id != DOMID_IDLE )
>      {
> @@ -116,7 +116,7 @@ static XSM_INLINE int cf_check xsm_set_system_active(void)
>          return -EPERM;
>      }
>  
> -    d->is_privileged = false;
> +    d->role &= ~ROLE_CONTROL_DOMAIN;
>  
>      return 0;
>  }
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 78225f68c1..0a31719f43 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -193,7 +193,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
>      default:
>          if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
>          {
> -            if ( d->is_privileged )
> +            if ( d->role & ROLE_CONTROL_DOMAIN )
>                  dsec->sid = SECINITSID_DOM0;
>              else if ( pv_shim )
>                  dsec->sid = SECINITSID_DOMU;
> @@ -213,7 +213,7 @@ static int cf_check flask_set_system_active(void)
>  
>      dsec = d->ssid;
>  
> -    ASSERT(d->is_privileged);
> +    ASSERT(d->role & ROLE_CONTROL_DOMAIN);
>      ASSERT(dsec->sid == SECINITSID_XENBOOT);
>      ASSERT(dsec->self_sid == SECINITSID_XENBOOT);
>  
> @@ -224,11 +224,11 @@ static int cf_check flask_set_system_active(void)
>      }
>  
>      /*
> -     * While is_privileged has no significant meaning under flask, set to false
> -     * as is_privileged is not only used for a privilege check but also as a
> -     * type of domain check, specifically if the domain is the control domain.
> +     * While domain roles have no significant meaning under flask, mask out
> +     * control domain role as it is not only used for a privilege check but
> +     * also as a type of domain check.
>       */
> -    d->is_privileged = false;
> +    d->role &= ~ROLE_CONTROL_DOMAIN;
>  
>      dsec->self_sid = dsec->sid = SECINITSID_XEN;
>  
> -- 
> 2.20.1
> 

Re: [RFC 3/6] roles: add a role for xenstore domain

[Stefano Stabellini]

On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> Expand the possible roles for a domain to include a role for the Xenstore
> domain.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/common/domain.c     | 3 +++
>  xen/include/xen/sched.h | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 0ff1d52e3d..dbf055c559 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -633,6 +633,9 @@ struct domain *domain_create(domid_t domid,
>          d->role |= ROLE_HARDWARE_DOMAIN;
>      }
>  
> +    if ( d->options & XEN_DOMCTL_CDF_xs_domain )
> +        d->role |= ROLE_XENSTORE_DOMAIN;
> +
>      TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>  
>      lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 695f240326..ec0f9baff6 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -470,6 +470,7 @@ struct domain
>  #define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
>  #define ROLE_CONTROL_DOMAIN    (1U<<1)
>  #define ROLE_HARDWARE_DOMAIN   (1U<<2)
> +#define ROLE_XENSTORE_DOMAIN   (1U<<3)
>      uint8_t          role;
>      /* Can this guest access the Xen console? */
>      bool             is_console;
> @@ -1165,7 +1166,7 @@ static inline bool is_vcpu_online(const struct vcpu *v)
>  
>  static inline bool is_xenstore_domain(const struct domain *d)
>  {
> -    return d->options & XEN_DOMCTL_CDF_xs_domain;
> +    return d->role & ROLE_XENSTORE_DOMAIN;
>  }
>  
>  static always_inline bool is_iommu_enabled(const struct domain *d)
> -- 
> 2.20.1
> 

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Stefano Stabellini]

On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> The field `is_console` suggests that the field represents a state of being or
> posession, not that it reflects the privilege to access the console. In this
  ^ possession

> patch the field is renamed to capabilities to encapsulate the capabilities a
> domain has been granted. The first capability being the ability to read/write
> the Xen console.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Patch looks fine to me aside the two minor nits. I am not sure I
understand 100% the difference between capabilities and roles but I am
OK with the patch. I'd like to hear Julien's feedback on this as well. 


> ---
>  xen/arch/arm/domain_build.c |  4 +++-
>  xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
>  xen/include/xsm/dummy.h     |  2 +-
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 51b4daefe1..ad7432b029 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -4076,7 +4076,9 @@ void __init create_domUs(void)
>              panic("Error creating domain %s (rc = %ld)\n",
>                    dt_node_name(node), PTR_ERR(d));
>  
> -        d->is_console = true;
> +        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )

code style


> +            printk("failed setting console_io on %pd\n", d);
> +
>          dt_device_set_used_by(node, d->domain_id);
>  
>          rc = construct_domU(d, node);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ec0f9baff6..b04fbe0565 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -472,8 +472,8 @@ struct domain
>  #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>  #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>      uint8_t          role;
> -    /* Can this guest access the Xen console? */
> -    bool             is_console;
> +#define CAP_CONSOLE_IO  (1U<<0)
> +    uint8_t          capabilities;
>      /* Is this guest being debugged by dom0? */
>      bool             debugger_attached;
>      /*
> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu *v)
>      return is_hvm_domain(v->domain);
>  }
>  
> +static always_inline bool domain_has_cap(
> +    const struct domain *d, uint8_t cap)
> +{
> +    return d->capabilities & cap;
> +}
> +
> +static always_inline bool domain_set_cap(
> +    struct domain *d, uint8_t cap)
> +{
> +    switch ( cap )
> +    {
> +    case CAP_CONSOLE_IO:
> +        d->capabilities |= cap;
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    return domain_has_cap(d, cap);
> +}
> +
>  static always_inline bool hap_enabled(const struct domain *d)
>  {
>      /* sanitise_domain_config() rejects HAP && !HVM */
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 18f1ddd127..067ff1d111 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -268,7 +268,7 @@ static XSM_INLINE int cf_check xsm_console_io(
>      XSM_DEFAULT_ARG struct domain *d, int cmd)
>  {
>      XSM_ASSERT_ACTION(XSM_OTHER);
> -    if ( d->is_console )
> +    if ( domain_has_cap(d, CAP_CONSOLE_IO) )
>          return xsm_default_action(XSM_HOOK, d, NULL);
>  #ifdef CONFIG_VERBOSE_DEBUG
>      if ( cmd == CONSOLEIO_write )
> -- 
> 2.20.1
> 

Re: [RFC 6/6] capabilities: convert attach debugger into a capability

[Stefano Stabellini]

On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> Expresses the ability to attach a debugger as a capability that a domain can be
> provisioned.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c      |  8 ++++----
>  xen/arch/x86/hvm/vmx/realmode.c |  2 +-
>  xen/arch/x86/hvm/vmx/vmcs.c     |  2 +-
>  xen/arch/x86/hvm/vmx/vmx.c      | 10 +++++-----
>  xen/arch/x86/traps.c            |  6 ++++--
>  xen/common/domctl.c             |  6 ++++--
>  xen/include/xen/sched.h         |  9 ++++++---
>  7 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 27170213ae..9872804d39 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -999,7 +999,7 @@ static void noreturn cf_check svm_do_resume(void)
>  {
>      struct vcpu *v = current;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> -    bool debug_state = (v->domain->debugger_attached ||
> +    bool debug_state = (domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) ||
>                          v->domain->arch.monitor.software_breakpoint_enabled ||
>                          v->domain->arch.monitor.debug_exception_enabled);
>      bool_t vcpu_guestmode = 0;
> @@ -1335,7 +1335,7 @@ static void cf_check svm_inject_event(const struct x86_event *event)
>          }
>          /* fall through */
>      case X86_EXC_BP:
> -        if ( curr->domain->debugger_attached )
> +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>          {
>              /* Debug/Int3: Trap to debugger. */
>              domain_pause_for_debugger();
> @@ -2732,7 +2732,7 @@ void svm_vmexit_handler(void)
>  
>      case VMEXIT_ICEBP:
>      case VMEXIT_EXCEPTION_DB:
> -        if ( !v->domain->debugger_attached )
> +        if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>          {
>              unsigned int trap_type;
>  
> @@ -2769,7 +2769,7 @@ void svm_vmexit_handler(void)
>          if ( insn_len == 0 )
>               break;
>  
> -        if ( v->domain->debugger_attached )
> +        if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>          {
>              /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
>              __update_guest_eip(regs, insn_len);
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
> index ff44ddcfa6..f761026a9d 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -121,7 +121,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
>  
>      if ( rc == X86EMUL_EXCEPTION )
>      {
> -        if ( unlikely(curr->domain->debugger_attached) &&
> +        if ( unlikely(domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH)) &&
>               ((hvmemul_ctxt->ctxt.event.vector == X86_EXC_DB) ||
>                (hvmemul_ctxt->ctxt.event.vector == X86_EXC_BP)) )
>          {
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 13719cc923..9474869018 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1912,7 +1912,7 @@ void cf_check vmx_do_resume(void)
>          hvm_asid_flush_vcpu(v);
>      }
>  
> -    debug_state = v->domain->debugger_attached
> +    debug_state = domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH)
>                    || v->domain->arch.monitor.software_breakpoint_enabled
>                    || v->domain->arch.monitor.singlestep_enabled;
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 7ec44018d4..5069e3cbf3 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2041,7 +2041,7 @@ static void cf_check vmx_inject_event(const struct x86_event *event)
>              break;
>          /* fall through */
>      case X86_EXC_BP:
> -        if ( curr->domain->debugger_attached )
> +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>          {
>              /* Debug/Int3: Trap to debugger. */
>              domain_pause_for_debugger();
> @@ -2121,7 +2121,7 @@ static void cf_check vmx_set_info_guest(struct vcpu *v)
>       * immediately vmexit and hence make no progress.
>       */
>      __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
> -    if ( v->domain->debugger_attached &&
> +    if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) &&
>           (v->arch.user_regs.eflags & X86_EFLAGS_TF) &&
>           (intr_shadow & VMX_INTR_SHADOW_STI) )
>      {
> @@ -4283,7 +4283,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                  }
>              }
>  
> -            if ( !v->domain->debugger_attached )
> +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>              {
>                  unsigned long insn_len = 0;
>                  int rc;
> @@ -4307,7 +4307,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              break;
>          case X86_EXC_BP:
>              HVMTRACE_1D(TRAP, vector);
> -            if ( !v->domain->debugger_attached )
> +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>              {
>                  unsigned long insn_len;
>                  int rc;
> @@ -4647,7 +4647,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                                HVM_MONITOR_SINGLESTEP_BREAKPOINT,
>                                0, 0, 0);
>  
> -            if ( v->domain->debugger_attached )
> +            if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>                  domain_pause_for_debugger();
>          }
>  
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 4229bda159..041ced35ea 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1214,7 +1214,8 @@ void do_int3(struct cpu_user_regs *regs)
>          return;
>      }
>  
> -    if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
> +    if ( guest_kernel_mode(curr, regs) &&
> +         domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>      {
>          curr->arch.gdbsx_vcpu_event = X86_EXC_BP;
>          domain_pause_for_debugger();
> @@ -1995,7 +1996,8 @@ void do_debug(struct cpu_user_regs *regs)
>      v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>      v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
>  
> -    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
> +    if ( guest_kernel_mode(v, regs) &&
> +         domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>      {
>          domain_pause_for_debugger();
>          return;
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 505e29c0dc..895ddf0600 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -99,7 +99,8 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>          ((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) |
> +        (domain_has_cap(d, CAP_DEBUGGER_ATTACH) ?
> +                                          XEN_DOMINF_debugged  : 0) |
>          (is_xenstore_domain(d)          ? XEN_DOMINF_xs_domain : 0) |
>          (is_hvm_domain(d)               ? XEN_DOMINF_hvm_guest : 0) |
>          d->shutdown_code << XEN_DOMINF_shutdownshift;
> @@ -643,7 +644,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          else
>          {
>              domain_pause(d);
> -            d->debugger_attached = !!op->u.setdebugging.enable;
> +            if ( !!op->u.setdebugging.enable )
> +                domain_set_cap(d, CAP_DEBUGGER_ATTACH);
>              domain_unpause(d); /* causes guest to latch new status */
>          }
>          break;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ebfe65cd73..47eadb5008 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -474,9 +474,8 @@ struct domain
>      uint8_t          role;
>  #define CAP_CONSOLE_IO         (1U<<0)
>  #define CAP_DISABLE_CPU_FAULT  (1U<<1)
> -    uint8_t          capabilities;
> -    /* Is this guest being debugged by dom0? */
> -    bool             debugger_attached;
> +#define CAP_DEBUGGER_ATTACH    (1U<<2)
> +    uint16_t         capabilities;

No need to switch to uint16_t just yet?


>      /*
>       * Set to true at the very end of domain creation, when the domain is
>       * unpaused for the first time by the systemcontroller.
> @@ -1166,6 +1165,10 @@ static always_inline bool domain_set_cap(
>          if ( is_pv_domain(d) && is_control_domain(d) )
>              d->capabilities |= cap;
>          break;
> +    case CAP_DEBUGGER_ATTACH:
> +        if ( !is_control_domain(d) )
> +            d->capabilities |= cap;

I take it is not possible to attach it to dom0? I am not familiar with
XEN_DOMCTL_setdebugging but the hypercall doesn't seem to have such
restriction.


> +        break;
>      default:
>          return false;
>      }
> -- 
> 2.20.1
> 

Re: [RFC 1/6] dom0: replace explict zero checks

[Jan Beulich]

On 02.08.2023 02:24, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>>  void watchdog_domain_init(struct domain *d);
>>  void watchdog_domain_destroy(struct domain *d);
>>  
>> +static always_inline bool is_initial_domain(const struct domain *d)
> 
> I know you used always_inline only because is_hardware_domain just below
> also uses it, but I wonder if we need it.
> 
> Also, Robero, it looks like always_inline is missing from
> docs/misra/C-language-toolchain.rst? Or do we plan to get rid of it?

Under "Non-standard tokens" we have both __inline__ and __attribute__
listed, which I think is enough to cover this specific case as well?

Jan

Re: [RFC 1/6] dom0: replace explict zero checks

[Jan Beulich]

On 01.08.2023 22:20, Daniel P. Smith wrote:
> A legacy concept is that the initial domain will have a domain id of zero. As a
> result there are places where a check that a domain is the inital domain is
> determined by an explicit check that the domid is zero.

It might help if you at least outlined here why/how this is going to
change.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>  void watchdog_domain_init(struct domain *d);
>  void watchdog_domain_destroy(struct domain *d);
>  
> +static always_inline bool is_initial_domain(const struct domain *d)
> +{
> +    static int init_domain_id = 0;

This may then also help with the question on why you use a static
variable here. (In any event the type of this variable wants to
be correct; plain int isn't appropriate ...

> +    return d->domain_id == init_domain_id;

... for this comparison.)

Jan

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Jan Beulich]

On 01.08.2023 22:20, Daniel P. Smith wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
>      setup_io_bitmap(dom0);
>  #endif
>  
> +    /*
> +     * "dom0" may have been created under the unbounded role, demote it from
> +     * that role, reducing it to the control domain role and any other roles it
> +     * may have been given.
> +     */
> +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);

This doesn't look to remove anything, when taking into account ...

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -467,8 +467,10 @@ struct domain
>  #endif
>      /* is node-affinity automatically computed? */
>      bool             auto_node_affinity;
> -    /* Is this guest fully privileged (aka dom0)? */
> -    bool             is_privileged;
> +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
> +#define ROLE_CONTROL_DOMAIN    (1U<<1)
> +#define ROLE_HARDWARE_DOMAIN   (1U<<2)

... that each of the constants has just a single bit set. Seeing the
& there I was expecting something like

#define ROLE_UNBOUNDED_DOMAIN  (ROLE_CONTROL_DOMAIN | ROLE_HARDWARE_DOMAIN)

instead.

Jan

Re: [RFC 1/6] dom0: replace explict zero checks

[Stefano Stabellini]

On Wed, 2 Aug 2023, Jan Beulich wrote:
> On 02.08.2023 02:24, Stefano Stabellini wrote:
> > On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> >> --- a/xen/include/xen/sched.h
> >> +++ b/xen/include/xen/sched.h
> >> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
> >>  void watchdog_domain_init(struct domain *d);
> >>  void watchdog_domain_destroy(struct domain *d);
> >>  
> >> +static always_inline bool is_initial_domain(const struct domain *d)
> > 
> > I know you used always_inline only because is_hardware_domain just below
> > also uses it, but I wonder if we need it.
> > 
> > Also, Robero, it looks like always_inline is missing from
> > docs/misra/C-language-toolchain.rst? Or do we plan to get rid of it?
> 
> Under "Non-standard tokens" we have both __inline__ and __attribute__
> listed, which I think is enough to cover this specific case as well?

I think we should add always_inline explicitely to
docs/misra/C-language-toolchain.rst to avoid any doubts about it

Re: [RFC 1/6] dom0: replace explict zero checks

[Daniel P. Smith]

On 8/1/23 20:24, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>> A legacy concept is that the initial domain will have a domain id of zero. As a
>> result there are places where a check that a domain is the inital domain is
>> determined by an explicit check that the domid is zero.
>>
>> This commit seeks to abstract this check into a function call and replace all
>> check locations with the function call.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Thanks for the patch, this is a good cleanup!

Thank you for the sentiment as well as giving it a review!

>> ---
>>   xen/common/domain.c         | 4 ++--
>>   xen/common/sched/arinc653.c | 2 +-
>>   xen/common/sched/core.c     | 4 ++--
>>   xen/include/xen/sched.h     | 7 +++++++
>>   4 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 304aa04fa6..8fb3c052f5 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -309,7 +309,7 @@ static int late_hwdom_init(struct domain *d)
>>       struct domain *dom0;
>>       int rv;
>>   
>> -    if ( d != hardware_domain || d->domain_id == 0 )
>> +    if ( d != hardware_domain || is_initial_domain(d) )
>>           return 0;
>>   
>>       rv = xsm_init_hardware_domain(XSM_HOOK, d);
>> @@ -612,7 +612,7 @@ struct domain *domain_create(domid_t domid,
>>       d->is_privileged = flags & CDF_privileged;
>>   
>>       /* Sort out our idea of is_hardware_domain(). */
>> -    if ( domid == 0 || domid == hardware_domid )
>> +    if ( is_initial_domain(d) || domid == hardware_domid )
>>       {
>>           if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
>>               panic("The value of hardware_dom must be a valid domain ID\n");
>> diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
>> index a82c0d7314..31e8270af3 100644
>> --- a/xen/common/sched/arinc653.c
>> +++ b/xen/common/sched/arinc653.c
>> @@ -404,7 +404,7 @@ a653sched_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
>>        * Add every one of dom0's units to the schedule, as long as there are
>>        * slots available.
>>        */
>> -    if ( unit->domain->domain_id == 0 )
>> +    if ( is_initial_domain(unit->domain) )
>>       {
>>           entry = sched_priv->num_schedule_entries;
>>   
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 022f548652..210ad30f94 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -585,7 +585,7 @@ int sched_init_vcpu(struct vcpu *v)
>>            */
>>           sched_set_affinity(unit, cpumask_of(0), cpumask_of(0));
>>       }
>> -    else if ( d->domain_id == 0 && opt_dom0_vcpus_pin )
>> +    else if ( is_initial_domain(d) && opt_dom0_vcpus_pin )
>>       {
>>           /*
>>            * If dom0_vcpus_pin is specified, dom0 vCPUs are pinned 1:1 to
>> @@ -594,7 +594,7 @@ int sched_init_vcpu(struct vcpu *v)
>>           sched_set_affinity(unit, cpumask_of(processor), &cpumask_all);
>>       }
>>   #ifdef CONFIG_X86
>> -    else if ( d->domain_id == 0 )
>> +    else if ( is_initial_domain(d) )
>>       {
>>           /*
>>            * In absence of dom0_vcpus_pin instead, the hard and soft affinity of
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 854f3e32c0..a9276a7bed 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>>   void watchdog_domain_init(struct domain *d);
>>   void watchdog_domain_destroy(struct domain *d);
>>   
>> +static always_inline bool is_initial_domain(const struct domain *d)
> 
> I know you used always_inline only because is_hardware_domain just below
> also uses it, but I wonder if we need it.

I was under the impression that access checks like these could end up in 
fast paths, so we wanted to coax the compiler into inlining these 
whenever possible. If others feel it is not needed, I have no objection 
to moving it to just inline.

> Also, Robero, it looks like always_inline is missing from
> docs/misra/C-language-toolchain.rst? Or do we plan to get rid of it?
> 
> 
>> +{
>> +    static int init_domain_id = 0;
> 
> I take it is done this way because you plan to make it configurable?

As you see in a later patch, it gets changed into an assignable role for 
the domain.

>> +    return d->domain_id == init_domain_id;
>> +}
>> +
>>   /*
>>    * Use this check when the following are both true:
>>    *  - Using this feature or interface requires full access to the hardware
>> -- 
>> 2.20.1
>>

Re: [RFC 1/6] dom0: replace explict zero checks

[Daniel P. Smith]

On 8/2/23 03:46, Jan Beulich wrote:
> On 01.08.2023 22:20, Daniel P. Smith wrote:
>> A legacy concept is that the initial domain will have a domain id of zero. As a
>> result there are places where a check that a domain is the inital domain is
>> determined by an explicit check that the domid is zero.
> 
> It might help if you at least outlined here why/how this is going to
> change.

Okay, I will try expanding on this further.

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>>   void watchdog_domain_init(struct domain *d);
>>   void watchdog_domain_destroy(struct domain *d);
>>   
>> +static always_inline bool is_initial_domain(const struct domain *d)
>> +{
>> +    static int init_domain_id = 0;
> 
> This may then also help with the question on why you use a static
> variable here. (In any event the type of this variable wants to
> be correct; plain int isn't appropriate ...

Ah, so this is a dated patch that I brought because of the abstraction 
it made. The intent in the original series for making it static was in 
preparation to handle the shim case where init_domid() would have return 
a non-zero value.

So the static can be dropped and changed to domid_t.

>> +    return d->domain_id == init_domain_id;
> 
> ... for this comparison.)
> 
> Jan

v/r,
dps

Re: [RFC 1/6] dom0: replace explict zero checks

[Julien Grall]

Hi Daniel,

On 03/08/2023 14:33, Daniel P. Smith wrote:
> On 8/2/23 03:46, Jan Beulich wrote:
>> On 01.08.2023 22:20, Daniel P. Smith wrote:
>>> A legacy concept is that the initial domain will have a domain id of 
>>> zero. As a
>>> result there are places where a check that a domain is the inital 
>>> domain is
>>> determined by an explicit check that the domid is zero.
>>
>> It might help if you at least outlined here why/how this is going to
>> change.
> 
> Okay, I will try expanding on this further.
> 
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>>>   void watchdog_domain_init(struct domain *d);
>>>   void watchdog_domain_destroy(struct domain *d);
>>> +static always_inline bool is_initial_domain(const struct domain *d)
>>> +{
>>> +    static int init_domain_id = 0;
>>
>> This may then also help with the question on why you use a static
>> variable here. (In any event the type of this variable wants to
>> be correct; plain int isn't appropriate ...
> 
> Ah, so this is a dated patch that I brought because of the abstraction 
> it made. The intent in the original series for making it static was in 
> preparation to handle the shim case where init_domid() would have return 
> a non-zero value.
> 
> So the static can be dropped and changed to domid_t.

Looking at one of the follow-up patch, I see:

  static always_inline bool is_initial_domain(const struct domain *d)
  {
-    static int init_domain_id = 0;
-
-    return d->domain_id == init_domain_id;
+    return d->role & ROLE_UNBOUNDED_DOMAIN;
  }

So is there any point to have the local variable? IOW, can't this simply 
be "d->domain_id == 0"?

Cheers,

-- 
Julien Grall

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Daniel P. Smith]

On 8/1/23 20:54, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>> The existing concepts such as unbounded domain, ie. all powerful, control
>> domain and hardware domain are, effectively, roles the domains provide for the
>> system. Currently, these are represented with booleans within `struct domain`
>> or global domid variables that are compared against. This patch begins to
>> formalize these roles by replacing the `is_control` and `is_console`, along
>> with expanding the check against the global `hardware_domain` with a single
>> encapsulating role attribute in `struct domain`.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> This is definitely heading in the right direction

Thank you, it is good to know there is some agreement here.

>> ---
>>   xen/arch/arm/domain_build.c |  2 ++
>>   xen/arch/x86/setup.c        |  2 ++
>>   xen/common/domain.c         | 14 +++++++++++++-
>>   xen/include/xen/sched.h     | 16 +++++++++-------
>>   xen/include/xsm/dummy.h     |  4 ++--
>>   xen/xsm/flask/hooks.c       | 12 ++++++------
>>   6 files changed, 34 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 39b4ee03a5..51b4daefe1 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -4201,6 +4201,8 @@ void __init create_dom0(void)
>>       if ( IS_ERR(dom0) )
>>           panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
>>   
>> +    dom0->role |= ROLE_UNBOUNDED_DOMAIN;
> 
> I am not a fan of "UNBOUNDED". Maybe "PRIMARY"? "PRIVILEGED"? "SUPER"?
> "ROOT"?
> 
> I also recognize I am not good at naming things so I'll stop here and
> let other provide better feedback :-)

In first version of hyperlaunch and in the early roles work, I was 
working to move toward eliminating this concept entirely. The reality is 
this is a model that has existed for over 20 years and there are those 
who accept and embrace the model. Introducing the name UNBOUNDED was to 
at least break the idea that the all powerful domain necessarily is the 
first/initial domain to run. With hyperlaunch, there are still 
security-based scenarios where you might want to run a DomB before 
starting an all privileged domain. I spent quite some time, probably 
more than I should have, to find a good name that expresses what this 
role is. Considering a comment below and a comment by Jan, I am starting 
to think a better way to view it is a domain that assumes all roles in 
the system. So your suggestions of SUPER or ROOT might be more fitting. 
I considered ROLE_ALL, but something about it doesn't sit right with me. 
With that said I welcome the yak shaving of naming to begin. ( ^_^)

> Also, do we actually need unbounded given that it gets replaced with
> control_domain pretty soon?

Yes, because as mentioned above, this is meant to express a domain that 
has been assigned all roles, for which later the domain may decided to 
delegate the role to another domain.

> I am asking because I think that at least from a safety perspective it
> would be a problem to run a domain as "unbounded". In a safety system,
> we wouldn't want anything to be unbounded, not even temporarily at boot.
> If "unbounded" is removed before running dom0, then of course it is no
> problem because actually dom0 never gets to run with "unbounded" set.

I think this is were the name UNBOUNDED may have been a bad choice. The 
UNBOUNDED role is dom0. It is the control domain, the hardware domain, 
the Xenstore domain, and the crash domain (if that were to be solidified).

> (I am currently thinking of solving the privilege issue by using XSM and
> removing most privileges from Dom0.)

I obviously would be a huge advocate of that approach. ( ^_^)

>>       if ( alloc_dom0_vcpu0(dom0) == NULL )
>>           panic("Error creating domain 0 vcpu0\n");
>>   
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 2dbe9857aa..4e20edc3bf 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const module_t *image,
>>       if ( IS_ERR(d) )
>>           panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
>>   
>> +    d->role |= ROLE_UNBOUNDED_DOMAIN;
>> +
>>       init_dom0_cpuid_policy(d);
>>   
>>       if ( alloc_dom0_vcpu0(d) == NULL )
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 8fb3c052f5..0ff1d52e3d 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
>>       setup_io_bitmap(dom0);
>>   #endif
>>   
>> +    /*
>> +     * "dom0" may have been created under the unbounded role, demote it from
>> +     * that role, reducing it to the control domain role and any other roles it
>> +     * may have been given.
>> +     */
>> +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
>> +    dom0->role |= ROLE_CONTROL_DOMAIN;
> 
> I think we need a better definition of the three roles to understand
> what this mean.

Definition and as highlighted, a better name.

>>       rcu_unlock_domain(dom0);
>>   
>>       iommu_hwdom_init(d);
>> @@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
>>       }
>>   
>>       /* Sort out our idea of is_control_domain(). */
>> -    d->is_privileged = flags & CDF_privileged;
>> +    if ( flags & CDF_privileged )
>> +        d->role |= ROLE_CONTROL_DOMAIN;
>> +    else
>> +        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
>>   
>>       /* Sort out our idea of is_hardware_domain(). */
>>       if ( is_initial_domain(d) || domid == hardware_domid )
>> @@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
>>   
>>           old_hwdom = hardware_domain;
>>           hardware_domain = d;
>> +        d->role |= ROLE_HARDWARE_DOMAIN;
>>       }
>>   
>>       TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index a9276a7bed..695f240326 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -467,8 +467,10 @@ struct domain
>>   #endif
>>       /* is node-affinity automatically computed? */
>>       bool             auto_node_affinity;
>> -    /* Is this guest fully privileged (aka dom0)? */
>> -    bool             is_privileged;
>> +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
>> +#define ROLE_CONTROL_DOMAIN    (1U<<1)
>> +#define ROLE_HARDWARE_DOMAIN   (1U<<2)
> 
> This is a great step in the right direction but I think at least a short
> in-code comment to explain the difference between the three

Ack.

>> +    uint8_t          role;
>>       /* Can this guest access the Xen console? */
>>       bool             is_console;
>>       /* Is this guest being debugged by dom0? */
>> @@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
>>   
>>   static always_inline bool is_initial_domain(const struct domain *d)
>>   {
>> -    static int init_domain_id = 0;
>> -
>> -    return d->domain_id == init_domain_id;
>> +    return d->role & ROLE_UNBOUNDED_DOMAIN;
>>   }
> 
> As far as I can tell this is the only functional change in this patch:
> given that dom0 loses unbounded soon after boot, the "is_initial_domain"
> checks will start to fail?

Today, dom0 should not lose any of its roles at boot unless dom0less 
were to create a hardware domain.

Upon reflection, I am thinking this check might want renaming to align 
with the rename of this role.

> We have a few of them in the code and I couldn't rule out that at least
> these 3 could happen at runtime:
> 
> xen/common/sched/core.c:    else if ( is_initial_domain(d) && opt_dom0_vcpus_pin )
> xen/common/sched/core.c:    else if ( is_initial_domain(d) )
> xen/common/sched/arinc653.c:    if ( is_initial_domain(unit->domain) )
> 
> Maybe they need to be changed to control_domain checks?

Perhaps, I would want to study them a bit before switching them over.

>>   /*
>> @@ -1076,7 +1076,8 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>>       if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>           return false;
>>   
>> -    return evaluate_nospec(d == hardware_domain);
>> +    return evaluate_nospec(((d->role & ROLE_HARDWARE_DOMAIN) ||
>> +        is_initial_domain(d)) && (d == hardware_domain));
>>   }
>>   
>>   /* This check is for functionality specific to a control domain */
>> @@ -1085,7 +1086,8 @@ static always_inline bool is_control_domain(const struct domain *d)
>>       if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>           return false;
>>   
>> -    return evaluate_nospec(d->is_privileged);
>> +    return evaluate_nospec((d->role & ROLE_CONTROL_DOMAIN) ||
>> +        is_initial_domain(d));
>>   }
>>   
>>   #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>> index 8671af1ba4..18f1ddd127 100644
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -108,7 +108,7 @@ static XSM_INLINE int cf_check xsm_set_system_active(void)
>>   {
>>       struct domain *d = current->domain;
>>   
>> -    ASSERT(d->is_privileged);
>> +    ASSERT(d->role & ROLE_CONTROL_DOMAIN);
>>   
>>       if ( d->domain_id != DOMID_IDLE )
>>       {
>> @@ -116,7 +116,7 @@ static XSM_INLINE int cf_check xsm_set_system_active(void)
>>           return -EPERM;
>>       }
>>   
>> -    d->is_privileged = false;
>> +    d->role &= ~ROLE_CONTROL_DOMAIN;
>>   
>>       return 0;
>>   }
>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index 78225f68c1..0a31719f43 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -193,7 +193,7 @@ static int cf_check flask_domain_alloc_security(struct domain *d)
>>       default:
>>           if ( domain_sid(current->domain) == SECINITSID_XENBOOT )
>>           {
>> -            if ( d->is_privileged )
>> +            if ( d->role & ROLE_CONTROL_DOMAIN )
>>                   dsec->sid = SECINITSID_DOM0;
>>               else if ( pv_shim )
>>                   dsec->sid = SECINITSID_DOMU;
>> @@ -213,7 +213,7 @@ static int cf_check flask_set_system_active(void)
>>   
>>       dsec = d->ssid;
>>   
>> -    ASSERT(d->is_privileged);
>> +    ASSERT(d->role & ROLE_CONTROL_DOMAIN);
>>       ASSERT(dsec->sid == SECINITSID_XENBOOT);
>>       ASSERT(dsec->self_sid == SECINITSID_XENBOOT);
>>   
>> @@ -224,11 +224,11 @@ static int cf_check flask_set_system_active(void)
>>       }
>>   
>>       /*
>> -     * While is_privileged has no significant meaning under flask, set to false
>> -     * as is_privileged is not only used for a privilege check but also as a
>> -     * type of domain check, specifically if the domain is the control domain.
>> +     * While domain roles have no significant meaning under flask, mask out
>> +     * control domain role as it is not only used for a privilege check but
>> +     * also as a type of domain check.
>>        */
>> -    d->is_privileged = false;
>> +    d->role &= ~ROLE_CONTROL_DOMAIN;
>>   
>>       dsec->self_sid = dsec->sid = SECINITSID_XEN;
>>   
>> -- 
>> 2.20.1
>>

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Daniel P. Smith]

On 8/2/23 03:51, Jan Beulich wrote:
> On 01.08.2023 22:20, Daniel P. Smith wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
>>       setup_io_bitmap(dom0);
>>   #endif
>>   
>> +    /*
>> +     * "dom0" may have been created under the unbounded role, demote it from
>> +     * that role, reducing it to the control domain role and any other roles it
>> +     * may have been given.
>> +     */
>> +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
> 
> This doesn't look to remove anything, when taking into account ...

Ugh, you are correct. It was meant to be a bitwise and of dom0-role with 
a mask that has every bit set except ROLE_UNBOUNDED_DOMAIN and 
ROLE_HARDWARE_DOMAIN. But being a bonehead, I bitwise and the two roles 
instead of or-ing them. I agree with your comment below, which will 
reduce to just masking a bitwise not of ROLE_HARDWARE_DOMAIN.

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -467,8 +467,10 @@ struct domain
>>   #endif
>>       /* is node-affinity automatically computed? */
>>       bool             auto_node_affinity;
>> -    /* Is this guest fully privileged (aka dom0)? */
>> -    bool             is_privileged;
>> +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
>> +#define ROLE_CONTROL_DOMAIN    (1U<<1)
>> +#define ROLE_HARDWARE_DOMAIN   (1U<<2)
> 
> ... that each of the constants has just a single bit set. Seeing the
> & there I was expecting something like
> 
> #define ROLE_UNBOUNDED_DOMAIN  (ROLE_CONTROL_DOMAIN | ROLE_HARDWARE_DOMAIN)
> 
> instead.

Agree, instead of consuming one the limited number of bits for a role 
that represents a domain having all roles, just or all the roles 
together. Then I can reclaim one of the bits of the flag field.

v/r,
dps

Re: [RFC 3/6] roles: add a role for xenstore domain

[Daniel P. Smith]

On 8/1/23 20:57, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>> Expand the possible roles for a domain to include a role for the Xenstore
>> domain.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you!

>> ---
>>   xen/common/domain.c     | 3 +++
>>   xen/include/xen/sched.h | 3 ++-
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 0ff1d52e3d..dbf055c559 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -633,6 +633,9 @@ struct domain *domain_create(domid_t domid,
>>           d->role |= ROLE_HARDWARE_DOMAIN;
>>       }
>>   
>> +    if ( d->options & XEN_DOMCTL_CDF_xs_domain )
>> +        d->role |= ROLE_XENSTORE_DOMAIN;
>> +
>>       TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>>   
>>       lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 695f240326..ec0f9baff6 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -470,6 +470,7 @@ struct domain
>>   #define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
>>   #define ROLE_CONTROL_DOMAIN    (1U<<1)
>>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>> +#define ROLE_XENSTORE_DOMAIN   (1U<<3)
>>       uint8_t          role;
>>       /* Can this guest access the Xen console? */
>>       bool             is_console;
>> @@ -1165,7 +1166,7 @@ static inline bool is_vcpu_online(const struct vcpu *v)
>>   
>>   static inline bool is_xenstore_domain(const struct domain *d)
>>   {
>> -    return d->options & XEN_DOMCTL_CDF_xs_domain;
>> +    return d->role & ROLE_XENSTORE_DOMAIN;
>>   }
>>   
>>   static always_inline bool is_iommu_enabled(const struct domain *d)
>> -- 
>> 2.20.1
>>

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Daniel P. Smith]

On 8/1/23 21:01, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>> The field `is_console` suggests that the field represents a state of being or
>> posession, not that it reflects the privilege to access the console. In this
>    ^ possession

Thank you for the catch.

>> patch the field is renamed to capabilities to encapsulate the capabilities a
>> domain has been granted. The first capability being the ability to read/write
>> the Xen console.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Patch looks fine to me aside the two minor nits. I am not sure I
> understand 100% the difference between capabilities and roles but I am
> OK with the patch. I'd like to hear Julien's feedback on this as well.

This might be worth a section in the hypervisor-guide. As mentioned in 
the cover letter, this was originally proposed as being under XSM. A 
challenge I ran into is that, depending on your view, the 
`is_privileged` field and `hardware_domain` global were either abused as 
a function check and a non-resource privilege check or are just 
multifaceted variables. This is why the concept of the role was struck 
upon, it is more intuitive (for me at least) that have a role is 
something that imparts accesses (privilege checks) and dictates 
hypervisor behaviors when handling the domain (function checks). This 
then brings us to an access or behavior that may be inherent to some 
role(s) but may want to grant on an individually to a guest. A prime 
example of this is console_io, for which it is inherent that the 
hardware domain role will have access but may want to grant to a guest 
without granting it an entire role. This is why I provided for 
identifying these capabilities so that they may be assigned individually 
to a domain.

While the role/capability is a natural progression from how the 
hypervisor currently operates. Another approach that could be consider 
to deliver a similar experience would be to break down every access and 
function into a capability and then define the standard roles as a 
conglomeration of certain capabilities.

I am open to suggestions here.

>> ---
>>   xen/arch/arm/domain_build.c |  4 +++-
>>   xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
>>   xen/include/xsm/dummy.h     |  2 +-
>>   3 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 51b4daefe1..ad7432b029 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -4076,7 +4076,9 @@ void __init create_domUs(void)
>>               panic("Error creating domain %s (rc = %ld)\n",
>>                     dt_node_name(node), PTR_ERR(d));
>>   
>> -        d->is_console = true;
>> +        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )
> 
> code style
> 
> 
>> +            printk("failed setting console_io on %pd\n", d);
>> +
>>           dt_device_set_used_by(node, d->domain_id);
>>   
>>           rc = construct_domU(d, node);
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index ec0f9baff6..b04fbe0565 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -472,8 +472,8 @@ struct domain
>>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>>       uint8_t          role;
>> -    /* Can this guest access the Xen console? */
>> -    bool             is_console;
>> +#define CAP_CONSOLE_IO  (1U<<0)
>> +    uint8_t          capabilities;
>>       /* Is this guest being debugged by dom0? */
>>       bool             debugger_attached;
>>       /*
>> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu *v)
>>       return is_hvm_domain(v->domain);
>>   }
>>   
>> +static always_inline bool domain_has_cap(
>> +    const struct domain *d, uint8_t cap)
>> +{
>> +    return d->capabilities & cap;
>> +}
>> +
>> +static always_inline bool domain_set_cap(
>> +    struct domain *d, uint8_t cap)
>> +{
>> +    switch ( cap )
>> +    {
>> +    case CAP_CONSOLE_IO:
>> +        d->capabilities |= cap;
>> +        break;
>> +    default:
>> +        return false;
>> +    }
>> +
>> +    return domain_has_cap(d, cap);
>> +}
>> +
>>   static always_inline bool hap_enabled(const struct domain *d)
>>   {
>>       /* sanitise_domain_config() rejects HAP && !HVM */
>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>> index 18f1ddd127..067ff1d111 100644
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -268,7 +268,7 @@ static XSM_INLINE int cf_check xsm_console_io(
>>       XSM_DEFAULT_ARG struct domain *d, int cmd)
>>   {
>>       XSM_ASSERT_ACTION(XSM_OTHER);
>> -    if ( d->is_console )
>> +    if ( domain_has_cap(d, CAP_CONSOLE_IO) )
>>           return xsm_default_action(XSM_HOOK, d, NULL);
>>   #ifdef CONFIG_VERBOSE_DEBUG
>>       if ( cmd == CONSOLEIO_write )
>> -- 
>> 2.20.1
>>

Re: [RFC 6/6] capabilities: convert attach debugger into a capability

[Daniel P. Smith]

On 8/1/23 21:06, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>> Expresses the ability to attach a debugger as a capability that a domain can be
>> provisioned.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/hvm/svm/svm.c      |  8 ++++----
>>   xen/arch/x86/hvm/vmx/realmode.c |  2 +-
>>   xen/arch/x86/hvm/vmx/vmcs.c     |  2 +-
>>   xen/arch/x86/hvm/vmx/vmx.c      | 10 +++++-----
>>   xen/arch/x86/traps.c            |  6 ++++--
>>   xen/common/domctl.c             |  6 ++++--
>>   xen/include/xen/sched.h         |  9 ++++++---
>>   7 files changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index 27170213ae..9872804d39 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -999,7 +999,7 @@ static void noreturn cf_check svm_do_resume(void)
>>   {
>>       struct vcpu *v = current;
>>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>> -    bool debug_state = (v->domain->debugger_attached ||
>> +    bool debug_state = (domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) ||
>>                           v->domain->arch.monitor.software_breakpoint_enabled ||
>>                           v->domain->arch.monitor.debug_exception_enabled);
>>       bool_t vcpu_guestmode = 0;
>> @@ -1335,7 +1335,7 @@ static void cf_check svm_inject_event(const struct x86_event *event)
>>           }
>>           /* fall through */
>>       case X86_EXC_BP:
>> -        if ( curr->domain->debugger_attached )
>> +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>>           {
>>               /* Debug/Int3: Trap to debugger. */
>>               domain_pause_for_debugger();
>> @@ -2732,7 +2732,7 @@ void svm_vmexit_handler(void)
>>   
>>       case VMEXIT_ICEBP:
>>       case VMEXIT_EXCEPTION_DB:
>> -        if ( !v->domain->debugger_attached )
>> +        if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>>           {
>>               unsigned int trap_type;
>>   
>> @@ -2769,7 +2769,7 @@ void svm_vmexit_handler(void)
>>           if ( insn_len == 0 )
>>                break;
>>   
>> -        if ( v->domain->debugger_attached )
>> +        if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>>           {
>>               /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
>>               __update_guest_eip(regs, insn_len);
>> diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
>> index ff44ddcfa6..f761026a9d 100644
>> --- a/xen/arch/x86/hvm/vmx/realmode.c
>> +++ b/xen/arch/x86/hvm/vmx/realmode.c
>> @@ -121,7 +121,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
>>   
>>       if ( rc == X86EMUL_EXCEPTION )
>>       {
>> -        if ( unlikely(curr->domain->debugger_attached) &&
>> +        if ( unlikely(domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH)) &&
>>                ((hvmemul_ctxt->ctxt.event.vector == X86_EXC_DB) ||
>>                 (hvmemul_ctxt->ctxt.event.vector == X86_EXC_BP)) )
>>           {
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 13719cc923..9474869018 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1912,7 +1912,7 @@ void cf_check vmx_do_resume(void)
>>           hvm_asid_flush_vcpu(v);
>>       }
>>   
>> -    debug_state = v->domain->debugger_attached
>> +    debug_state = domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH)
>>                     || v->domain->arch.monitor.software_breakpoint_enabled
>>                     || v->domain->arch.monitor.singlestep_enabled;
>>   
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 7ec44018d4..5069e3cbf3 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2041,7 +2041,7 @@ static void cf_check vmx_inject_event(const struct x86_event *event)
>>               break;
>>           /* fall through */
>>       case X86_EXC_BP:
>> -        if ( curr->domain->debugger_attached )
>> +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>>           {
>>               /* Debug/Int3: Trap to debugger. */
>>               domain_pause_for_debugger();
>> @@ -2121,7 +2121,7 @@ static void cf_check vmx_set_info_guest(struct vcpu *v)
>>        * immediately vmexit and hence make no progress.
>>        */
>>       __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
>> -    if ( v->domain->debugger_attached &&
>> +    if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) &&
>>            (v->arch.user_regs.eflags & X86_EFLAGS_TF) &&
>>            (intr_shadow & VMX_INTR_SHADOW_STI) )
>>       {
>> @@ -4283,7 +4283,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>                   }
>>               }
>>   
>> -            if ( !v->domain->debugger_attached )
>> +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>>               {
>>                   unsigned long insn_len = 0;
>>                   int rc;
>> @@ -4307,7 +4307,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>               break;
>>           case X86_EXC_BP:
>>               HVMTRACE_1D(TRAP, vector);
>> -            if ( !v->domain->debugger_attached )
>> +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>>               {
>>                   unsigned long insn_len;
>>                   int rc;
>> @@ -4647,7 +4647,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>                                 HVM_MONITOR_SINGLESTEP_BREAKPOINT,
>>                                 0, 0, 0);
>>   
>> -            if ( v->domain->debugger_attached )
>> +            if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>>                   domain_pause_for_debugger();
>>           }
>>   
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 4229bda159..041ced35ea 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1214,7 +1214,8 @@ void do_int3(struct cpu_user_regs *regs)
>>           return;
>>       }
>>   
>> -    if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
>> +    if ( guest_kernel_mode(curr, regs) &&
>> +         domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>>       {
>>           curr->arch.gdbsx_vcpu_event = X86_EXC_BP;
>>           domain_pause_for_debugger();
>> @@ -1995,7 +1996,8 @@ void do_debug(struct cpu_user_regs *regs)
>>       v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>>       v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
>>   
>> -    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
>> +    if ( guest_kernel_mode(v, regs) &&
>> +         domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>>       {
>>           domain_pause_for_debugger();
>>           return;
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 505e29c0dc..895ddf0600 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -99,7 +99,8 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>           ((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) |
>> +        (domain_has_cap(d, CAP_DEBUGGER_ATTACH) ?
>> +                                          XEN_DOMINF_debugged  : 0) |
>>           (is_xenstore_domain(d)          ? XEN_DOMINF_xs_domain : 0) |
>>           (is_hvm_domain(d)               ? XEN_DOMINF_hvm_guest : 0) |
>>           d->shutdown_code << XEN_DOMINF_shutdownshift;
>> @@ -643,7 +644,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>           else
>>           {
>>               domain_pause(d);
>> -            d->debugger_attached = !!op->u.setdebugging.enable;
>> +            if ( !!op->u.setdebugging.enable )
>> +                domain_set_cap(d, CAP_DEBUGGER_ATTACH);
>>               domain_unpause(d); /* causes guest to latch new status */
>>           }
>>           break;
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index ebfe65cd73..47eadb5008 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -474,9 +474,8 @@ struct domain
>>       uint8_t          role;
>>   #define CAP_CONSOLE_IO         (1U<<0)
>>   #define CAP_DISABLE_CPU_FAULT  (1U<<1)
>> -    uint8_t          capabilities;
>> -    /* Is this guest being debugged by dom0? */
>> -    bool             debugger_attached;
>> +#define CAP_DEBUGGER_ATTACH    (1U<<2)
>> +    uint16_t         capabilities;
> 
> No need to switch to uint16_t just yet?

I know space is tight in struct domain, wanted to reclaim the freed 
space into capabilities (or roles). One thing I was considering if 
enough space could be found is instead replace it all with a pointer to 
a new struct that held these values. It would allow using heap space and 
future growth of the structure. As of this patch, it is consuming 5 
bytes and would need to find an additional 3 bytes. Is there a 
willingness to entertain such an approach?

>>       /*
>>        * Set to true at the very end of domain creation, when the domain is
>>        * unpaused for the first time by the systemcontroller.
>> @@ -1166,6 +1165,10 @@ static always_inline bool domain_set_cap(
>>           if ( is_pv_domain(d) && is_control_domain(d) )
>>               d->capabilities |= cap;
>>           break;
>> +    case CAP_DEBUGGER_ATTACH:
>> +        if ( !is_control_domain(d) )
>> +            d->capabilities |= cap;
> 
> I take it is not possible to attach it to dom0? I am not familiar with
> XEN_DOMCTL_setdebugging but the hypercall doesn't seem to have such
> restriction.

Good question, on first glance I can't see why I added this. Going to 
walk through it again and see if I can figure out why, otherwise I will 
drop it.

>> +        break;
>>       default:
>>           return false;
>>       }
>> -- 
>> 2.20.1
>>

Re: [RFC 1/6] dom0: replace explict zero checks

[Daniel P. Smith]

On 8/3/23 09:36, Julien Grall wrote:
> Hi Daniel,

Hey Julien,

> On 03/08/2023 14:33, Daniel P. Smith wrote:
>> On 8/2/23 03:46, Jan Beulich wrote:
>>> On 01.08.2023 22:20, Daniel P. Smith wrote:
>>>> A legacy concept is that the initial domain will have a domain id of 
>>>> zero. As a
>>>> result there are places where a check that a domain is the inital 
>>>> domain is
>>>> determined by an explicit check that the domid is zero.
>>>
>>> It might help if you at least outlined here why/how this is going to
>>> change.
>>
>> Okay, I will try expanding on this further.
>>
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -1058,6 +1058,13 @@ void scheduler_disable(void);
>>>>   void watchdog_domain_init(struct domain *d);
>>>>   void watchdog_domain_destroy(struct domain *d);
>>>> +static always_inline bool is_initial_domain(const struct domain *d)
>>>> +{
>>>> +    static int init_domain_id = 0;
>>>
>>> This may then also help with the question on why you use a static
>>> variable here. (In any event the type of this variable wants to
>>> be correct; plain int isn't appropriate ...
>>
>> Ah, so this is a dated patch that I brought because of the abstraction 
>> it made. The intent in the original series for making it static was in 
>> preparation to handle the shim case where init_domid() would have 
>> return a non-zero value.
>>
>> So the static can be dropped and changed to domid_t.
> 
> Looking at one of the follow-up patch, I see:
> 
>   static always_inline bool is_initial_domain(const struct domain *d)
>   {
> -    static int init_domain_id = 0;
> -
> -    return d->domain_id == init_domain_id;
> +    return d->role & ROLE_UNBOUNDED_DOMAIN;
>   }
> 
> So is there any point to have the local variable? IOW, can't this simply 
> be "d->domain_id == 0"?

Nope, you are right. All need for it drops with the static gone.

v/r,
dps

Re: [RFC 6/6] capabilities: convert attach debugger into a capability

[Jan Beulich]

On 03.08.2023 17:52, Daniel P. Smith wrote:
> On 8/1/23 21:06, Stefano Stabellini wrote:
>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -474,9 +474,8 @@ struct domain
>>>       uint8_t          role;
>>>   #define CAP_CONSOLE_IO         (1U<<0)
>>>   #define CAP_DISABLE_CPU_FAULT  (1U<<1)
>>> -    uint8_t          capabilities;
>>> -    /* Is this guest being debugged by dom0? */
>>> -    bool             debugger_attached;
>>> +#define CAP_DEBUGGER_ATTACH    (1U<<2)
>>> +    uint16_t         capabilities;
>>
>> No need to switch to uint16_t just yet?
> 
> I know space is tight in struct domain, wanted to reclaim the freed 
> space into capabilities (or roles). One thing I was considering if 
> enough space could be found is instead replace it all with a pointer to 
> a new struct that held these values. It would allow using heap space and 
> future growth of the structure. As of this patch, it is consuming 5 
> bytes and would need to find an additional 3 bytes. Is there a 
> willingness to entertain such an approach?

Usually we do such conversion when data belonging to a subsystem (for lack
of a better term) grows big enough, not right away when eventual data size
isn't even known yet.

Jan

Re: [RFC 6/6] capabilities: convert attach debugger into a capability

[Daniel P. Smith]

On 8/3/23 11:59, Jan Beulich wrote:
> On 03.08.2023 17:52, Daniel P. Smith wrote:
>> On 8/1/23 21:06, Stefano Stabellini wrote:
>>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -474,9 +474,8 @@ struct domain
>>>>        uint8_t          role;
>>>>    #define CAP_CONSOLE_IO         (1U<<0)
>>>>    #define CAP_DISABLE_CPU_FAULT  (1U<<1)
>>>> -    uint8_t          capabilities;
>>>> -    /* Is this guest being debugged by dom0? */
>>>> -    bool             debugger_attached;
>>>> +#define CAP_DEBUGGER_ATTACH    (1U<<2)
>>>> +    uint16_t         capabilities;
>>>
>>> No need to switch to uint16_t just yet?
>>
>> I know space is tight in struct domain, wanted to reclaim the freed
>> space into capabilities (or roles). One thing I was considering if
>> enough space could be found is instead replace it all with a pointer to
>> a new struct that held these values. It would allow using heap space and
>> future growth of the structure. As of this patch, it is consuming 5
>> bytes and would need to find an additional 3 bytes. Is there a
>> willingness to entertain such an approach?
> 
> Usually we do such conversion when data belonging to a subsystem (for lack
> of a better term) grows big enough, not right away when eventual data size
> isn't even known yet.

Right and if there is a desire to instead follow the suggestion in my 
reply to Stefano on patch 4, that approach could be sizeable. Larger 
than 40 bits/flags, that I am less confident about.

v/r,
dps

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Stefano Stabellini]

On Thu, 3 Aug 2023, Daniel P. Smith wrote:
> On 8/1/23 20:54, Stefano Stabellini wrote:
> > On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> > > The existing concepts such as unbounded domain, ie. all powerful, control
> > > domain and hardware domain are, effectively, roles the domains provide for
> > > the
> > > system. Currently, these are represented with booleans within `struct
> > > domain`
> > > or global domid variables that are compared against. This patch begins to
> > > formalize these roles by replacing the `is_control` and `is_console`,
> > > along
> > > with expanding the check against the global `hardware_domain` with a
> > > single
> > > encapsulating role attribute in `struct domain`.
> > > 
> > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > 
> > This is definitely heading in the right direction
> 
> Thank you, it is good to know there is some agreement here.
> 
> > > ---
> > >   xen/arch/arm/domain_build.c |  2 ++
> > >   xen/arch/x86/setup.c        |  2 ++
> > >   xen/common/domain.c         | 14 +++++++++++++-
> > >   xen/include/xen/sched.h     | 16 +++++++++-------
> > >   xen/include/xsm/dummy.h     |  4 ++--
> > >   xen/xsm/flask/hooks.c       | 12 ++++++------
> > >   6 files changed, 34 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 39b4ee03a5..51b4daefe1 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -4201,6 +4201,8 @@ void __init create_dom0(void)
> > >       if ( IS_ERR(dom0) )
> > >           panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
> > >   +    dom0->role |= ROLE_UNBOUNDED_DOMAIN;
> > 
> > I am not a fan of "UNBOUNDED". Maybe "PRIMARY"? "PRIVILEGED"? "SUPER"?
> > "ROOT"?
> > 
> > I also recognize I am not good at naming things so I'll stop here and
> > let other provide better feedback :-)
> 
> In first version of hyperlaunch and in the early roles work, I was working to
> move toward eliminating this concept entirely. The reality is this is a model
> that has existed for over 20 years and there are those who accept and embrace
> the model. Introducing the name UNBOUNDED was to at least break the idea that
> the all powerful domain necessarily is the first/initial domain to run. With
> hyperlaunch, there are still security-based scenarios where you might want to
> run a DomB before starting an all privileged domain. I spent quite some time,
> probably more than I should have, to find a good name that expresses what this
> role is. Considering a comment below and a comment by Jan, I am starting to
> think a better way to view it is a domain that assumes all roles in the
> system. So your suggestions of SUPER or ROOT might be more fitting. I
> considered ROLE_ALL, but something about it doesn't sit right with me. With
> that said I welcome the yak shaving of naming to begin. ( ^_^)
> 
> > Also, do we actually need unbounded given that it gets replaced with
> > control_domain pretty soon?
> 
> Yes, because as mentioned above, this is meant to express a domain that has
> been assigned all roles, for which later the domain may decided to delegate
> the role to another domain.
> 
> > I am asking because I think that at least from a safety perspective it
> > would be a problem to run a domain as "unbounded". In a safety system,
> > we wouldn't want anything to be unbounded, not even temporarily at boot.
> > If "unbounded" is removed before running dom0, then of course it is no
> > problem because actually dom0 never gets to run with "unbounded" set.
> 
> I think this is were the name UNBOUNDED may have been a bad choice. The
> UNBOUNDED role is dom0. It is the control domain, the hardware domain, the
> Xenstore domain, and the crash domain (if that were to be solidified).
> 
> > (I am currently thinking of solving the privilege issue by using XSM and
> > removing most privileges from Dom0.)
> 
> I obviously would be a huge advocate of that approach. ( ^_^)

Thanks for the history, that helps. I was asking because I would like to
make sure that all the options below are possible and easy to achieve:

1) traditional dom0 + some traditional domUs booted in a dom0less fashion
2) only traditional domUs booted in a dom0less fashion (no dom0 at all)
3) not-godlike-but-still-super dom0 + some traditional domUs booted in a dom0less fashion
4) domB booting

This ROLE_ALL domain would be dom0 in 1) and would be domB in 4).
In 2), there is no dom0 so there should also be no ROLE_ALL domain. All
good so far.


In 3), it looked to me that we would be creating a ROLE_ALL domain, then
taking away some of the ROLEs. It doesn't sound right? Let's say that we
want to have a hardware_domain (in the sense of default recipient of
hardware, not necessarily privileged) with dm_ops access, but no domctl
access. How would you go about it?

Would it be required to go through the ROLE_ALL stage? How does it
compare to the way it would work today without this patch applied?
Today, does XSM kick in after is_privileged is set, effectively being
the same thing as XSM kicking in later and removing some ROLEs after
ROLE_ALL is already set? So, basically nothing is changing?



> > >       if ( alloc_dom0_vcpu0(dom0) == NULL )
> > >           panic("Error creating domain 0 vcpu0\n");
> > >   diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > > index 2dbe9857aa..4e20edc3bf 100644
> > > --- a/xen/arch/x86/setup.c
> > > +++ b/xen/arch/x86/setup.c
> > > @@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const
> > > module_t *image,
> > >       if ( IS_ERR(d) )
> > >           panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
> > >   +    d->role |= ROLE_UNBOUNDED_DOMAIN;
> > > +
> > >       init_dom0_cpuid_policy(d);
> > >         if ( alloc_dom0_vcpu0(d) == NULL )
> > > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > index 8fb3c052f5..0ff1d52e3d 100644
> > > --- a/xen/common/domain.c
> > > +++ b/xen/common/domain.c
> > > @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
> > >       setup_io_bitmap(dom0);
> > >   #endif
> > >   +    /*
> > > +     * "dom0" may have been created under the unbounded role, demote it
> > > from
> > > +     * that role, reducing it to the control domain role and any other
> > > roles it
> > > +     * may have been given.
> > > +     */
> > > +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
> > > +    dom0->role |= ROLE_CONTROL_DOMAIN;
> > 
> > I think we need a better definition of the three roles to understand
> > what this mean.
> 
> Definition and as highlighted, a better name.
> 
> > >       rcu_unlock_domain(dom0);
> > >         iommu_hwdom_init(d);
> > > @@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
> > >       }
> > >         /* Sort out our idea of is_control_domain(). */
> > > -    d->is_privileged = flags & CDF_privileged;
> > > +    if ( flags & CDF_privileged )
> > > +        d->role |= ROLE_CONTROL_DOMAIN;
> > > +    else
> > > +        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
> > >         /* Sort out our idea of is_hardware_domain(). */
> > >       if ( is_initial_domain(d) || domid == hardware_domid )
> > > @@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
> > >             old_hwdom = hardware_domain;
> > >           hardware_domain = d;
> > > +        d->role |= ROLE_HARDWARE_DOMAIN;
> > >       }
> > >         TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
> > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > index a9276a7bed..695f240326 100644
> > > --- a/xen/include/xen/sched.h
> > > +++ b/xen/include/xen/sched.h
> > > @@ -467,8 +467,10 @@ struct domain
> > >   #endif
> > >       /* is node-affinity automatically computed? */
> > >       bool             auto_node_affinity;
> > > -    /* Is this guest fully privileged (aka dom0)? */
> > > -    bool             is_privileged;
> > > +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
> > > +#define ROLE_CONTROL_DOMAIN    (1U<<1)
> > > +#define ROLE_HARDWARE_DOMAIN   (1U<<2)
> > 
> > This is a great step in the right direction but I think at least a short
> > in-code comment to explain the difference between the three
> 
> Ack.
> 
> > > +    uint8_t          role;
> > >       /* Can this guest access the Xen console? */
> > >       bool             is_console;
> > >       /* Is this guest being debugged by dom0? */
> > > @@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
> > >     static always_inline bool is_initial_domain(const struct domain *d)
> > >   {
> > > -    static int init_domain_id = 0;
> > > -
> > > -    return d->domain_id == init_domain_id;
> > > +    return d->role & ROLE_UNBOUNDED_DOMAIN;
> > >   }
> > 
> > As far as I can tell this is the only functional change in this patch:
> > given that dom0 loses unbounded soon after boot, the "is_initial_domain"
> > checks will start to fail?
> 
> Today, dom0 should not lose any of its roles at boot unless dom0less were to
> create a hardware domain.

I don't understand this sentence. To me, hardware_domain means "default
recipient of hardware devices". It also happens to be traditionally
associated with Dom0, so many privilege checks are hardware_domain
check, although they should be instead control_domain checks.

So if you say "dom0 should not lose any of its roles at boot unless
dom0less were to create a hardware domain", I read it as:

"dom0 (all powerful) would not lose any of its powers at boot unless we
created dom0 (hardware domain, all powerful) with other domUs at boot
using dom0less."

which I don't understand



> Upon reflection, I am thinking this check might want renaming to align with
> the rename of this role.
> 
> > We have a few of them in the code and I couldn't rule out that at least
> > these 3 could happen at runtime:
> > 
> > xen/common/sched/core.c:    else if ( is_initial_domain(d) &&
> > opt_dom0_vcpus_pin )
> > xen/common/sched/core.c:    else if ( is_initial_domain(d) )
> > xen/common/sched/arinc653.c:    if ( is_initial_domain(unit->domain) )
> > 
> > Maybe they need to be changed to control_domain checks?
> 
> Perhaps, I would want to study them a bit before switching them over.

+1

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Julien Grall]

Hi,

On 01/08/2023 21:20, Daniel P. Smith wrote:
> The field `is_console` suggests that the field represents a state of being or
> posession, not that it reflects the privilege to access the console. In this
> patch the field is renamed to capabilities to encapsulate the capabilities a
> domain has been granted. The first capability being the ability to read/write
> the Xen console.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/arm/domain_build.c |  4 +++-
>   xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
>   xen/include/xsm/dummy.h     |  2 +-
>   3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 51b4daefe1..ad7432b029 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -4076,7 +4076,9 @@ void __init create_domUs(void)
>               panic("Error creating domain %s (rc = %ld)\n",
>                     dt_node_name(node), PTR_ERR(d));
>   
> -        d->is_console = true;
> +        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )

Coding style: We don't usually add a space after '!'.

> +            printk("failed setting console_io on %pd\n", d);

I find a bit odd that we would continue even if the cap cannot be set. 
Can you clarify?

> +
>           dt_device_set_used_by(node, d->domain_id);
>   
>           rc = construct_domU(d, node);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ec0f9baff6..b04fbe0565 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -472,8 +472,8 @@ struct domain
>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>       uint8_t          role;
> -    /* Can this guest access the Xen console? */
> -    bool             is_console;
> +#define CAP_CONSOLE_IO  (1U<<0)
Coding style: Space before and after <<.

> +    uint8_t          capabilities;
>       /* Is this guest being debugged by dom0? */
>       bool             debugger_attached;
>       /*
> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu *v)
>       return is_hvm_domain(v->domain);
>   }
>   
> +static always_inline bool domain_has_cap(
> +    const struct domain *d, uint8_t cap)

Coding style: We don't usually wrap the arguments this way. See 
domain_create() for an example.

> +{
> +    return d->capabilities & cap;
> +}
> +
> +static always_inline bool domain_set_cap(
> +    struct domain *d, uint8_t cap)

Same about the coding style here.

Also, do you expect the cap to be set only when the domain is created? 
If not, would you prevent potentially concurrent update to d->capabilities?


> +{
> +    switch ( cap )
> +    {
> +    case CAP_CONSOLE_IO:
> +        d->capabilities |= cap;
> +        break;
> +    default:

Is this meant to be reached? If not, maybe add ASSERT_UNREACHABLE()?

> +        return false;
> +    }
> +
> +    return domain_has_cap(d, cap);
> +} > +
>   static always_inline bool hap_enabled(const struct domain *d)
>   {
>       /* sanitise_domain_config() rejects HAP && !HVM */
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 18f1ddd127..067ff1d111 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -268,7 +268,7 @@ static XSM_INLINE int cf_check xsm_console_io(
>       XSM_DEFAULT_ARG struct domain *d, int cmd)
>   {
>       XSM_ASSERT_ACTION(XSM_OTHER);
> -    if ( d->is_console )
> +    if ( domain_has_cap(d, CAP_CONSOLE_IO) )
>           return xsm_default_action(XSM_HOOK, d, NULL);
>   #ifdef CONFIG_VERBOSE_DEBUG
>       if ( cmd == CONSOLEIO_write )

Cheers,

-- 
Julien Grall

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Julien Grall]

Hi Daniel,

On 03/08/2023 16:41, Daniel P. Smith wrote:
> On 8/1/23 21:01, Stefano Stabellini wrote:
>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>> patch the field is renamed to capabilities to encapsulate the 
>>> capabilities a
>>> domain has been granted. The first capability being the ability to 
>>> read/write
>>> the Xen console.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>
>> Patch looks fine to me aside the two minor nits. I am not sure I
>> understand 100% the difference between capabilities and roles but I am
>> OK with the patch. I'd like to hear Julien's feedback on this as well.
> 
> This might be worth a section in the hypervisor-guide. As mentioned in 
> the cover letter, this was originally proposed as being under XSM. A 
> challenge I ran into is that, depending on your view, the 
> `is_privileged` field and `hardware_domain` global were either abused as 
> a function check and a non-resource privilege check or are just 
> multifaceted variables. This is why the concept of the role was struck 
> upon, it is more intuitive (for me at least) that have a role is 
> something that imparts accesses (privilege checks) and dictates 
> hypervisor behaviors when handling the domain (function checks). This 
> then brings us to an access or behavior that may be inherent to some 
> role(s) but may want to grant on an individually to a guest. A prime 
> example of this is console_io, for which it is inherent that the 
> hardware domain role will have access but may want to grant to a guest 
> without granting it an entire role. This is why I provided for 
> identifying these capabilities so that they may be assigned individually 
> to a domain.

Thanks for the explanation. Just to confirm my understanding, what you 
are suggesting is that for a given role, a domain will at least have the 
matching capabilities (more could be granted). Is that correct?

If so, this wouldn't this mean we can remove d->role and simply use 
d->capabilities?

> 
> While the role/capability is a natural progression from how the 
> hypervisor currently operates. Another approach that could be consider 
> to deliver a similar experience would be to break down every access and 
> function into a capability and then define the standard roles as a 
> conglomeration of certain capabilities.

At least from the explanation above, I think it would make sense to 
break down role to multiple capabilities.

Cheers,

-- 
Julien Grall

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Jan Beulich]

On 01.08.2023 22:20, Daniel P. Smith wrote:
> @@ -1076,7 +1076,8 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>      if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>          return false;
>  
> -    return evaluate_nospec(d == hardware_domain);
> +    return evaluate_nospec(((d->role & ROLE_HARDWARE_DOMAIN) ||
> +        is_initial_domain(d)) && (d == hardware_domain));
>  }
>  
>  /* This check is for functionality specific to a control domain */
> @@ -1085,7 +1086,8 @@ static always_inline bool is_control_domain(const struct domain *d)
>      if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>          return false;
>  
> -    return evaluate_nospec(d->is_privileged);
> +    return evaluate_nospec((d->role & ROLE_CONTROL_DOMAIN) ||
> +        is_initial_domain(d));
>  }

Why these complicated conditions, and not just the check of the single
relevant role bit? (Also note that indentation used here doesn't really
match our expectations, but there are other style issues in the patch
as well, which I assume is okay for an RFC.)

Jan

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Jan Beulich]

On 01.08.2023 22:20, Daniel P. Smith wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -472,8 +472,8 @@ struct domain
>  #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>  #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>      uint8_t          role;
> -    /* Can this guest access the Xen console? */
> -    bool             is_console;
> +#define CAP_CONSOLE_IO  (1U<<0)
> +    uint8_t          capabilities;
>      /* Is this guest being debugged by dom0? */
>      bool             debugger_attached;
>      /*
> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu *v)
>      return is_hvm_domain(v->domain);
>  }
>  
> +static always_inline bool domain_has_cap(
> +    const struct domain *d, uint8_t cap)
> +{
> +    return d->capabilities & cap;
> +}

What you implement here is effectively domain_has_any_cap(), which I
don't think is of much use. At the very least you want to assert that
cap is a power of two. But perhaps you rather want the caller to pass
in a bit number, not a mask, such that it's obvious that only one
thing can be checked at a time.

> +static always_inline bool domain_set_cap(
> +    struct domain *d, uint8_t cap)
> +{
> +    switch ( cap )
> +    {
> +    case CAP_CONSOLE_IO:
> +        d->capabilities |= cap;
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    return domain_has_cap(d, cap);
> +}

The "set" operation doesn't need to be an inline function, does it?

Jan

Re: [RFC 5/6] capabilities: add dom0 cpu faulting disable

[Jan Beulich]

On 01.08.2023 22:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -164,48 +164,46 @@ static void set_cpuid_faulting(bool enable)
>  
>  void ctxt_switch_levelling(const struct vcpu *next)
>  {
> -	const struct domain *nextd = next ? next->domain : NULL;
> -	bool enable_cpuid_faulting;
> -
> -	if (cpu_has_cpuid_faulting ||
> -	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
> -		/*
> -		 * No need to alter the faulting setting if we are switching
> -		 * to idle; it won't affect any code running in idle context.
> -		 */
> -		if (nextd && is_idle_domain(nextd))
> -			return;
> -		/*
> -		 * We *should* be enabling faulting for PV control domains.
> -		 *
> -		 * The domain builder has now been updated to not depend on
> -		 * seeing host CPUID values.  This makes it compatible with
> -		 * PVH toolstack domains, and lets us enable faulting by
> -		 * default for all PV domains.
> -		 *
> -		 * However, as PV control domains have never had faulting
> -		 * enforced on them before, there might plausibly be other
> -		 * dependenices on host CPUID data.  Therefore, we have left
> -		 * an interim escape hatch in the form of
> -		 * `dom0=no-cpuid-faulting` to restore the older behaviour.
> -		 */
> -		enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting ||
> -		                                  !is_control_domain(nextd) ||
> -		                                  !is_pv_domain(nextd)) &&
> -		                        (is_pv_domain(nextd) ||
> -		                         next->arch.msrs->
> -		                         misc_features_enables.cpuid_faulting);
> -
> -		if (cpu_has_cpuid_faulting)
> -			set_cpuid_faulting(enable_cpuid_faulting);
> -		else
> -			amd_set_cpuid_user_dis(enable_cpuid_faulting);
> -
> -		return;
> -	}
> -
> -	if (ctxt_switch_masking)
> -		alternative_vcall(ctxt_switch_masking, next);
> +    const struct domain *nextd = next ? next->domain : NULL;
> +    bool enable_cpuid_faulting;
> +
> +    if ( cpu_has_cpuid_faulting ||
> +         boot_cpu_has(X86_FEATURE_CPUID_USER_DIS) ) {
> +        /*
> +        * No need to alter the faulting setting if we are switching
> +        * to idle; it won't affect any code running in idle context.
> +        */
> +        if (nextd && is_idle_domain(nextd))
> +            return;
> +        /*
> +        * We *should* be enabling faulting for PV control domains.
> +        *
> +        * The domain builder has now been updated to not depend on
> +        * seeing host CPUID values.  This makes it compatible with
> +        * PVH toolstack domains, and lets us enable faulting by
> +        * default for all PV domains.
> +        *
> +        * However, as PV control domains have never had faulting
> +        * enforced on them before, there might plausibly be other
> +        * dependenices on host CPUID data.  Therefore, we have left
> +        * an interim escape hatch in the form of
> +        * `dom0=no-cpuid-faulting` to restore the older behaviour.
> +        */
> +        enable_cpuid_faulting = nextd &&
> +            domain_has_cap(nextd, CAP_DISABLE_CPU_FAULT) &&
> +            (is_pv_domain(nextd) ||
> +            next->arch.msrs->misc_features_enables.cpuid_faulting);
> +
> +        if (cpu_has_cpuid_faulting)
> +            set_cpuid_faulting(enable_cpuid_faulting);
> +        else
> +            amd_set_cpuid_user_dis(enable_cpuid_faulting);
> +
> +        return;
> +    }
> +
> +    if (ctxt_switch_masking)
> +        alternative_vcall(ctxt_switch_masking, next);
>  }

I don't think I can spot what exactly changes here. Please avoid re-
indenting the entire function.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image,
>  
>      d->role |= ROLE_UNBOUNDED_DOMAIN;
>  
> +    if ( !opt_dom0_cpuid_faulting &&
> +         !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) )
> +        printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d);

No "Dom" please when you use %pd. Also I don't think you mean "set". Plus
we commonly use "%pd: xyz failed\n".

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -472,7 +472,8 @@ struct domain
>  #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>  #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>      uint8_t          role;
> -#define CAP_CONSOLE_IO  (1U<<0)
> +#define CAP_CONSOLE_IO         (1U<<0)
> +#define CAP_DISABLE_CPU_FAULT  (1U<<1)
>      uint8_t          capabilities;
>      /* Is this guest being debugged by dom0? */
>      bool             debugger_attached;
> @@ -1160,6 +1161,11 @@ static always_inline bool domain_set_cap(
>      case CAP_CONSOLE_IO:
>          d->capabilities |= cap;
>          break;
> +    case CAP_DISABLE_CPU_FAULT:
> +        /* Disabling cpu faulting is only allowed for a PV control domain. */
> +        if ( is_pv_domain(d) && is_control_domain(d) )
> +            d->capabilities |= cap;
> +        break;

How do you express the x86-ness of this? Plus shouldn't this fail when
either of the two conditions isn't met?

Jan

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Daniel P. Smith]

On 8/3/23 16:19, Stefano Stabellini wrote:
> On Thu, 3 Aug 2023, Daniel P. Smith wrote:
>> On 8/1/23 20:54, Stefano Stabellini wrote:
>>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>>> The existing concepts such as unbounded domain, ie. all powerful, control
>>>> domain and hardware domain are, effectively, roles the domains provide for
>>>> the
>>>> system. Currently, these are represented with booleans within `struct
>>>> domain`
>>>> or global domid variables that are compared against. This patch begins to
>>>> formalize these roles by replacing the `is_control` and `is_console`,
>>>> along
>>>> with expanding the check against the global `hardware_domain` with a
>>>> single
>>>> encapsulating role attribute in `struct domain`.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>
>>> This is definitely heading in the right direction
>>
>> Thank you, it is good to know there is some agreement here.
>>
>>>> ---
>>>>    xen/arch/arm/domain_build.c |  2 ++
>>>>    xen/arch/x86/setup.c        |  2 ++
>>>>    xen/common/domain.c         | 14 +++++++++++++-
>>>>    xen/include/xen/sched.h     | 16 +++++++++-------
>>>>    xen/include/xsm/dummy.h     |  4 ++--
>>>>    xen/xsm/flask/hooks.c       | 12 ++++++------
>>>>    6 files changed, 34 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index 39b4ee03a5..51b4daefe1 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -4201,6 +4201,8 @@ void __init create_dom0(void)
>>>>        if ( IS_ERR(dom0) )
>>>>            panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
>>>>    +    dom0->role |= ROLE_UNBOUNDED_DOMAIN;
>>>
>>> I am not a fan of "UNBOUNDED". Maybe "PRIMARY"? "PRIVILEGED"? "SUPER"?
>>> "ROOT"?
>>>
>>> I also recognize I am not good at naming things so I'll stop here and
>>> let other provide better feedback :-)
>>
>> In first version of hyperlaunch and in the early roles work, I was working to
>> move toward eliminating this concept entirely. The reality is this is a model
>> that has existed for over 20 years and there are those who accept and embrace
>> the model. Introducing the name UNBOUNDED was to at least break the idea that
>> the all powerful domain necessarily is the first/initial domain to run. With
>> hyperlaunch, there are still security-based scenarios where you might want to
>> run a DomB before starting an all privileged domain. I spent quite some time,
>> probably more than I should have, to find a good name that expresses what this
>> role is. Considering a comment below and a comment by Jan, I am starting to
>> think a better way to view it is a domain that assumes all roles in the
>> system. So your suggestions of SUPER or ROOT might be more fitting. I
>> considered ROLE_ALL, but something about it doesn't sit right with me. With
>> that said I welcome the yak shaving of naming to begin. ( ^_^)
>>
>>> Also, do we actually need unbounded given that it gets replaced with
>>> control_domain pretty soon?
>>
>> Yes, because as mentioned above, this is meant to express a domain that has
>> been assigned all roles, for which later the domain may decided to delegate
>> the role to another domain.
>>
>>> I am asking because I think that at least from a safety perspective it
>>> would be a problem to run a domain as "unbounded". In a safety system,
>>> we wouldn't want anything to be unbounded, not even temporarily at boot.
>>> If "unbounded" is removed before running dom0, then of course it is no
>>> problem because actually dom0 never gets to run with "unbounded" set.
>>
>> I think this is were the name UNBOUNDED may have been a bad choice. The
>> UNBOUNDED role is dom0. It is the control domain, the hardware domain, the
>> Xenstore domain, and the crash domain (if that were to be solidified).
>>
>>> (I am currently thinking of solving the privilege issue by using XSM and
>>> removing most privileges from Dom0.)
>>
>> I obviously would be a huge advocate of that approach. ( ^_^)
> 
> Thanks for the history, that helps. I was asking because I would like to
> make sure that all the options below are possible and easy to achieve:
> 
> 1) traditional dom0 + some traditional domUs booted in a dom0less fashion
> 2) only traditional domUs booted in a dom0less fashion (no dom0 at all)
> 3) not-godlike-but-still-super dom0 + some traditional domUs booted in a dom0less fashion
> 4) domB booting
> 
> This ROLE_ALL domain would be dom0 in 1) and would be domB in 4).
> In 2), there is no dom0 so there should also be no ROLE_ALL domain. All
> good so far.

You are correct that constructing a traditional dom0 would be 
accomplished by assigning this role. As mentioned over in the 
hyperlaunch devicetree series that by a domain be a traditional dom0 
would not be about ensuring it was give domid 0, but that it was 
assigned this role.

As for domB, no, it would not get the ROLE_ALL. It's purpose is assist 
the hyperlaunch domain builder with functions that just should not be 
done inside the hypervisor. A less-than-ideal analogy, is domB is like 
an initrd for Linux. Like an initrd is an environment that runs in 
user-space with controlled kernel interfaces to interact with system, 
domB provides the concept of a domain role that has restricted access to 
hypervisor interfaces to those needed to configure/setup the domains 
constructed by the hypervisor. In the roles case, a course-grained 
common base set of capabilities will need to be determined, but those 
using FLASK will be able to do fine grained access assignment for more 
rigid/stringent use cases.

Yes, in scenario 2 you provided, no domain would get assigned ROLE_ALL.

> In 3), it looked to me that we would be creating a ROLE_ALL domain, then
> taking away some of the ROLEs. It doesn't sound right? Let's say that we
> want to have a hardware_domain (in the sense of default recipient of
> hardware, not necessarily privileged) with dm_ops access, but no domctl
> access. How would you go about it?

With what is there today for dom0less, you have to go down the path that 
when dom0less constructs the hwdom, this will trigger dom0 to be demoted 
to the control domain role before either of them get unpaused.

Moving forward to a when hyperlaunch appears, then you would just assign 
the control domain role to "dom0" and the hardware domain role to 
"hwdom" and nothing would ever be assigned ROLE_ALL.

> Would it be required to go through the ROLE_ALL stage? How does it
> compare to the way it would work today without this patch applied?
> Today, does XSM kick in after is_privileged is set, effectively being
> the same thing as XSM kicking in later and removing some ROLEs after
> ROLE_ALL is already set? So, basically nothing is changing?

Until hyperlaunch, yes you would have to go through ROLE_ALL, but in 
dom0less case, as long as the hwdom is one of the domains it constructs, 
dom0 would never run with that role.

Your next question is a bit tricky. XSM is effective fairly early in 
start_xen() and begins enforcing its policy. The tricky part is that 
there is functionality and hypervisor behaviors that are not protected 
behind an XSM hook but where is_privileged, checked via 
is_control_domain(), is used to controls how they work. There are other 
places in the code where one would say that DAC is enforced before MAC, 
specifically one of the is_{control,hardware,xenstore}_domain() is 
enforced before the fine grained XSM check is made. In most cases this 
is not an issue, the role can just be given to the domain and the FLASK 
policy can be relied upon to restrict down. But there are some esoteric 
cases that could be constructed where this behavior would break the 
security model.

>>>>        if ( alloc_dom0_vcpu0(dom0) == NULL )
>>>>            panic("Error creating domain 0 vcpu0\n");
>>>>    diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>> index 2dbe9857aa..4e20edc3bf 100644
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const
>>>> module_t *image,
>>>>        if ( IS_ERR(d) )
>>>>            panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
>>>>    +    d->role |= ROLE_UNBOUNDED_DOMAIN;
>>>> +
>>>>        init_dom0_cpuid_policy(d);
>>>>          if ( alloc_dom0_vcpu0(d) == NULL )
>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>> index 8fb3c052f5..0ff1d52e3d 100644
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
>>>>        setup_io_bitmap(dom0);
>>>>    #endif
>>>>    +    /*
>>>> +     * "dom0" may have been created under the unbounded role, demote it
>>>> from
>>>> +     * that role, reducing it to the control domain role and any other
>>>> roles it
>>>> +     * may have been given.
>>>> +     */
>>>> +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
>>>> +    dom0->role |= ROLE_CONTROL_DOMAIN;
>>>
>>> I think we need a better definition of the three roles to understand
>>> what this mean.
>>
>> Definition and as highlighted, a better name.
>>
>>>>        rcu_unlock_domain(dom0);
>>>>          iommu_hwdom_init(d);
>>>> @@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
>>>>        }
>>>>          /* Sort out our idea of is_control_domain(). */
>>>> -    d->is_privileged = flags & CDF_privileged;
>>>> +    if ( flags & CDF_privileged )
>>>> +        d->role |= ROLE_CONTROL_DOMAIN;
>>>> +    else
>>>> +        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
>>>>          /* Sort out our idea of is_hardware_domain(). */
>>>>        if ( is_initial_domain(d) || domid == hardware_domid )
>>>> @@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
>>>>              old_hwdom = hardware_domain;
>>>>            hardware_domain = d;
>>>> +        d->role |= ROLE_HARDWARE_DOMAIN;
>>>>        }
>>>>          TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>> index a9276a7bed..695f240326 100644
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -467,8 +467,10 @@ struct domain
>>>>    #endif
>>>>        /* is node-affinity automatically computed? */
>>>>        bool             auto_node_affinity;
>>>> -    /* Is this guest fully privileged (aka dom0)? */
>>>> -    bool             is_privileged;
>>>> +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
>>>> +#define ROLE_CONTROL_DOMAIN    (1U<<1)
>>>> +#define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>>
>>> This is a great step in the right direction but I think at least a short
>>> in-code comment to explain the difference between the three
>>
>> Ack.
>>
>>>> +    uint8_t          role;
>>>>        /* Can this guest access the Xen console? */
>>>>        bool             is_console;
>>>>        /* Is this guest being debugged by dom0? */
>>>> @@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
>>>>      static always_inline bool is_initial_domain(const struct domain *d)
>>>>    {
>>>> -    static int init_domain_id = 0;
>>>> -
>>>> -    return d->domain_id == init_domain_id;
>>>> +    return d->role & ROLE_UNBOUNDED_DOMAIN;
>>>>    }
>>>
>>> As far as I can tell this is the only functional change in this patch:
>>> given that dom0 loses unbounded soon after boot, the "is_initial_domain"
>>> checks will start to fail?
>>
>> Today, dom0 should not lose any of its roles at boot unless dom0less were to
>> create a hardware domain.
> 
> I don't understand this sentence. To me, hardware_domain means "default
> recipient of hardware devices". It also happens to be traditionally
> associated with Dom0, so many privilege checks are hardware_domain
> check, although they should be instead control_domain checks.

What I am saying is that with this patch, dom0 will retain ROLE_ALL 
unless a separate hardware domain is constructed. If a separate hardware 
domain, aka late hardware domain, is created, then dom0 will loose 
ROLE_ALL and be demoted/limited to only ROLE_CONTROL_DOMAIN and the new 
late hardware domain will be give ROLE_HARDWARE_DOMAIN.

> So if you say "dom0 should not lose any of its roles at boot unless
> dom0less were to create a hardware domain", I read it as:
> 
> "dom0 (all powerful) would not lose any of its powers at boot unless we
> created dom0 (hardware domain, all powerful) with other domUs at boot
> using dom0less."

Let's say dom0less set the xen command line parameter hardware_dom=1, 
and it has a configuration that constructs dom0 and 3 domU. The the 
first domU constructed would result in dom0 get limited to 
ROLE_CONTROL_DOMAIN and dom1 would be given ROLE_HARDWARE_DOMAIN. What I 
would advise against, as I think it would end up crashing Xen, is if you 
gave dom0less a configuration with command line parameter 
hardware_dom=1, no dom0 and 3 domU. This would trigger the 
late_hwdom_init() which would it ASSERT(dom0 != NULL) and crash the 
hypervisor.

> which I don't understand

Did the above help?

>> Upon reflection, I am thinking this check might want renaming to align with
>> the rename of this role.
>>
>>> We have a few of them in the code and I couldn't rule out that at least
>>> these 3 could happen at runtime:
>>>
>>> xen/common/sched/core.c:    else if ( is_initial_domain(d) &&
>>> opt_dom0_vcpus_pin )
>>> xen/common/sched/core.c:    else if ( is_initial_domain(d) )
>>> xen/common/sched/arinc653.c:    if ( is_initial_domain(unit->domain) )
>>>
>>> Maybe they need to be changed to control_domain checks?
>>
>> Perhaps, I would want to study them a bit before switching them over.
> 
> +1

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Daniel P. Smith]

On 8/8/23 11:15, Jan Beulich wrote:
> On 01.08.2023 22:20, Daniel P. Smith wrote:
>> @@ -1076,7 +1076,8 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>>       if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>           return false;
>>   
>> -    return evaluate_nospec(d == hardware_domain);
>> +    return evaluate_nospec(((d->role & ROLE_HARDWARE_DOMAIN) ||
>> +        is_initial_domain(d)) && (d == hardware_domain));
>>   }
>>   
>>   /* This check is for functionality specific to a control domain */
>> @@ -1085,7 +1086,8 @@ static always_inline bool is_control_domain(const struct domain *d)
>>       if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>           return false;
>>   
>> -    return evaluate_nospec(d->is_privileged);
>> +    return evaluate_nospec((d->role & ROLE_CONTROL_DOMAIN) ||
>> +        is_initial_domain(d));
>>   }
> 
> Why these complicated conditions, and not just the check of the single
> relevant role bit? (Also note that indentation used here doesn't really
> match our expectations, but there are other style issues in the patch
> as well, which I assume is okay for an RFC.)

They go away with suggestion to move ROLE_UNBOUNDED a concatenation of 
all the other roles.

Ack on the style.

v/r,
dps

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Daniel P. Smith]

On 8/3/23 17:24, Julien Grall wrote:
> Hi Daniel,
> 
> On 03/08/2023 16:41, Daniel P. Smith wrote:
>> On 8/1/23 21:01, Stefano Stabellini wrote:
>>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>>> patch the field is renamed to capabilities to encapsulate the 
>>>> capabilities a
>>>> domain has been granted. The first capability being the ability to 
>>>> read/write
>>>> the Xen console.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>
>>> Patch looks fine to me aside the two minor nits. I am not sure I
>>> understand 100% the difference between capabilities and roles but I am
>>> OK with the patch. I'd like to hear Julien's feedback on this as well.
>>
>> This might be worth a section in the hypervisor-guide. As mentioned in 
>> the cover letter, this was originally proposed as being under XSM. A 
>> challenge I ran into is that, depending on your view, the 
>> `is_privileged` field and `hardware_domain` global were either abused 
>> as a function check and a non-resource privilege check or are just 
>> multifaceted variables. This is why the concept of the role was struck 
>> upon, it is more intuitive (for me at least) that have a role is 
>> something that imparts accesses (privilege checks) and dictates 
>> hypervisor behaviors when handling the domain (function checks). This 
>> then brings us to an access or behavior that may be inherent to some 
>> role(s) but may want to grant on an individually to a guest. A prime 
>> example of this is console_io, for which it is inherent that the 
>> hardware domain role will have access but may want to grant to a guest 
>> without granting it an entire role. This is why I provided for 
>> identifying these capabilities so that they may be assigned 
>> individually to a domain.
> 
> Thanks for the explanation. Just to confirm my understanding, what you 
> are suggesting is that for a given role, a domain will at least have the 
> matching capabilities (more could be granted). Is that correct?
> 
> If so, this wouldn't this mean we can remove d->role and simply use 
> d->capabilities?

We could start out with CAP_CTRL and CAP_HW, but it is a little 
illogical. For instance, control domain has many capabilities, they just 
have never been fully broken out. XSM did some, but the focus there was 
just on system resources. Similar with the hardware domain. You are 
right that it would better deal with the limited number of bits 
currently available.

>>
>> While the role/capability is a natural progression from how the 
>> hypervisor currently operates. Another approach that could be consider 
>> to deliver a similar experience would be to break down every access 
>> and function into a capability and then define the standard roles as a 
>> conglomeration of certain capabilities.
> 
> At least from the explanation above, I think it would make sense to 
> break down role to multiple capabilities.

Would it be acceptable to do this incrementally over time as we are able 
to determine what needs to be broken out as a distinct capability?

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Daniel P. Smith]

On 8/3/23 17:03, Julien Grall wrote:
> Hi,

Greetings

> On 01/08/2023 21:20, Daniel P. Smith wrote:
>> The field `is_console` suggests that the field represents a state of 
>> being or
>> posession, not that it reflects the privilege to access the console. 
>> In this
>> patch the field is renamed to capabilities to encapsulate the 
>> capabilities a
>> domain has been granted. The first capability being the ability to 
>> read/write
>> the Xen console.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/arm/domain_build.c |  4 +++-
>>   xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
>>   xen/include/xsm/dummy.h     |  2 +-
>>   3 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 51b4daefe1..ad7432b029 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -4076,7 +4076,9 @@ void __init create_domUs(void)
>>               panic("Error creating domain %s (rc = %ld)\n",
>>                     dt_node_name(node), PTR_ERR(d));
>> -        d->is_console = true;
>> +        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )
> 
> Coding style: We don't usually add a space after '!'.

Ack.

>> +            printk("failed setting console_io on %pd\n", d);
> 
> I find a bit odd that we would continue even if the cap cannot be set. 
> Can you clarify?

This is the construction of a domU, so the system is very much capable 
of coming up and reviewing the hypervisor messages from dom0 to discover 
the issue. I am hard pressed to believe the hypervisor should be 
panicked because the domU is not allowed to use the hypervisor's console.

g>> +
>>           dt_device_set_used_by(node, d->domain_id);
>>           rc = construct_domU(d, node);
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index ec0f9baff6..b04fbe0565 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -472,8 +472,8 @@ struct domain
>>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>>       uint8_t          role;
>> -    /* Can this guest access the Xen console? */
>> -    bool             is_console;
>> +#define CAP_CONSOLE_IO  (1U<<0)
> Coding style: Space before and after <<.

Ack.

>> +    uint8_t          capabilities;
>>       /* Is this guest being debugged by dom0? */
>>       bool             debugger_attached;
>>       /*
>> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const 
>> struct vcpu *v)
>>       return is_hvm_domain(v->domain);
>>   }
>> +static always_inline bool domain_has_cap(
>> +    const struct domain *d, uint8_t cap)
> 
> Coding style: We don't usually wrap the arguments this way. See 
> domain_create() for an example.

I was informed it was[1], also, please see next_domain_in_cpupool() 
amongst many others further below.

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2021-07/msg01133.html

>> +{
>> +    return d->capabilities & cap;
>> +}
>> +
>> +static always_inline bool domain_set_cap(
>> +    struct domain *d, uint8_t cap)
> 
> Same about the coding style here.

Ditto.

> Also, do you expect the cap to be set only when the domain is created? 
> If not, would you prevent potentially concurrent update to d->capabilities?

Currently the only means being devise to set this is via hyperlaunch 
domain creation. If a domctl op was added to be able to manipulate the 
caps, then yes a lock on the domain would be advised to block. With that 
said, if we switch over to CAP_CTRL/HW, then it might be good to grab a 
lock on the domain for the late hardware domain case.

>> +{
>> +    switch ( cap )
>> +    {
>> +    case CAP_CONSOLE_IO:
>> +        d->capabilities |= cap;
>> +        break;
>> +    default:
> 
> Is this meant to be reached? If not, maybe add ASSERT_UNREACHABLE()?

Yah, that would be a good idea.

>> +        return false;
>> +    }
>> +
>> +    return domain_has_cap(d, cap);
>> +} > +
>>   static always_inline bool hap_enabled(const struct domain *d)
>>   {
>>       /* sanitise_domain_config() rejects HAP && !HVM */
>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>> index 18f1ddd127..067ff1d111 100644
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -268,7 +268,7 @@ static XSM_INLINE int cf_check xsm_console_io(
>>       XSM_DEFAULT_ARG struct domain *d, int cmd)
>>   {
>>       XSM_ASSERT_ACTION(XSM_OTHER);
>> -    if ( d->is_console )
>> +    if ( domain_has_cap(d, CAP_CONSOLE_IO) )
>>           return xsm_default_action(XSM_HOOK, d, NULL);
>>   #ifdef CONFIG_VERBOSE_DEBUG
>>       if ( cmd == CONSOLEIO_write )


v/r,
dps

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Daniel P. Smith]

On 8/8/23 11:24, Jan Beulich wrote:
> On 01.08.2023 22:20, Daniel P. Smith wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -472,8 +472,8 @@ struct domain
>>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>>       uint8_t          role;
>> -    /* Can this guest access the Xen console? */
>> -    bool             is_console;
>> +#define CAP_CONSOLE_IO  (1U<<0)
>> +    uint8_t          capabilities;
>>       /* Is this guest being debugged by dom0? */
>>       bool             debugger_attached;
>>       /*
>> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu *v)
>>       return is_hvm_domain(v->domain);
>>   }
>>   
>> +static always_inline bool domain_has_cap(
>> +    const struct domain *d, uint8_t cap)
>> +{
>> +    return d->capabilities & cap;
>> +}
> 
> What you implement here is effectively domain_has_any_cap(), which I
> don't think is of much use. At the very least you want to assert that
> cap is a power of two. But perhaps you rather want the caller to pass
> in a bit number, not a mask, such that it's obvious that only one
> thing can be checked at a time.

I did this thinking I was going to save keystrokes and encapsulate the 
check, but I just double checked and it would have only saved one. I 
will drop this and put the explicit bit checks at all the check sites.

>> +static always_inline bool domain_set_cap(
>> +    struct domain *d, uint8_t cap)
>> +{
>> +    switch ( cap )
>> +    {
>> +    case CAP_CONSOLE_IO:
>> +        d->capabilities |= cap;
>> +        break;
>> +    default:
>> +        return false;
>> +    }
>> +
>> +    return domain_has_cap(d, cap);
>> +}
> 
> The "set" operation doesn't need to be an inline function, does it?

I guess not, I can move into common/domain.c.

v/r,
dps

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Stefano Stabellini]

On Tue, 8 Aug 2023, Daniel P. Smith wrote:
> On 8/3/23 16:19, Stefano Stabellini wrote:
> > On Thu, 3 Aug 2023, Daniel P. Smith wrote:
> > > On 8/1/23 20:54, Stefano Stabellini wrote:
> > > > On Tue, 1 Aug 2023, Daniel P. Smith wrote:
> > > > > The existing concepts such as unbounded domain, ie. all powerful,
> > > > > control
> > > > > domain and hardware domain are, effectively, roles the domains provide
> > > > > for
> > > > > the
> > > > > system. Currently, these are represented with booleans within `struct
> > > > > domain`
> > > > > or global domid variables that are compared against. This patch begins
> > > > > to
> > > > > formalize these roles by replacing the `is_control` and `is_console`,
> > > > > along
> > > > > with expanding the check against the global `hardware_domain` with a
> > > > > single
> > > > > encapsulating role attribute in `struct domain`.
> > > > > 
> > > > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > > > 
> > > > This is definitely heading in the right direction
> > > 
> > > Thank you, it is good to know there is some agreement here.
> > > 
> > > > > ---
> > > > >    xen/arch/arm/domain_build.c |  2 ++
> > > > >    xen/arch/x86/setup.c        |  2 ++
> > > > >    xen/common/domain.c         | 14 +++++++++++++-
> > > > >    xen/include/xen/sched.h     | 16 +++++++++-------
> > > > >    xen/include/xsm/dummy.h     |  4 ++--
> > > > >    xen/xsm/flask/hooks.c       | 12 ++++++------
> > > > >    6 files changed, 34 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > > index 39b4ee03a5..51b4daefe1 100644
> > > > > --- a/xen/arch/arm/domain_build.c
> > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > @@ -4201,6 +4201,8 @@ void __init create_dom0(void)
> > > > >        if ( IS_ERR(dom0) )
> > > > >            panic("Error creating domain 0 (rc = %ld)\n",
> > > > > PTR_ERR(dom0));
> > > > >    +    dom0->role |= ROLE_UNBOUNDED_DOMAIN;
> > > > 
> > > > I am not a fan of "UNBOUNDED". Maybe "PRIMARY"? "PRIVILEGED"? "SUPER"?
> > > > "ROOT"?
> > > > 
> > > > I also recognize I am not good at naming things so I'll stop here and
> > > > let other provide better feedback :-)
> > > 
> > > In first version of hyperlaunch and in the early roles work, I was working
> > > to
> > > move toward eliminating this concept entirely. The reality is this is a
> > > model
> > > that has existed for over 20 years and there are those who accept and
> > > embrace
> > > the model. Introducing the name UNBOUNDED was to at least break the idea
> > > that
> > > the all powerful domain necessarily is the first/initial domain to run.
> > > With
> > > hyperlaunch, there are still security-based scenarios where you might want
> > > to
> > > run a DomB before starting an all privileged domain. I spent quite some
> > > time,
> > > probably more than I should have, to find a good name that expresses what
> > > this
> > > role is. Considering a comment below and a comment by Jan, I am starting
> > > to
> > > think a better way to view it is a domain that assumes all roles in the
> > > system. So your suggestions of SUPER or ROOT might be more fitting. I
> > > considered ROLE_ALL, but something about it doesn't sit right with me.
> > > With
> > > that said I welcome the yak shaving of naming to begin. ( ^_^)
> > > 
> > > > Also, do we actually need unbounded given that it gets replaced with
> > > > control_domain pretty soon?
> > > 
> > > Yes, because as mentioned above, this is meant to express a domain that
> > > has
> > > been assigned all roles, for which later the domain may decided to
> > > delegate
> > > the role to another domain.
> > > 
> > > > I am asking because I think that at least from a safety perspective it
> > > > would be a problem to run a domain as "unbounded". In a safety system,
> > > > we wouldn't want anything to be unbounded, not even temporarily at boot.
> > > > If "unbounded" is removed before running dom0, then of course it is no
> > > > problem because actually dom0 never gets to run with "unbounded" set.
> > > 
> > > I think this is were the name UNBOUNDED may have been a bad choice. The
> > > UNBOUNDED role is dom0. It is the control domain, the hardware domain, the
> > > Xenstore domain, and the crash domain (if that were to be solidified).
> > > 
> > > > (I am currently thinking of solving the privilege issue by using XSM and
> > > > removing most privileges from Dom0.)
> > > 
> > > I obviously would be a huge advocate of that approach. ( ^_^)
> > 
> > Thanks for the history, that helps. I was asking because I would like to
> > make sure that all the options below are possible and easy to achieve:
> > 
> > 1) traditional dom0 + some traditional domUs booted in a dom0less fashion
> > 2) only traditional domUs booted in a dom0less fashion (no dom0 at all)
> > 3) not-godlike-but-still-super dom0 + some traditional domUs booted in a
> > dom0less fashion
> > 4) domB booting
> > 
> > This ROLE_ALL domain would be dom0 in 1) and would be domB in 4).
> > In 2), there is no dom0 so there should also be no ROLE_ALL domain. All
> > good so far.
> 
> You are correct that constructing a traditional dom0 would be accomplished by
> assigning this role. As mentioned over in the hyperlaunch devicetree series
> that by a domain be a traditional dom0 would not be about ensuring it was give
> domid 0, but that it was assigned this role.
> 
> As for domB, no, it would not get the ROLE_ALL. It's purpose is assist the
> hyperlaunch domain builder with functions that just should not be done inside
> the hypervisor. A less-than-ideal analogy, is domB is like an initrd for
> Linux. Like an initrd is an environment that runs in user-space with
> controlled kernel interfaces to interact with system, domB provides the
> concept of a domain role that has restricted access to hypervisor interfaces
> to those needed to configure/setup the domains constructed by the hypervisor.
> In the roles case, a course-grained common base set of capabilities will need
> to be determined, but those using FLASK will be able to do fine grained access
> assignment for more rigid/stringent use cases.
> 
> Yes, in scenario 2 you provided, no domain would get assigned ROLE_ALL.
> 
> > In 3), it looked to me that we would be creating a ROLE_ALL domain, then
> > taking away some of the ROLEs. It doesn't sound right? Let's say that we
> > want to have a hardware_domain (in the sense of default recipient of
> > hardware, not necessarily privileged) with dm_ops access, but no domctl
> > access. How would you go about it?
> 
> With what is there today for dom0less, you have to go down the path that when
> dom0less constructs the hwdom, this will trigger dom0 to be demoted to the
> control domain role before either of them get unpaused.
> 
> Moving forward to a when hyperlaunch appears, then you would just assign the
> control domain role to "dom0" and the hardware domain role to "hwdom" and
> nothing would ever be assigned ROLE_ALL.

OK


> > Would it be required to go through the ROLE_ALL stage? How does it
> > compare to the way it would work today without this patch applied?
> > Today, does XSM kick in after is_privileged is set, effectively being
> > the same thing as XSM kicking in later and removing some ROLEs after
> > ROLE_ALL is already set? So, basically nothing is changing?
> 
> Until hyperlaunch, yes you would have to go through ROLE_ALL, but in dom0less
> case, as long as the hwdom is one of the domains it constructs, dom0 would
> never run with that role.

OK, I think we are on the same page. For extra clarity I want to add
that in scenario 3) there is no control domain at all. There is only
hardware_domain with a few control powers (not many).


> Your next question is a bit tricky. XSM is effective fairly early in
> start_xen() and begins enforcing its policy. The tricky part is that there is
> functionality and hypervisor behaviors that are not protected behind an XSM
> hook but where is_privileged, checked via is_control_domain(), is used to
> controls how they work. There are other places in the code where one would say
> that DAC is enforced before MAC, specifically one of the
> is_{control,hardware,xenstore}_domain() is enforced before the fine grained
> XSM check is made. In most cases this is not an issue, the role can just be
> given to the domain and the FLASK policy can be relied upon to restrict down.
> But there are some esoteric cases that could be constructed where this
> behavior would break the security model.

OK. I understand there are corner cases, but I am also understanding
that "nothing is changing" in terms of order of XSM enforcement and
deprivilege execution with this patch series? It is not getting "worse"
the window of time where Dom0 has ROLE_ALL/is_privileged?


> > > > >        if ( alloc_dom0_vcpu0(dom0) == NULL )
> > > > >            panic("Error creating domain 0 vcpu0\n");
> > > > >    diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > > > > index 2dbe9857aa..4e20edc3bf 100644
> > > > > --- a/xen/arch/x86/setup.c
> > > > > +++ b/xen/arch/x86/setup.c
> > > > > @@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const
> > > > > module_t *image,
> > > > >        if ( IS_ERR(d) )
> > > > >            panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
> > > > >    +    d->role |= ROLE_UNBOUNDED_DOMAIN;
> > > > > +
> > > > >        init_dom0_cpuid_policy(d);
> > > > >          if ( alloc_dom0_vcpu0(d) == NULL )
> > > > > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > > > index 8fb3c052f5..0ff1d52e3d 100644
> > > > > --- a/xen/common/domain.c
> > > > > +++ b/xen/common/domain.c
> > > > > @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
> > > > >        setup_io_bitmap(dom0);
> > > > >    #endif
> > > > >    +    /*
> > > > > +     * "dom0" may have been created under the unbounded role, demote
> > > > > it
> > > > > from
> > > > > +     * that role, reducing it to the control domain role and any
> > > > > other
> > > > > roles it
> > > > > +     * may have been given.
> > > > > +     */
> > > > > +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
> > > > > +    dom0->role |= ROLE_CONTROL_DOMAIN;
> > > > 
> > > > I think we need a better definition of the three roles to understand
> > > > what this mean.
> > > 
> > > Definition and as highlighted, a better name.
> > > 
> > > > >        rcu_unlock_domain(dom0);
> > > > >          iommu_hwdom_init(d);
> > > > > @@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
> > > > >        }
> > > > >          /* Sort out our idea of is_control_domain(). */
> > > > > -    d->is_privileged = flags & CDF_privileged;
> > > > > +    if ( flags & CDF_privileged )
> > > > > +        d->role |= ROLE_CONTROL_DOMAIN;
> > > > > +    else
> > > > > +        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
> > > > >          /* Sort out our idea of is_hardware_domain(). */
> > > > >        if ( is_initial_domain(d) || domid == hardware_domid )
> > > > > @@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
> > > > >              old_hwdom = hardware_domain;
> > > > >            hardware_domain = d;
> > > > > +        d->role |= ROLE_HARDWARE_DOMAIN;
> > > > >        }
> > > > >          TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
> > > > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > > > index a9276a7bed..695f240326 100644
> > > > > --- a/xen/include/xen/sched.h
> > > > > +++ b/xen/include/xen/sched.h
> > > > > @@ -467,8 +467,10 @@ struct domain
> > > > >    #endif
> > > > >        /* is node-affinity automatically computed? */
> > > > >        bool             auto_node_affinity;
> > > > > -    /* Is this guest fully privileged (aka dom0)? */
> > > > > -    bool             is_privileged;
> > > > > +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
> > > > > +#define ROLE_CONTROL_DOMAIN    (1U<<1)
> > > > > +#define ROLE_HARDWARE_DOMAIN   (1U<<2)
> > > > 
> > > > This is a great step in the right direction but I think at least a short
> > > > in-code comment to explain the difference between the three
> > > 
> > > Ack.
> > > 
> > > > > +    uint8_t          role;
> > > > >        /* Can this guest access the Xen console? */
> > > > >        bool             is_console;
> > > > >        /* Is this guest being debugged by dom0? */
> > > > > @@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
> > > > >      static always_inline bool is_initial_domain(const struct domain
> > > > > *d)
> > > > >    {
> > > > > -    static int init_domain_id = 0;
> > > > > -
> > > > > -    return d->domain_id == init_domain_id;
> > > > > +    return d->role & ROLE_UNBOUNDED_DOMAIN;
> > > > >    }
> > > > 
> > > > As far as I can tell this is the only functional change in this patch:
> > > > given that dom0 loses unbounded soon after boot, the "is_initial_domain"
> > > > checks will start to fail?
> > > 
> > > Today, dom0 should not lose any of its roles at boot unless dom0less were
> > > to
> > > create a hardware domain.
> > 
> > I don't understand this sentence. To me, hardware_domain means "default
> > recipient of hardware devices". It also happens to be traditionally
> > associated with Dom0, so many privilege checks are hardware_domain
> > check, although they should be instead control_domain checks.
> 
> What I am saying is that with this patch, dom0 will retain ROLE_ALL unless a
> separate hardware domain is constructed. If a separate hardware domain, aka
> late hardware domain, is created, then dom0 will loose ROLE_ALL and be
> demoted/limited to only ROLE_CONTROL_DOMAIN and the new late hardware domain
> will be give ROLE_HARDWARE_DOMAIN.
> 
> > So if you say "dom0 should not lose any of its roles at boot unless
> > dom0less were to create a hardware domain", I read it as:
> > 
> > "dom0 (all powerful) would not lose any of its powers at boot unless we
> > created dom0 (hardware domain, all powerful) with other domUs at boot
> > using dom0less."
> 
> Let's say dom0less set the xen command line parameter hardware_dom=1, and it
> has a configuration that constructs dom0 and 3 domU. The the first domU
> constructed would result in dom0 get limited to ROLE_CONTROL_DOMAIN and dom1
> would be given ROLE_HARDWARE_DOMAIN. What I would advise against, as I think
> it would end up crashing Xen, is if you gave dom0less a configuration with
> command line parameter hardware_dom=1, no dom0 and 3 domU. This would trigger
> the late_hwdom_init() which would it ASSERT(dom0 != NULL) and crash the
> hypervisor.

Thanks for the example, it is finally clear!

I don't think we should add hardware_dom=1 to the command line (I don't
know if it was just an example to make it easier to understand for me).
Instead it should a property on device tree. hardware_dom=1 is not great
because it is tied to the order of domain construction. Instead it
should be a device tree property under the specific domain. That way you
can clearly specify which one is tradition dom0, or which is
hardware_domain and which is control_domain.


> > which I don't understand
> 
> Did the above help?

A lot, thanks!


> > > Upon reflection, I am thinking this check might want renaming to align
> > > with
> > > the rename of this role.
> > > 
> > > > We have a few of them in the code and I couldn't rule out that at least
> > > > these 3 could happen at runtime:
> > > > 
> > > > xen/common/sched/core.c:    else if ( is_initial_domain(d) &&
> > > > opt_dom0_vcpus_pin )
> > > > xen/common/sched/core.c:    else if ( is_initial_domain(d) )
> > > > xen/common/sched/arinc653.c:    if ( is_initial_domain(unit->domain) )
> > > > 
> > > > Maybe they need to be changed to control_domain checks?
> > > 
> > > Perhaps, I would want to study them a bit before switching them over.
> > 
> > +1
> 

Re: [RFC 5/6] capabilities: add dom0 cpu faulting disable

[Daniel P. Smith]

On 8/8/23 11:30, Jan Beulich wrote:
> On 01.08.2023 22:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -164,48 +164,46 @@ static void set_cpuid_faulting(bool enable)
>>   
>>   void ctxt_switch_levelling(const struct vcpu *next)
>>   {
>> -	const struct domain *nextd = next ? next->domain : NULL;
>> -	bool enable_cpuid_faulting;
>> -
>> -	if (cpu_has_cpuid_faulting ||
>> -	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
>> -		/*
>> -		 * No need to alter the faulting setting if we are switching
>> -		 * to idle; it won't affect any code running in idle context.
>> -		 */
>> -		if (nextd && is_idle_domain(nextd))
>> -			return;
>> -		/*
>> -		 * We *should* be enabling faulting for PV control domains.
>> -		 *
>> -		 * The domain builder has now been updated to not depend on
>> -		 * seeing host CPUID values.  This makes it compatible with
>> -		 * PVH toolstack domains, and lets us enable faulting by
>> -		 * default for all PV domains.
>> -		 *
>> -		 * However, as PV control domains have never had faulting
>> -		 * enforced on them before, there might plausibly be other
>> -		 * dependenices on host CPUID data.  Therefore, we have left
>> -		 * an interim escape hatch in the form of
>> -		 * `dom0=no-cpuid-faulting` to restore the older behaviour.
>> -		 */
>> -		enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting ||
>> -		                                  !is_control_domain(nextd) ||
>> -		                                  !is_pv_domain(nextd)) &&
>> -		                        (is_pv_domain(nextd) ||
>> -		                         next->arch.msrs->
>> -		                         misc_features_enables.cpuid_faulting);
>> -
>> -		if (cpu_has_cpuid_faulting)
>> -			set_cpuid_faulting(enable_cpuid_faulting);
>> -		else
>> -			amd_set_cpuid_user_dis(enable_cpuid_faulting);
>> -
>> -		return;
>> -	}
>> -
>> -	if (ctxt_switch_masking)
>> -		alternative_vcall(ctxt_switch_masking, next);
>> +    const struct domain *nextd = next ? next->domain : NULL;
>> +    bool enable_cpuid_faulting;
>> +
>> +    if ( cpu_has_cpuid_faulting ||
>> +         boot_cpu_has(X86_FEATURE_CPUID_USER_DIS) ) {
>> +        /*
>> +        * No need to alter the faulting setting if we are switching
>> +        * to idle; it won't affect any code running in idle context.
>> +        */
>> +        if (nextd && is_idle_domain(nextd))
>> +            return;
>> +        /*
>> +        * We *should* be enabling faulting for PV control domains.
>> +        *
>> +        * The domain builder has now been updated to not depend on
>> +        * seeing host CPUID values.  This makes it compatible with
>> +        * PVH toolstack domains, and lets us enable faulting by
>> +        * default for all PV domains.
>> +        *
>> +        * However, as PV control domains have never had faulting
>> +        * enforced on them before, there might plausibly be other
>> +        * dependenices on host CPUID data.  Therefore, we have left
>> +        * an interim escape hatch in the form of
>> +        * `dom0=no-cpuid-faulting` to restore the older behaviour.
>> +        */
>> +        enable_cpuid_faulting = nextd &&
>> +            domain_has_cap(nextd, CAP_DISABLE_CPU_FAULT) &&
>> +            (is_pv_domain(nextd) ||
>> +            next->arch.msrs->misc_features_enables.cpuid_faulting);
>> +
>> +        if (cpu_has_cpuid_faulting)
>> +            set_cpuid_faulting(enable_cpuid_faulting);
>> +        else
>> +            amd_set_cpuid_user_dis(enable_cpuid_faulting);
>> +
>> +        return;
>> +    }
>> +
>> +    if (ctxt_switch_masking)
>> +        alternative_vcall(ctxt_switch_masking, next);
>>   }
> 
> I don't think I can spot what exactly changes here. Please avoid re-
> indenting the entire function.

Oh, that was not intentional. I must have done a retab as that entire 
function is indented using hardtabs.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image,
>>   
>>       d->role |= ROLE_UNBOUNDED_DOMAIN;
>>   
>> +    if ( !opt_dom0_cpuid_faulting &&
>> +         !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) )
>> +        printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d);
> 
> No "Dom" please when you use %pd. Also I don't think you mean "set". Plus
> we commonly use "%pd: xyz failed\n".

Ack on the "Dom" removal and "%pd:".

As for set, it failed to set the capability on the domain. You could say 
enable but that is nothing more than synonyms, not changing the meaning 
of the statement.

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -472,7 +472,8 @@ struct domain
>>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>>       uint8_t          role;
>> -#define CAP_CONSOLE_IO  (1U<<0)
>> +#define CAP_CONSOLE_IO         (1U<<0)
>> +#define CAP_DISABLE_CPU_FAULT  (1U<<1)
>>       uint8_t          capabilities;
>>       /* Is this guest being debugged by dom0? */
>>       bool             debugger_attached;
>> @@ -1160,6 +1161,11 @@ static always_inline bool domain_set_cap(
>>       case CAP_CONSOLE_IO:
>>           d->capabilities |= cap;
>>           break;
>> +    case CAP_DISABLE_CPU_FAULT:
>> +        /* Disabling cpu faulting is only allowed for a PV control domain. */
>> +        if ( is_pv_domain(d) && is_control_domain(d) )
>> +            d->capabilities |= cap;
>> +        break;
> 
> How do you express the x86-ness of this? Plus shouldn't this fail when
> either of the two conditions isn't met?

You are correct, since this moves an x86 capability out into common, it 
should be ifdef'ed for x86.

Correct me if I am wrong here, but in the original check it would 
evaluate that all three conditions to be true. All this change did is 
effectively move the last two conditions into domain_set_cap(). Thus 
storing the evaluation of the first two conditions during dom0 
capability setup for the result to later be evaluated during dom0 cpuid 
policy setup as it was done before.

v/r,
dps

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Daniel P. Smith]

On 8/8/23 19:38, Stefano Stabellini wrote:
> On Tue, 8 Aug 2023, Daniel P. Smith wrote:
>> On 8/3/23 16:19, Stefano Stabellini wrote:
>>> On Thu, 3 Aug 2023, Daniel P. Smith wrote:
>>>> On 8/1/23 20:54, Stefano Stabellini wrote:
>>>>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>>>>> The existing concepts such as unbounded domain, ie. all powerful,
>>>>>> control
>>>>>> domain and hardware domain are, effectively, roles the domains provide
>>>>>> for
>>>>>> the
>>>>>> system. Currently, these are represented with booleans within `struct
>>>>>> domain`
>>>>>> or global domid variables that are compared against. This patch begins
>>>>>> to
>>>>>> formalize these roles by replacing the `is_control` and `is_console`,
>>>>>> along
>>>>>> with expanding the check against the global `hardware_domain` with a
>>>>>> single
>>>>>> encapsulating role attribute in `struct domain`.
>>>>>>
>>>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>>>
>>>>> This is definitely heading in the right direction
>>>>
>>>> Thank you, it is good to know there is some agreement here.
>>>>
>>>>>> ---
>>>>>>     xen/arch/arm/domain_build.c |  2 ++
>>>>>>     xen/arch/x86/setup.c        |  2 ++
>>>>>>     xen/common/domain.c         | 14 +++++++++++++-
>>>>>>     xen/include/xen/sched.h     | 16 +++++++++-------
>>>>>>     xen/include/xsm/dummy.h     |  4 ++--
>>>>>>     xen/xsm/flask/hooks.c       | 12 ++++++------
>>>>>>     6 files changed, 34 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>>> index 39b4ee03a5..51b4daefe1 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -4201,6 +4201,8 @@ void __init create_dom0(void)
>>>>>>         if ( IS_ERR(dom0) )
>>>>>>             panic("Error creating domain 0 (rc = %ld)\n",
>>>>>> PTR_ERR(dom0));
>>>>>>     +    dom0->role |= ROLE_UNBOUNDED_DOMAIN;
>>>>>
>>>>> I am not a fan of "UNBOUNDED". Maybe "PRIMARY"? "PRIVILEGED"? "SUPER"?
>>>>> "ROOT"?
>>>>>
>>>>> I also recognize I am not good at naming things so I'll stop here and
>>>>> let other provide better feedback :-)
>>>>
>>>> In first version of hyperlaunch and in the early roles work, I was working
>>>> to
>>>> move toward eliminating this concept entirely. The reality is this is a
>>>> model
>>>> that has existed for over 20 years and there are those who accept and
>>>> embrace
>>>> the model. Introducing the name UNBOUNDED was to at least break the idea
>>>> that
>>>> the all powerful domain necessarily is the first/initial domain to run.
>>>> With
>>>> hyperlaunch, there are still security-based scenarios where you might want
>>>> to
>>>> run a DomB before starting an all privileged domain. I spent quite some
>>>> time,
>>>> probably more than I should have, to find a good name that expresses what
>>>> this
>>>> role is. Considering a comment below and a comment by Jan, I am starting
>>>> to
>>>> think a better way to view it is a domain that assumes all roles in the
>>>> system. So your suggestions of SUPER or ROOT might be more fitting. I
>>>> considered ROLE_ALL, but something about it doesn't sit right with me.
>>>> With
>>>> that said I welcome the yak shaving of naming to begin. ( ^_^)
>>>>
>>>>> Also, do we actually need unbounded given that it gets replaced with
>>>>> control_domain pretty soon?
>>>>
>>>> Yes, because as mentioned above, this is meant to express a domain that
>>>> has
>>>> been assigned all roles, for which later the domain may decided to
>>>> delegate
>>>> the role to another domain.
>>>>
>>>>> I am asking because I think that at least from a safety perspective it
>>>>> would be a problem to run a domain as "unbounded". In a safety system,
>>>>> we wouldn't want anything to be unbounded, not even temporarily at boot.
>>>>> If "unbounded" is removed before running dom0, then of course it is no
>>>>> problem because actually dom0 never gets to run with "unbounded" set.
>>>>
>>>> I think this is were the name UNBOUNDED may have been a bad choice. The
>>>> UNBOUNDED role is dom0. It is the control domain, the hardware domain, the
>>>> Xenstore domain, and the crash domain (if that were to be solidified).
>>>>
>>>>> (I am currently thinking of solving the privilege issue by using XSM and
>>>>> removing most privileges from Dom0.)
>>>>
>>>> I obviously would be a huge advocate of that approach. ( ^_^)
>>>
>>> Thanks for the history, that helps. I was asking because I would like to
>>> make sure that all the options below are possible and easy to achieve:
>>>
>>> 1) traditional dom0 + some traditional domUs booted in a dom0less fashion
>>> 2) only traditional domUs booted in a dom0less fashion (no dom0 at all)
>>> 3) not-godlike-but-still-super dom0 + some traditional domUs booted in a
>>> dom0less fashion
>>> 4) domB booting
>>>
>>> This ROLE_ALL domain would be dom0 in 1) and would be domB in 4).
>>> In 2), there is no dom0 so there should also be no ROLE_ALL domain. All
>>> good so far.
>>
>> You are correct that constructing a traditional dom0 would be accomplished by
>> assigning this role. As mentioned over in the hyperlaunch devicetree series
>> that by a domain be a traditional dom0 would not be about ensuring it was give
>> domid 0, but that it was assigned this role.
>>
>> As for domB, no, it would not get the ROLE_ALL. It's purpose is assist the
>> hyperlaunch domain builder with functions that just should not be done inside
>> the hypervisor. A less-than-ideal analogy, is domB is like an initrd for
>> Linux. Like an initrd is an environment that runs in user-space with
>> controlled kernel interfaces to interact with system, domB provides the
>> concept of a domain role that has restricted access to hypervisor interfaces
>> to those needed to configure/setup the domains constructed by the hypervisor.
>> In the roles case, a course-grained common base set of capabilities will need
>> to be determined, but those using FLASK will be able to do fine grained access
>> assignment for more rigid/stringent use cases.
>>
>> Yes, in scenario 2 you provided, no domain would get assigned ROLE_ALL.
>>
>>> In 3), it looked to me that we would be creating a ROLE_ALL domain, then
>>> taking away some of the ROLEs. It doesn't sound right? Let's say that we
>>> want to have a hardware_domain (in the sense of default recipient of
>>> hardware, not necessarily privileged) with dm_ops access, but no domctl
>>> access. How would you go about it?
>>
>> With what is there today for dom0less, you have to go down the path that when
>> dom0less constructs the hwdom, this will trigger dom0 to be demoted to the
>> control domain role before either of them get unpaused.
>>
>> Moving forward to a when hyperlaunch appears, then you would just assign the
>> control domain role to "dom0" and the hardware domain role to "hwdom" and
>> nothing would ever be assigned ROLE_ALL.
> 
> OK

Apologies I was more clear the first time.

>>> Would it be required to go through the ROLE_ALL stage? How does it
>>> compare to the way it would work today without this patch applied?
>>> Today, does XSM kick in after is_privileged is set, effectively being
>>> the same thing as XSM kicking in later and removing some ROLEs after
>>> ROLE_ALL is already set? So, basically nothing is changing?
>>
>> Until hyperlaunch, yes you would have to go through ROLE_ALL, but in dom0less
>> case, as long as the hwdom is one of the domains it constructs, dom0 would
>> never run with that role.
> 
> OK, I think we are on the same page. For extra clarity I want to add
> that in scenario 3) there is no control domain at all. There is only
> hardware_domain with a few control powers (not many).

If that is the case, then you need FLASK and would either change the 
dom0_t label to remove the privileges you don't want it to have or 
derive a new label from dom0_t, again with the privileges you don't want 
removed. I think the latter would be recommended as you can give that 
label a name to reflect what the domain is.

>> Your next question is a bit tricky. XSM is effective fairly early in
>> start_xen() and begins enforcing its policy. The tricky part is that there is
>> functionality and hypervisor behaviors that are not protected behind an XSM
>> hook but where is_privileged, checked via is_control_domain(), is used to
>> controls how they work. There are other places in the code where one would say
>> that DAC is enforced before MAC, specifically one of the
>> is_{control,hardware,xenstore}_domain() is enforced before the fine grained
>> XSM check is made. In most cases this is not an issue, the role can just be
>> given to the domain and the FLASK policy can be relied upon to restrict down.
>> But there are some esoteric cases that could be constructed where this
>> behavior would break the security model.
> 
> OK. I understand there are corner cases, but I am also understanding
> that "nothing is changing" in terms of order of XSM enforcement and
> deprivilege execution with this patch series? It is not getting "worse"
> the window of time where Dom0 has ROLE_ALL/is_privileged?

Correct.

>>>>>>         if ( alloc_dom0_vcpu0(dom0) == NULL )
>>>>>>             panic("Error creating domain 0 vcpu0\n");
>>>>>>     diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>>>> index 2dbe9857aa..4e20edc3bf 100644
>>>>>> --- a/xen/arch/x86/setup.c
>>>>>> +++ b/xen/arch/x86/setup.c
>>>>>> @@ -905,6 +905,8 @@ static struct domain *__init create_dom0(const
>>>>>> module_t *image,
>>>>>>         if ( IS_ERR(d) )
>>>>>>             panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
>>>>>>     +    d->role |= ROLE_UNBOUNDED_DOMAIN;
>>>>>> +
>>>>>>         init_dom0_cpuid_policy(d);
>>>>>>           if ( alloc_dom0_vcpu0(d) == NULL )
>>>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>>>> index 8fb3c052f5..0ff1d52e3d 100644
>>>>>> --- a/xen/common/domain.c
>>>>>> +++ b/xen/common/domain.c
>>>>>> @@ -340,6 +340,14 @@ static int late_hwdom_init(struct domain *d)
>>>>>>         setup_io_bitmap(dom0);
>>>>>>     #endif
>>>>>>     +    /*
>>>>>> +     * "dom0" may have been created under the unbounded role, demote
>>>>>> it
>>>>>> from
>>>>>> +     * that role, reducing it to the control domain role and any
>>>>>> other
>>>>>> roles it
>>>>>> +     * may have been given.
>>>>>> +     */
>>>>>> +    dom0->role &= ~(ROLE_UNBOUNDED_DOMAIN & ROLE_HARDWARE_DOMAIN);
>>>>>> +    dom0->role |= ROLE_CONTROL_DOMAIN;
>>>>>
>>>>> I think we need a better definition of the three roles to understand
>>>>> what this mean.
>>>>
>>>> Definition and as highlighted, a better name.
>>>>
>>>>>>         rcu_unlock_domain(dom0);
>>>>>>           iommu_hwdom_init(d);
>>>>>> @@ -609,7 +617,10 @@ struct domain *domain_create(domid_t domid,
>>>>>>         }
>>>>>>           /* Sort out our idea of is_control_domain(). */
>>>>>> -    d->is_privileged = flags & CDF_privileged;
>>>>>> +    if ( flags & CDF_privileged )
>>>>>> +        d->role |= ROLE_CONTROL_DOMAIN;
>>>>>> +    else
>>>>>> +        d->role &= ~ROLE_CONTROL_DOMAIN; /*ensure not set */
>>>>>>           /* Sort out our idea of is_hardware_domain(). */
>>>>>>         if ( is_initial_domain(d) || domid == hardware_domid )
>>>>>> @@ -619,6 +630,7 @@ struct domain *domain_create(domid_t domid,
>>>>>>               old_hwdom = hardware_domain;
>>>>>>             hardware_domain = d;
>>>>>> +        d->role |= ROLE_HARDWARE_DOMAIN;
>>>>>>         }
>>>>>>           TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>>>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>>>> index a9276a7bed..695f240326 100644
>>>>>> --- a/xen/include/xen/sched.h
>>>>>> +++ b/xen/include/xen/sched.h
>>>>>> @@ -467,8 +467,10 @@ struct domain
>>>>>>     #endif
>>>>>>         /* is node-affinity automatically computed? */
>>>>>>         bool             auto_node_affinity;
>>>>>> -    /* Is this guest fully privileged (aka dom0)? */
>>>>>> -    bool             is_privileged;
>>>>>> +#define ROLE_UNBOUNDED_DOMAIN  (1U<<0)
>>>>>> +#define ROLE_CONTROL_DOMAIN    (1U<<1)
>>>>>> +#define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>>>>
>>>>> This is a great step in the right direction but I think at least a short
>>>>> in-code comment to explain the difference between the three
>>>>
>>>> Ack.
>>>>
>>>>>> +    uint8_t          role;
>>>>>>         /* Can this guest access the Xen console? */
>>>>>>         bool             is_console;
>>>>>>         /* Is this guest being debugged by dom0? */
>>>>>> @@ -1060,9 +1062,7 @@ void watchdog_domain_destroy(struct domain *d);
>>>>>>       static always_inline bool is_initial_domain(const struct domain
>>>>>> *d)
>>>>>>     {
>>>>>> -    static int init_domain_id = 0;
>>>>>> -
>>>>>> -    return d->domain_id == init_domain_id;
>>>>>> +    return d->role & ROLE_UNBOUNDED_DOMAIN;
>>>>>>     }
>>>>>
>>>>> As far as I can tell this is the only functional change in this patch:
>>>>> given that dom0 loses unbounded soon after boot, the "is_initial_domain"
>>>>> checks will start to fail?
>>>>
>>>> Today, dom0 should not lose any of its roles at boot unless dom0less were
>>>> to
>>>> create a hardware domain.
>>>
>>> I don't understand this sentence. To me, hardware_domain means "default
>>> recipient of hardware devices". It also happens to be traditionally
>>> associated with Dom0, so many privilege checks are hardware_domain
>>> check, although they should be instead control_domain checks.
>>
>> What I am saying is that with this patch, dom0 will retain ROLE_ALL unless a
>> separate hardware domain is constructed. If a separate hardware domain, aka
>> late hardware domain, is created, then dom0 will loose ROLE_ALL and be
>> demoted/limited to only ROLE_CONTROL_DOMAIN and the new late hardware domain
>> will be give ROLE_HARDWARE_DOMAIN.
>>
>>> So if you say "dom0 should not lose any of its roles at boot unless
>>> dom0less were to create a hardware domain", I read it as:
>>>
>>> "dom0 (all powerful) would not lose any of its powers at boot unless we
>>> created dom0 (hardware domain, all powerful) with other domUs at boot
>>> using dom0less."
>>
>> Let's say dom0less set the xen command line parameter hardware_dom=1, and it
>> has a configuration that constructs dom0 and 3 domU. The the first domU
>> constructed would result in dom0 get limited to ROLE_CONTROL_DOMAIN and dom1
>> would be given ROLE_HARDWARE_DOMAIN. What I would advise against, as I think
>> it would end up crashing Xen, is if you gave dom0less a configuration with
>> command line parameter hardware_dom=1, no dom0 and 3 domU. This would trigger
>> the late_hwdom_init() which would it ASSERT(dom0 != NULL) and crash the
>> hypervisor.
> 
> Thanks for the example, it is finally clear!
> 
> I don't think we should add hardware_dom=1 to the command line (I don't
> know if it was just an example to make it easier to understand for me).
> Instead it should a property on device tree. hardware_dom=1 is not great
> because it is tied to the order of domain construction. Instead it
> should be a device tree property under the specific domain. That way you
> can clearly specify which one is tradition dom0, or which is
> hardware_domain and which is control_domain.

I was describing with today's code, how you would launch a late hardware 
domain. The logic to trigger the hardware transfer from dom0 to the 
hardware domain is triggered by the command line parameter 
`hardware-dom`. With hyperlaunch, this can eventually be retired and 
separate control domain and hardware domain can be constructed at launch 
as we expose access to roles/capabilities through its DT interface.

>>> which I don't understand
>>
>> Did the above help?
> 
> A lot, thanks!

Your welcome, glad I was able to articulate it better this time.

>>>> Upon reflection, I am thinking this check might want renaming to align
>>>> with
>>>> the rename of this role.
>>>>
>>>>> We have a few of them in the code and I couldn't rule out that at least
>>>>> these 3 could happen at runtime:
>>>>>
>>>>> xen/common/sched/core.c:    else if ( is_initial_domain(d) &&
>>>>> opt_dom0_vcpus_pin )
>>>>> xen/common/sched/core.c:    else if ( is_initial_domain(d) )
>>>>> xen/common/sched/arinc653.c:    if ( is_initial_domain(unit->domain) )
>>>>>
>>>>> Maybe they need to be changed to control_domain checks?
>>>>
>>>> Perhaps, I would want to study them a bit before switching them over.
>>>
>>> +1
>>

Re: [RFC 2/6] roles: provide abstraction for the possible domain roles

[Stefano Stabellini]

On Tue, 8 Aug 2023, Daniel P. Smith wrote:
> > Thanks for the example, it is finally clear!
> > 
> > I don't think we should add hardware_dom=1 to the command line (I don't
> > know if it was just an example to make it easier to understand for me).
> > Instead it should a property on device tree. hardware_dom=1 is not great
> > because it is tied to the order of domain construction. Instead it
> > should be a device tree property under the specific domain. That way you
> > can clearly specify which one is tradition dom0, or which is
> > hardware_domain and which is control_domain.
> 
> I was describing with today's code, how you would launch a late hardware
> domain. The logic to trigger the hardware transfer from dom0 to the hardware
> domain is triggered by the command line parameter `hardware-dom`. With
> hyperlaunch, this can eventually be retired and separate control domain and
> hardware domain can be constructed at launch as we expose access to
> roles/capabilities through its DT interface.

+1


> > > > which I don't understand
> > > 
> > > Did the above help?
> > 
> > A lot, thanks!
> 
> Your welcome, glad I was able to articulate it better this time.

Thanks for the explanation, it all looks good to me.

Re: [RFC 5/6] capabilities: add dom0 cpu faulting disable

[Jan Beulich]

On 09.08.2023 01:59, Daniel P. Smith wrote:
> On 8/8/23 11:30, Jan Beulich wrote:
>> On 01.08.2023 22:20, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image,
>>>   
>>>       d->role |= ROLE_UNBOUNDED_DOMAIN;
>>>   
>>> +    if ( !opt_dom0_cpuid_faulting &&
>>> +         !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) )
>>> +        printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d);
>>
>> No "Dom" please when you use %pd. Also I don't think you mean "set". Plus
>> we commonly use "%pd: xyz failed\n".
> 
> Ack on the "Dom" removal and "%pd:".
> 
> As for set, it failed to set the capability on the domain. You could say 
> enable but that is nothing more than synonyms, not changing the meaning 
> of the statement.

But you don't say "capability" in the message. That's what is being set.
But what you do instead is disable CPUID faulting. In fact I wonder
whether expressing that as a capability actually makes sense. To me a
capability is something a domain may make use of, but doesn't have to.
That's not the case here: CPUID faulting is either active for a domain,
or it is not.

Jan

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Julien Grall]

Hi Daniel,

On 08/08/2023 23:31, Daniel P. Smith wrote:
> 
> 
> On 8/3/23 17:24, Julien Grall wrote:
>> Hi Daniel,
>>
>> On 03/08/2023 16:41, Daniel P. Smith wrote:
>>> On 8/1/23 21:01, Stefano Stabellini wrote:
>>>> On Tue, 1 Aug 2023, Daniel P. Smith wrote:
>>>>> patch the field is renamed to capabilities to encapsulate the 
>>>>> capabilities a
>>>>> domain has been granted. The first capability being the ability to 
>>>>> read/write
>>>>> the Xen console.
>>>>>
>>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>>
>>>> Patch looks fine to me aside the two minor nits. I am not sure I
>>>> understand 100% the difference between capabilities and roles but I am
>>>> OK with the patch. I'd like to hear Julien's feedback on this as well.
>>>
>>> This might be worth a section in the hypervisor-guide. As mentioned 
>>> in the cover letter, this was originally proposed as being under XSM. 
>>> A challenge I ran into is that, depending on your view, the 
>>> `is_privileged` field and `hardware_domain` global were either abused 
>>> as a function check and a non-resource privilege check or are just 
>>> multifaceted variables. This is why the concept of the role was 
>>> struck upon, it is more intuitive (for me at least) that have a role 
>>> is something that imparts accesses (privilege checks) and dictates 
>>> hypervisor behaviors when handling the domain (function checks). This 
>>> then brings us to an access or behavior that may be inherent to some 
>>> role(s) but may want to grant on an individually to a guest. A prime 
>>> example of this is console_io, for which it is inherent that the 
>>> hardware domain role will have access but may want to grant to a 
>>> guest without granting it an entire role. This is why I provided for 
>>> identifying these capabilities so that they may be assigned 
>>> individually to a domain.
>>
>> Thanks for the explanation. Just to confirm my understanding, what you 
>> are suggesting is that for a given role, a domain will at least have 
>> the matching capabilities (more could be granted). Is that correct?
>>
>> If so, this wouldn't this mean we can remove d->role and simply use 
>> d->capabilities?
> 
> We could start out with CAP_CTRL and CAP_HW, but it is a little 
> illogical. For instance, control domain has many capabilities, they just 
> have never been fully broken out. XSM did some, but the focus there was 
> just on system resources. Similar with the hardware domain. You are 
> right that it would better deal with the limited number of bits 
> currently available.
> 
>>>
>>> While the role/capability is a natural progression from how the 
>>> hypervisor currently operates. Another approach that could be 
>>> consider to deliver a similar experience would be to break down every 
>>> access and function into a capability and then define the standard 
>>> roles as a conglomeration of certain capabilities.
>>
>> At least from the explanation above, I think it would make sense to 
>> break down role to multiple capabilities.
> 
> Would it be acceptable to do this incrementally over time as we are able 
> to determine what needs to be broken out as a distinct capability?

I would be fine with that. Note that some care will be needed for the 
Device-Tree to either version the capabilities or at least not break 
boot when using an old DT on a new Xen.

Cheers,

-- 
Julien Grall

Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Julien Grall]

Hi Daniel,

On 08/08/2023 23:49, Daniel P. Smith wrote:
> On 8/3/23 17:03, Julien Grall wrote:
>> On 01/08/2023 21:20, Daniel P. Smith wrote:
>>> The field `is_console` suggests that the field represents a state of 
>>> being or
>>> posession, not that it reflects the privilege to access the console. 
>>> In this
>>> patch the field is renamed to capabilities to encapsulate the 
>>> capabilities a
>>> domain has been granted. The first capability being the ability to 
>>> read/write
>>> the Xen console.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> ---
>>>   xen/arch/arm/domain_build.c |  4 +++-
>>>   xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
>>>   xen/include/xsm/dummy.h     |  2 +-
>>>   3 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 51b4daefe1..ad7432b029 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -4076,7 +4076,9 @@ void __init create_domUs(void)
>>>               panic("Error creating domain %s (rc = %ld)\n",
>>>                     dt_node_name(node), PTR_ERR(d));
>>> -        d->is_console = true;
>>> +        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )
>>
>> Coding style: We don't usually add a space after '!'.
> 
> Ack.
> 
>>> +            printk("failed setting console_io on %pd\n", d);
>>
>> I find a bit odd that we would continue even if the cap cannot be set. 
>> Can you clarify?
> 
> This is the construction of a domU, so the system is very much capable 
> of coming up and reviewing the hypervisor messages from dom0 to discover 
> the issue. I am hard pressed to believe the hypervisor should be 
> panicked because the domU is not allowed to use the hypervisor's console.

I understand the system may be able to boot. However, the problem is 
that it may take a while to discover that the console is not working 
properly (the more if you only use it for error logging).

So on Arm, we have so always decided to fail early rather than late in 
order to help debugging. So I would rather not change the behavior even 
if this is "just" for the console.

If you expect the console to be disabled, then we should provide a 
property in the Device-Tree to select/deselect. It should not be hidden.

> 
> g>> +
>>>           dt_device_set_used_by(node, d->domain_id);
>>>           rc = construct_domU(d, node);
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index ec0f9baff6..b04fbe0565 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -472,8 +472,8 @@ struct domain
>>>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>>>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>>>       uint8_t          role;
>>> -    /* Can this guest access the Xen console? */
>>> -    bool             is_console;
>>> +#define CAP_CONSOLE_IO  (1U<<0)
>> Coding style: Space before and after <<.
> 
> Ack.
> 
>>> +    uint8_t          capabilities;
>>>       /* Is this guest being debugged by dom0? */
>>>       bool             debugger_attached;
>>>       /*
>>> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const 
>>> struct vcpu *v)
>>>       return is_hvm_domain(v->domain);
>>>   }
>>> +static always_inline bool domain_has_cap(
>>> +    const struct domain *d, uint8_t cap)
>>
>> Coding style: We don't usually wrap the arguments this way. See 
>> domain_create() for an example.
> 
> I was informed it was[1], also, please see next_domain_in_cpupool() 
> amongst many others further below.

The unwritten coding style strike again... I am not sure where the 
agreement comes from. At least on Arm, we have been using the first 
version in that thread and if it can't be wrapped to 80 characters, then 
move the "static inline void " on its own line.

The advantage with the Arm approach is that parameters are always 
indented the same way. Anyway, the way you wrote is not my personal 
preference but I am also not up to bikeshed too much on it. Hopefully 
this sort of style discussion will be resolved with clang-format.

[...]

> 
>> Also, do you expect the cap to be set only when the domain is created? 
>> If not, would you prevent potentially concurrent update to 
>> d->capabilities?
> 
> Currently the only means being devise to set this is via hyperlaunch 
> domain creation. If a domctl op was added to be able to manipulate the 
> caps, then yes a lock on the domain would be advised to block.

Loking at patch #6, you are using domctl there.

> With that 
> said, if we switch over to CAP_CTRL/HW, then it might be good to grab a 
> lock on the domain for the late hardware domain case.

Are you planning to clear the caps? If not, then using set_bit() and 
test_bit() should be enough.

Cheers,

-- 
Julien Grall

Re: [RFC 6/6] capabilities: convert attach debugger into a capability

[Julien Grall]

Hi Daniel,

On 01/08/2023 21:20, Daniel P. Smith wrote:
> Expresses the ability to attach a debugger as a capability that a domain can be
> provisioned.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/x86/hvm/svm/svm.c      |  8 ++++----
>   xen/arch/x86/hvm/vmx/realmode.c |  2 +-
>   xen/arch/x86/hvm/vmx/vmcs.c     |  2 +-
>   xen/arch/x86/hvm/vmx/vmx.c      | 10 +++++-----
>   xen/arch/x86/traps.c            |  6 ++++--
>   xen/common/domctl.c             |  6 ++++--
>   xen/include/xen/sched.h         |  9 ++++++---
>   7 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 27170213ae..9872804d39 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -999,7 +999,7 @@ static void noreturn cf_check svm_do_resume(void)
>   {
>       struct vcpu *v = current;
>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> -    bool debug_state = (v->domain->debugger_attached ||
> +    bool debug_state = (domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) ||
>                           v->domain->arch.monitor.software_breakpoint_enabled ||
>                           v->domain->arch.monitor.debug_exception_enabled);
>       bool_t vcpu_guestmode = 0;
> @@ -1335,7 +1335,7 @@ static void cf_check svm_inject_event(const struct x86_event *event)
>           }
>           /* fall through */
>       case X86_EXC_BP:
> -        if ( curr->domain->debugger_attached )
> +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>           {
>               /* Debug/Int3: Trap to debugger. */
>               domain_pause_for_debugger();
> @@ -2732,7 +2732,7 @@ void svm_vmexit_handler(void)
>   
>       case VMEXIT_ICEBP:
>       case VMEXIT_EXCEPTION_DB:
> -        if ( !v->domain->debugger_attached )
> +        if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>           {
>               unsigned int trap_type;
>   
> @@ -2769,7 +2769,7 @@ void svm_vmexit_handler(void)
>           if ( insn_len == 0 )
>                break;
>   
> -        if ( v->domain->debugger_attached )
> +        if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>           {
>               /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
>               __update_guest_eip(regs, insn_len);
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
> index ff44ddcfa6..f761026a9d 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -121,7 +121,7 @@ void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
>   
>       if ( rc == X86EMUL_EXCEPTION )
>       {
> -        if ( unlikely(curr->domain->debugger_attached) &&
> +        if ( unlikely(domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH)) &&
>                ((hvmemul_ctxt->ctxt.event.vector == X86_EXC_DB) ||
>                 (hvmemul_ctxt->ctxt.event.vector == X86_EXC_BP)) )
>           {
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 13719cc923..9474869018 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1912,7 +1912,7 @@ void cf_check vmx_do_resume(void)
>           hvm_asid_flush_vcpu(v);
>       }
>   
> -    debug_state = v->domain->debugger_attached
> +    debug_state = domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH)
>                     || v->domain->arch.monitor.software_breakpoint_enabled
>                     || v->domain->arch.monitor.singlestep_enabled;
>   
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 7ec44018d4..5069e3cbf3 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2041,7 +2041,7 @@ static void cf_check vmx_inject_event(const struct x86_event *event)
>               break;
>           /* fall through */
>       case X86_EXC_BP:
> -        if ( curr->domain->debugger_attached )
> +        if ( domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>           {
>               /* Debug/Int3: Trap to debugger. */
>               domain_pause_for_debugger();
> @@ -2121,7 +2121,7 @@ static void cf_check vmx_set_info_guest(struct vcpu *v)
>        * immediately vmexit and hence make no progress.
>        */
>       __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
> -    if ( v->domain->debugger_attached &&
> +    if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) &&
>            (v->arch.user_regs.eflags & X86_EFLAGS_TF) &&
>            (intr_shadow & VMX_INTR_SHADOW_STI) )
>       {
> @@ -4283,7 +4283,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                   }
>               }
>   
> -            if ( !v->domain->debugger_attached )
> +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>               {
>                   unsigned long insn_len = 0;
>                   int rc;
> @@ -4307,7 +4307,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>               break;
>           case X86_EXC_BP:
>               HVMTRACE_1D(TRAP, vector);
> -            if ( !v->domain->debugger_attached )
> +            if ( !domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>               {
>                   unsigned long insn_len;
>                   int rc;
> @@ -4647,7 +4647,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                                 HVM_MONITOR_SINGLESTEP_BREAKPOINT,
>                                 0, 0, 0);
>   
> -            if ( v->domain->debugger_attached )
> +            if ( domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>                   domain_pause_for_debugger();
>           }
>   
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 4229bda159..041ced35ea 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1214,7 +1214,8 @@ void do_int3(struct cpu_user_regs *regs)
>           return;
>       }
>   
> -    if ( guest_kernel_mode(curr, regs) && curr->domain->debugger_attached )
> +    if ( guest_kernel_mode(curr, regs) &&
> +         domain_has_cap(curr->domain, CAP_DEBUGGER_ATTACH) )
>       {
>           curr->arch.gdbsx_vcpu_event = X86_EXC_BP;
>           domain_pause_for_debugger();
> @@ -1995,7 +1996,8 @@ void do_debug(struct cpu_user_regs *regs)
>       v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
>       v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
>   
> -    if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
> +    if ( guest_kernel_mode(v, regs) &&
> +         domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) )
>       {
>           domain_pause_for_debugger();
>           return;
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 505e29c0dc..895ddf0600 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -99,7 +99,8 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>           ((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) |
> +        (domain_has_cap(d, CAP_DEBUGGER_ATTACH) ?
> +                                          XEN_DOMINF_debugged  : 0) |
>           (is_xenstore_domain(d)          ? XEN_DOMINF_xs_domain : 0) |
>           (is_hvm_domain(d)               ? XEN_DOMINF_hvm_guest : 0) |
>           d->shutdown_code << XEN_DOMINF_shutdownshift;
> @@ -643,7 +644,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>           else
>           {
>               domain_pause(d);
> -            d->debugger_attached = !!op->u.setdebugging.enable;
> +            if ( !!op->u.setdebugging.enable )
> +                domain_set_cap(d, CAP_DEBUGGER_ATTACH);

 From my understanding, before this patch, it was possible to detach the 
debugger. But now, you don't seem to allow clearing. Is it intended? If 
so, can you explain why? (The outcome would want to be written down in 
the commit message).

>               domain_unpause(d); /* causes guest to latch new status */
>           }
>           break;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ebfe65cd73..47eadb5008 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -474,9 +474,8 @@ struct domain
>       uint8_t          role;
>   #define CAP_CONSOLE_IO         (1U<<0)
>   #define CAP_DISABLE_CPU_FAULT  (1U<<1)
> -    uint8_t          capabilities;
> -    /* Is this guest being debugged by dom0? */
> -    bool             debugger_attached;
> +#define CAP_DEBUGGER_ATTACH    (1U<<2)
> +    uint16_t         capabilities;
>       /*
>        * Set to true at the very end of domain creation, when the domain is
>        * unpaused for the first time by the systemcontroller.
> @@ -1166,6 +1165,10 @@ static always_inline bool domain_set_cap(
>           if ( is_pv_domain(d) && is_control_domain(d) )
>               d->capabilities |= cap;
>           break;
> +    case CAP_DEBUGGER_ATTACH:
> +        if ( !is_control_domain(d) )

This seems to be a new restriction. Can you explain why?

> +            d->capabilities |= cap;
> +        break;
>       default:
>           return false;
>       }

Cheers,

-- 
Julien Grall

Re: [RFC 6/6] capabilities: convert attach debugger into a capability

[Jan Beulich]

On 01.08.2023 22:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -999,7 +999,7 @@ static void noreturn cf_check svm_do_resume(void)
>  {
>      struct vcpu *v = current;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> -    bool debug_state = (v->domain->debugger_attached ||
> +    bool debug_state = (domain_has_cap(v->domain, CAP_DEBUGGER_ATTACH) ||
>                          v->domain->arch.monitor.software_breakpoint_enabled ||
>                          v->domain->arch.monitor.debug_exception_enabled);

Aren't you mixing capability and state here? d->debugger_attached set
means a debugger _is_ attached. The capability only says whether a
debugger may attach.

Jan