thr3ads.net - Xen devel - [PATCH v2 0/9] Rename/remove IS

If this information is useful, please help other people find it:
Share via:

Daniel De Graaf

2013-Apr-11 20:13 UTC

[PATCH v2 0/9] Rename/remove IS_PRIV

Following the conversion of most IS_PRIV hooks to XSM, the remaining
references to this function generally deal with direct hardware access
and not with the type of privilege checks that are best controlled by
XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain
and is used only when dealing with accesses that are both required by
dom0 and where it does not make sense to grant access to a domain other
than dom0.

There are a number of existing places in the hypervisor that check
domain_id for equality to zero to make some distinction on dom0; this
series replaces these checks with is_hardware_domain to be consistent in
how the hypervisor checks a domain''s access.

The first three patches have been posted before; the rest are new. I
have boot-tested the change on x86 (with PV domains), but have not
compile-tested on ARM.

Cleanup of IS_PRIV checks that should not be is_hardware_domain:
    [PATCH 1/9] xen/arch/x86: remove IS_PRIV access check bypasses
    [PATCH 2/9] xen/xsm: add hooks for claim
    [PATCH 3/9] hvm: convert access check for nested HVM to XSM
    [PATCH 4/9] xen/arch/x86: remove IS_PRIV_FOR references
    [PATCH 5/9] xen/arch/arm: remove rcu_lock_target_domain_by_id

Replace remaining calls to IS_PRIV:
    [PATCH 6/9] xen: rename IS_PRIV to is_hardware_domain

Use is_hardware_domain locations where (domid == 0) was used:
    [PATCH 7/9] xen: use domid check in is_hardware_domain
    [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid =   
[PATCH 9/9] IOMMU: use is_hardware_domain instead of domid == 0

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 1/9] xen/arch/x86: remove IS_PRIV access check bypasses

Several domctl functions dealing with rangesets contain a short-circuit
bypass if the domain is privileged. Since the construction of domain 0
permits access to all I/O ranges, the call to irq_access_permitted will
normally return true even without the IS_PRIV check, and the presence of
the IS_PRIV check prevents the creation of a privileged domain without
access to specific devices or IO memory ranges.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domctl.c | 12 ++++--------
 xen/arch/x86/irq.c    |  3 +--
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a196e2a..327a792 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -578,8 +578,7 @@ long arch_do_domctl(
             break;
 
         ret = -EPERM;
-        if ( !IS_PRIV(current->domain) &&
-             !irq_access_permitted(current->domain, bind->machine_irq) )
+        if ( !irq_access_permitted(current->domain, bind->machine_irq) )
             break;
 
         ret = -ESRCH;
@@ -602,8 +601,7 @@ long arch_do_domctl(
         bind = &(domctl->u.bind_pt_irq);
 
         ret = -EPERM;
-        if ( !IS_PRIV(current->domain) &&
-             !irq_access_permitted(current->domain, bind->machine_irq) )
+        if ( !irq_access_permitted(current->domain, bind->machine_irq) )
             break;
 
         ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
@@ -637,8 +635,7 @@ long arch_do_domctl(
             break;
 
         ret = -EPERM;
-        if ( !IS_PRIV(current->domain) &&
-             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns -
1) )
+        if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns -
1) )
             break;
 
         ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
@@ -715,8 +712,7 @@ long arch_do_domctl(
         }
 
         ret = -EPERM;
-        if ( !IS_PRIV(current->domain) &&
-             !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
+        if ( !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
             break;
 
         ret = xsm_ioport_mapping(XSM_HOOK, d, fmp, fmp + np - 1, add);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index dffb33a..33480fe 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1872,8 +1872,7 @@ int map_domain_pirq(
 
     ASSERT(spin_is_locked(&d->event_lock));
 
-    if ( !IS_PRIV(current->domain) &&
-         !irq_access_permitted(current->domain, pirq))
+    if ( !irq_access_permitted(current->domain, pirq))
         return -EPERM;
 
     if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >=
nr_irqs )
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 2/9] xen/xsm: add hooks for claim

This replaces the IS_PRIV checks on these newly introduced operations
with equivalent XSM hooks, and adds FLASK access vectors for them.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
---
 tools/flask/policy/policy/modules/xen/xen.if |  2 +-
 xen/common/memory.c                          | 15 ++++++++-------
 xen/include/xsm/dummy.h                      | 12 ++++++++++++
 xen/include/xsm/xsm.h                        | 12 ++++++++++++
 xen/xsm/dummy.c                              |  2 ++
 xen/xsm/flask/hooks.c                        | 13 +++++++++++++
 xen/xsm/flask/policy/access_vectors          |  4 +++-
 7 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.if
b/tools/flask/policy/policy/modules/xen/xen.if
index 2ce2212..fbb329d 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -49,7 +49,7 @@ define(`create_domain_common'', `
 			getdomaininfo hypercall setvcpucontext setextvcpucontext
 			getscheduler getvcpuinfo getvcpuextstate getaddrsize
 			getvcpuaffinity setvcpuaffinity };
-	allow $1 $2:domain2 { set_cpuid settsc setscheduler };
+	allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim };
 	allow $1 $2:security check_context;
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage
mmuext_op };
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 68501d1..3239d53 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -712,9 +712,6 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     case XENMEM_claim_pages:
-        if ( !IS_PRIV(current->domain) )
-            return -EPERM;
-
         if ( copy_from_guest(&reservation, arg, 1) )
             return -EFAULT;
 
@@ -731,17 +728,21 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( d == NULL )
             return -EINVAL;
 
-        rc = domain_set_outstanding_pages(d, reservation.nr_extents);
+        rc = xsm_claim_pages(XSM_PRIV, d);
+
+        if ( !rc )
+            rc = domain_set_outstanding_pages(d, reservation.nr_extents);
 
         rcu_unlock_domain(d);
 
         break;
 
     case XENMEM_get_outstanding_pages:
-        if ( !IS_PRIV(current->domain) )
-            return -EPERM;
+        rc = xsm_xenmem_get_outstanding_pages(XSM_PRIV);
+
+        if ( !rc )
+            rc = get_outstanding_claims();
 
-        rc = get_outstanding_claims();
         break;
 
     default:
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 025936a..ae81a51 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -247,6 +247,18 @@ static XSM_INLINE int xsm_memory_pin_page(XSM_DEFAULT_ARG
struct domain *d1, str
     return xsm_default_action(action, d1, d2);
 }
 
+static XSM_INLINE int xsm_claim_pages(XSM_DEFAULT_ARG struct domain *d)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    return xsm_default_action(action, current->domain, d);
+}
+
+static XSM_INLINE int xsm_xenmem_get_outstanding_pages(XSM_DEFAULT_VOID)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    return xsm_default_action(action, current->domain, NULL);
+}
+
 static XSM_INLINE int xsm_evtchn_unbound(XSM_DEFAULT_ARG struct domain *d,
struct evtchn *chn,
                                          domid_t id2)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index cba744c..bfe5635 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -92,6 +92,8 @@ struct xsm_operations {
     int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct
page_info *page);
     int (*add_to_physmap) (struct domain *d1, struct domain *d2);
     int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
+    int (*claim_pages) (struct domain *d);
+    int (*xenmem_get_outstanding_pages) (void);
 
     int (*console_io) (struct domain *d, int cmd);
 
@@ -348,6 +350,16 @@ static inline int xsm_remove_from_physmap(xsm_default_t
def, struct domain *d1,
     return xsm_ops->remove_from_physmap(d1, d2);
 }
 
+static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
+{
+    return xsm_ops->claim_pages(d);
+}
+
+static inline int xsm_xenmem_get_outstanding_pages(xsm_default_t def)
+{
+    return xsm_ops->xenmem_get_outstanding_pages();
+}
+
 static inline int xsm_console_io (xsm_default_t def, struct domain *d, int cmd)
 {
     return xsm_ops->console_io(d, cmd);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 6f1e0b4..594bbca 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -66,6 +66,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, memory_adjust_reservation);
     set_to_dummy_if_null(ops, memory_stat_reservation);
     set_to_dummy_if_null(ops, memory_pin_page);
+    set_to_dummy_if_null(ops, claim_pages);
+    set_to_dummy_if_null(ops, xenmem_get_outstanding_pages);
 
     set_to_dummy_if_null(ops, console_io);
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 29a78dd..832bc3f 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -417,6 +417,17 @@ static int flask_memory_pin_page(struct domain *d1, struct
domain *d2,
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PINPAGE);
 }
 
+static int flask_claim_pages(struct domain *d)
+{
+    return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCLAIM);
+}
+
+static int flask_xenmem_get_outstanding_pages(void)
+{
+    return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN,
+                                XEN__HEAP, NULL);
+}
+
 static int flask_console_io(struct domain *d, int cmd)
 {
     u32 perm;
@@ -1473,6 +1484,8 @@ static struct xsm_operations flask_ops = {
     .memory_adjust_reservation = flask_memory_adjust_reservation,
     .memory_stat_reservation = flask_memory_stat_reservation,
     .memory_pin_page = flask_memory_pin_page,
+    .claim_pages = flask_claim_pages,
+    .xenmem_get_outstanding_pages = flask_xenmem_get_outstanding_pages,
 
     .console_io = flask_console_io,
 
diff --git a/xen/xsm/flask/policy/access_vectors
b/xen/xsm/flask/policy/access_vectors
index 36cbacf..a2a502f 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -54,7 +54,7 @@ class xen
     debug
 # XEN_SYSCTL_getcpuinfo, XENPF_get_cpu_version, XENPF_get_cpuinfo
     getcpuinfo
-# XEN_SYSCTL_availheap
+# XEN_SYSCTL_availheap, XENMEM_get_outstanding_pages
     heap
 # XEN_SYSCTL_get_pmstat, XEN_SYSCTL_pm_op, XENPF_set_processor_pminfo,
 # XENPF_core_parking
@@ -190,6 +190,8 @@ class domain2
     settsc
 # XEN_DOMCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_putinfo
     setscheduler
+# XENMEM_claim_pages
+    setclaim
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM
domains
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 3/9] hvm: convert access check for nested HVM to XSM

This adds an XSM hook for enabling nested HVM support, replacing an
IS_PRIV check.  This hook is a partial duplicate with the xsm_hvm_param
hook, but using the existing hook would require adding the index to the
hook and would require the use of a custom hook for the xsm-disabled
case (using XSM_OTHER, which is less immediately readable) - whereas
adding a new hook retains the clarity of the existing code.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
---
 tools/flask/policy/policy/modules/xen/xen.if | 3 ++-
 xen/arch/x86/hvm/hvm.c                       | 6 ++----
 xen/include/xsm/dummy.h                      | 6 ++++++
 xen/include/xsm/xsm.h                        | 6 ++++++
 xen/xsm/dummy.c                              | 1 +
 xen/xsm/flask/hooks.c                        | 6 ++++++
 xen/xsm/flask/policy/access_vectors          | 2 ++
 7 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.if
b/tools/flask/policy/policy/modules/xen/xen.if
index fbb329d..f6f24a5 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -54,7 +54,8 @@ define(`create_domain_common'', `
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage
mmuext_op };
 	allow $1 $2:grant setup;
-	allow $1 $2:hvm { cacheattr getparam hvmctl irqlevel pciroute sethvmc setparam
pcilevel trackdirtyvram };
+	allow $1 $2:hvm { cacheattr getparam hvmctl irqlevel pciroute sethvmc
+			setparam pcilevel trackdirtyvram nested };
 '')
 
 # create_domain(priv, target)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 38e87ce..8522963 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3909,11 +3909,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE_PARAM(void) arg)
                     rc = -EINVAL;
                 break;
             case HVM_PARAM_NESTEDHVM:
-                if ( !IS_PRIV(current->domain) )
-                {
-                    rc = -EPERM;
+                rc = xsm_hvm_param_nested(XSM_PRIV, d);
+                if ( rc )
                     break;
-                }
                 if ( a.value > 1 )
                     rc = -EINVAL;
                 /* Remove the check below once we have
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index ae81a51..d53132b 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -485,6 +485,12 @@ static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct
domain *d, unsigned l
     return xsm_default_action(action, current->domain, d);
 }
 
+static XSM_INLINE int xsm_hvm_param_nested(XSM_DEFAULT_ARG struct domain *d)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    return xsm_default_action(action, current->domain, d);
+}
+
 #ifdef CONFIG_X86
 static XSM_INLINE int xsm_shadow_control(XSM_DEFAULT_ARG struct domain *d,
uint32_t op)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index bfe5635..b4065ad 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -132,6 +132,7 @@ struct xsm_operations {
     long (*do_xsm_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op);
 
     int (*hvm_param) (struct domain *d, unsigned long op);
+    int (*hvm_param_nested) (struct domain *d);
 
 #ifdef CONFIG_X86
     int (*shadow_control) (struct domain *d, uint32_t op);
@@ -505,6 +506,11 @@ static inline int xsm_hvm_param (xsm_default_t def, struct
domain *d, unsigned l
     return xsm_ops->hvm_param(d, op);
 }
 
+static inline int xsm_hvm_param_nested (xsm_default_t def, struct domain *d)
+{
+    return xsm_ops->hvm_param_nested(d);
+}
+
 #ifdef CONFIG_X86
 static inline int xsm_shadow_control (xsm_default_t def, struct domain *d,
uint32_t op)
 {
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 594bbca..f769642 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -103,6 +103,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, tmem_op);
     set_to_dummy_if_null(ops, tmem_control);
     set_to_dummy_if_null(ops, hvm_param);
+    set_to_dummy_if_null(ops, hvm_param_nested);
 
     set_to_dummy_if_null(ops, do_xsm_op);
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 832bc3f..78f0b92 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1101,6 +1101,11 @@ static int flask_hvm_param(struct domain *d, unsigned
long op)
     return current_has_perm(d, SECCLASS_HVM, perm);
 }
 
+static int flask_hvm_param_nested(struct domain *d)
+{
+    return current_has_perm(d, SECCLASS_HVM, HVM__NESTED);
+}
+
 #ifdef CONFIG_X86
 static int flask_shadow_control(struct domain *d, uint32_t op)
 {
@@ -1517,6 +1522,7 @@ static struct xsm_operations flask_ops = {
     .tmem_op = flask_tmem_op,
     .tmem_control = flask_tmem_control,
     .hvm_param = flask_hvm_param,
+    .hvm_param_nested = flask_hvm_param_nested,
 
     .do_xsm_op = do_flask_op,
 
diff --git a/xen/xsm/flask/policy/access_vectors
b/xen/xsm/flask/policy/access_vectors
index a2a502f..176235c 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -234,6 +234,8 @@ class hvm
 #  source = domain whose memory is being shared
 #  target = client domain
     share_mem
+# HVMOP_set_param setting HVM_PARAM_NESTEDHVM
+    nested
 }
 
 # Class event describes event channels.  Interdomain event channels have their
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 4/9] xen/arch/x86: remove IS_PRIV_FOR references

The check in guest_physmap_mark_populate_on_demand is redundant, since
its only caller is populate_physmap whose only caller checks the
xsm_memory_adjust_reservation hook prior to calling.

Add a new XSM hook for the other two checks since they allow privileged
domains to arbitrarily map a guest''s memory.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm.c                   |  2 +-
 xen/arch/x86/mm/p2m-pod.c           |  3 ---
 xen/arch/x86/mm/shadow/multi.c      | 19 +++++++++++--------
 xen/include/xsm/dummy.h             |  6 ++++++
 xen/include/xsm/xsm.h               |  6 ++++++
 xen/xsm/dummy.c                     |  1 +
 xen/xsm/flask/hooks.c               |  6 ++++++
 xen/xsm/flask/policy/access_vectors |  3 +++
 8 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 58e1402..0cd4203 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -815,7 +815,7 @@ get_page_from_l1e(
          * minor hack can go away.
          */
         if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) ||
-             !IS_PRIV_FOR(pg_owner, real_pg_owner) )
+             xsm_priv_mapping(XSM_TARGET, pg_owner, real_pg_owner) )
         {
             MEM_LOG("pg_owner %d l1e_owner %d, but real_pg_owner %d",
                     pg_owner->domain_id, l1e_owner->domain_id,
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 55936c6..04ffbcb 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1117,9 +1117,6 @@ guest_physmap_mark_populate_on_demand(struct domain *d,
unsigned long gfn,
     mfn_t omfn;
     int rc = 0;
 
-    if ( !IS_PRIV_FOR(current->domain, d) )
-        return -EPERM;
-
     if ( !paging_mode_translate(d) )
         return -EINVAL;
 
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index a593f76..a8ef75e 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -29,6 +29,7 @@
 #include <xen/perfc.h>
 #include <xen/domain_page.h>
 #include <xen/iocap.h>
+#include <xsm/xsm.h>
 #include <asm/page.h>
 #include <asm/current.h>
 #include <asm/shadow.h>
@@ -849,14 +850,16 @@ shadow_get_page_from_l1e(shadow_l1e_t sl1e, struct domain
*d, p2m_type_t type)
          !shadow_mode_translate(d) &&
          mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) &&
          (owner = page_get_owner(mfn_to_page(mfn))) &&
-         (d != owner) &&
-         IS_PRIV_FOR(d, owner))
-    {
-        res = get_page_from_l1e(sl1e, d, owner);
-        SHADOW_PRINTK("privileged domain %d installs map of mfn %05lx
"
-                       "which is owned by domain %d: %s\n",
-                       d->domain_id, mfn_x(mfn), owner->domain_id,
-                       res >= 0 ? "success" : "failed");
+         (d != owner) )
+    {
+        res = xsm_priv_mapping(XSM_TARGET, d, owner);
+        if ( !res ) {
+            res = get_page_from_l1e(sl1e, d, owner);
+            SHADOW_PRINTK("privileged domain %d installs map of mfn %05lx
"
+                           "which is owned by domain %d: %s\n",
+                           d->domain_id, mfn_x(mfn), owner->domain_id,
+                           res >= 0 ? "success" :
"failed");
+        }
     }
 
     /* Okay, it might still be a grant mapping PTE.  Try it. */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index d53132b..9bfe596 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -586,6 +586,12 @@ static XSM_INLINE int xsm_update_va_mapping(XSM_DEFAULT_ARG
struct domain *d, st
     return xsm_default_action(action, d, f);
 }
 
+static XSM_INLINE int xsm_priv_mapping(XSM_DEFAULT_ARG struct domain *d, struct
domain *t)
+{
+    XSM_ASSERT_ACTION(XSM_TARGET);
+    return xsm_default_action(action, d, t);
+}
+
 static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct
xen_domctl_bind_pt_irq *bind)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index b4065ad..69fe64a 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -156,6 +156,7 @@ struct xsm_operations {
                        struct domain *f, uint32_t flags);
     int (*mmuext_op) (struct domain *d, struct domain *f);
     int (*update_va_mapping) (struct domain *d, struct domain *f, l1_pgentry_t
pte);
+    int (*priv_mapping) (struct domain *d, struct domain *t);
     int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
     int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
*bind);
     int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t
allow);
@@ -594,6 +595,11 @@ static inline int xsm_update_va_mapping(xsm_default_t def,
struct domain *d, str
     return xsm_ops->update_va_mapping(d, f, pte);
 }
 
+static inline int xsm_priv_mapping(xsm_default_t def, struct domain *d, struct
domain *t)
+{
+    return xsm_ops->priv_mapping(d, t);
+}
+
 static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
                                                 struct xen_domctl_bind_pt_irq
*bind)
 {
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index f769642..3d84e73 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -126,6 +126,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, mmu_update);
     set_to_dummy_if_null(ops, mmuext_op);
     set_to_dummy_if_null(ops, update_va_mapping);
+    set_to_dummy_if_null(ops, priv_mapping);
     set_to_dummy_if_null(ops, bind_pt_irq);
     set_to_dummy_if_null(ops, unbind_pt_irq);
     set_to_dummy_if_null(ops, ioport_permission);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78f0b92..809e0f9 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1354,6 +1354,11 @@ static int flask_update_va_mapping(struct domain *d,
struct domain *f,
     return domain_has_perm(d, f, SECCLASS_MMU, map_perms);
 }
 
+static int flask_priv_mapping(struct domain *d, struct domain *t)
+{
+    return domain_has_perm(d, t, SECCLASS_MMU, MMU__TARGET_HACK);
+}
+
 static int flask_get_device_group(uint32_t machine_bdf)
 {
     u32 rsid;
@@ -1545,6 +1550,7 @@ static struct xsm_operations flask_ops = {
     .mmu_update = flask_mmu_update,
     .mmuext_op = flask_mmuext_op,
     .update_va_mapping = flask_update_va_mapping,
+    .priv_mapping = flask_priv_mapping,
     .get_device_group = flask_get_device_group,
     .test_assign_device = flask_test_assign_device,
     .assign_device = flask_assign_device,
diff --git a/xen/xsm/flask/policy/access_vectors
b/xen/xsm/flask/policy/access_vectors
index 176235c..63ee4dd 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -330,6 +330,9 @@ class mmu
 #  source = domain making the hypercall
 #  target = domain whose pages are being exchanged
     exchange
+# Allow a privileged domain to install a map of a page it does not own.  Used
+# for stub domain device models with the PV framebuffer.
+    target_hack
 }
 
 # control of the paging_domctl split by subop
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 5/9] xen/arch/arm: remove rcu_lock_target_domain_by_id

This function has been replaced with rcu_lock_domain_by_any_id and an
XSM check.  Two callers already had an XSM check; add a check to the
third.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/arch/arm/mm.c       | 23 +++++++++++++++--------
 xen/common/domain.c     | 34 ----------------------------------
 xen/include/xen/sched.h | 14 --------------
 xen/include/xsm/dummy.h |  8 ++++++++
 xen/include/xsm/xsm.h   | 11 +++++++++++
 xen/xsm/dummy.c         |  3 +++
 xen/xsm/flask/hooks.c   | 10 ++++++++++
 7 files changed, 47 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ba3140d..35cd1c9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -624,9 +624,16 @@ static int xenmem_add_to_physmap_one(
     {
         paddr_t maddr;
         struct domain *od;
-        rc = rcu_lock_target_domain_by_id(foreign_domid, &od);
-        if ( rc < 0 )
+        od = rcu_lock_domain_by_any_id(foreign_domid);
+        if ( od == NULL )
+            return -ESRCH;
+
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        if ( rc )
+        {
+            rcu_unlock_domain(od);
             return rc;
+        }
 
         maddr = p2m_lookup(od, idx << PAGE_SHIFT);
         if ( maddr == INVALID_PADDR )
@@ -718,9 +725,9 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void)
arg)
         if ( xatp.space == XENMAPSPACE_gmfn_foreign )
             return -EINVAL;
 
-        rc = rcu_lock_target_domain_by_id(xatp.domid, &d);
-        if ( rc != 0 )
-            return rc;
+        d = rcu_lock_domain_by_any_id(xatp.domid);
+        if ( d == NULL )
+            return -ESRCH;
 
         rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
         if ( rc )
@@ -749,9 +756,9 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void)
arg)
         if ( xatpr.space == XENMAPSPACE_gmfn_range )
             return -EINVAL;
 
-        rc = rcu_lock_target_domain_by_id(xatpr.domid, &d);
-        if ( rc != 0 )
-            return rc;
+        d = rcu_lock_domain_by_any_id(xatpr.domid);
+        if ( d == NULL )
+            return -ESRCH;
 
         rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
         if ( rc )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 590548e..ce6747c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -429,40 +429,6 @@ struct domain *rcu_lock_domain_by_any_id(domid_t dom)
     return rcu_lock_domain_by_id(dom);
 }
 
-int rcu_lock_target_domain_by_id(domid_t dom, struct domain **d)
-{
-    if ( dom == DOMID_SELF )
-    {
-        *d = rcu_lock_current_domain();
-        return 0;
-    }
-
-    if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
-        return -ESRCH;
-
-    if ( !IS_PRIV_FOR(current->domain, *d) )
-    {
-        rcu_unlock_domain(*d);
-        return -EPERM;
-    }
-
-    return 0;
-}
-
-int rcu_lock_remote_target_domain_by_id(domid_t dom, struct domain **d)
-{
-    if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
-        return -ESRCH;
-
-    if ( (*d == current->domain) || !IS_PRIV_FOR(current->domain, *d) )
-    {
-        rcu_unlock_domain(*d);
-        return -EPERM;
-    }
-
-    return 0;
-}
-
 int rcu_lock_remote_domain_by_id(domid_t dom, struct domain **d)
 {
     if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index d15d567..723885c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -471,20 +471,6 @@ struct domain *rcu_lock_domain_by_id(domid_t dom);
 struct domain *rcu_lock_domain_by_any_id(domid_t dom);
 
 /*
- * As above function, but accounts for current domain context:
- *  - Translates target DOMID_SELF into caller''s domain id; and
- *  - Checks that caller has permission to act on the target domain.
- */
-int rcu_lock_target_domain_by_id(domid_t dom, struct domain **d);
-
-/*
- * As rcu_lock_target_domain_by_id(), but will fail EPERM rather than resolve
- * to local domain. Successful return always resolves to a remote domain that
- * the local domain is privileged to control.
- */
-int rcu_lock_remote_target_domain_by_id(domid_t dom, struct domain **d);
-
-/*
  * As rcu_lock_domain_by_id(), but will fail EPERM or ESRCH rather than resolve
  * to local domain.
  */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 9bfe596..3912bd9 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -616,4 +616,12 @@ static XSM_INLINE int xsm_ioport_mapping(XSM_DEFAULT_ARG
struct domain *d, uint3
     return xsm_default_action(action, current->domain, d);
 }
 
+#endif /* CONFIG_X86 */
+
+#ifdef CONFIG_ARM
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d,
struct domain *t)
+{
+    XSM_ASSERT_ACTION(XSM_TARGET);
+    return xsm_default_action(action, d, t);
+}
 #endif
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 69fe64a..58a4fbb 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -162,6 +162,9 @@ struct xsm_operations {
     int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t
allow);
     int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t
allow);
 #endif
+#ifdef CONFIG_ARM
+    int (*map_gmfn_foreign) (struct domain *d, struct domain *t);
+#endif
 };
 
 #ifdef XSM_ENABLE
@@ -622,6 +625,14 @@ static inline int xsm_ioport_mapping (xsm_default_t def,
struct domain *d, uint3
     return xsm_ops->ioport_mapping(d, s, e, allow);
 }
 #endif /* CONFIG_X86 */
+
+#ifdef CONFIG_ARM
+static inline int xsm_map_gmfn_foreign (struct domain *d, struct domain *t)
+{
+    return xsm_ops->map_gmfn_foreign(d, t);
+}
+#endif /* CONFIG_ARM */
+
 #endif /* XSM_NO_WRAPPERS */
 
 extern int xsm_init(unsigned long *module_map, const multiboot_info_t *mbi,
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 3d84e73..937761f 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -132,4 +132,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, ioport_permission);
     set_to_dummy_if_null(ops, ioport_mapping);
 #endif
+#ifdef CONFIG_ARM
+    set_to_dummy_if_null(ops, map_gmfn_foreign);
+#endif
 }
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 809e0f9..6512c22 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1452,6 +1452,13 @@ static int flask_unbind_pt_irq (struct domain *d, struct
xen_domctl_bind_pt_irq
 {
     return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
 }
+#endif /* CONFIG_X86 */
+
+#ifdef CONFIG_ARM
+static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+{
+    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+}
 #endif
 
 long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
@@ -1560,6 +1567,9 @@ static struct xsm_operations flask_ops = {
     .ioport_permission = flask_ioport_permission,
     .ioport_mapping = flask_ioport_mapping,
 #endif
+#ifdef CONFIG_ARM
+    .map_gmfn_foreign = flask_map_gmfn_foreign,
+#endif
 };
 
 static __init int flask_init(void)
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 6/9] xen: rename IS_PRIV to is_hardware_domain

Since the remaining uses of IS_PRIV are actually concerned with the
domain having control of the hardware (i.e. being the initial domain),
clarify this by renaming IS_PRIV to is_hardware_domain.  This also
removes IS_PRIV_FOR since the only remaining user was xsm/dummy.h.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domctl.c   |  6 +++---
 xen/arch/x86/msi.c      |  2 +-
 xen/arch/x86/physdev.c  |  4 ++--
 xen/arch/x86/traps.c    | 12 ++++++------
 xen/include/xen/sched.h |  9 +++++++--
 xen/include/xsm/dummy.h | 32 ++++++++++++++++++++------------
 6 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 327a792..ed51106 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -662,7 +662,7 @@ long arch_do_domctl(
                     while ( i-- )
                         clear_mmio_p2m_entry(d, gfn + i);
                     if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1)
&&
-                         IS_PRIV(current->domain) )
+                         is_hardware_domain(current->domain) )
                         printk(XENLOG_ERR
                                "memory_map: failed to deny dom%d access to
[%lx,%lx]\n",
                                d->domain_id, mfn, mfn + nr_mfns - 1);
@@ -681,7 +681,7 @@ long arch_do_domctl(
             ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
             if ( !ret && add )
                 ret = -EIO;
-            if ( ret && IS_PRIV(current->domain) )
+            if ( ret && is_hardware_domain(current->domain) )
                 printk(XENLOG_ERR
                        "memory_map: error %ld %s dom%d access to
[%lx,%lx]\n",
                        ret, add ? "removing" : "denying",
d->domain_id,
@@ -768,7 +768,7 @@ long arch_do_domctl(
                     break;
                 }
             ret = ioports_deny_access(d, fmp, fmp + np - 1);
-            if ( ret && IS_PRIV(current->domain) )
+            if ( ret && is_hardware_domain(current->domain) )
                 printk(XENLOG_ERR
                        "ioport_map: error %ld denying dom%d access to
[%x,%x]\n",
                        ret, d->domain_id, fmp, fmp + np - 1);
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 6cc8f7a..4059b32 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -810,7 +810,7 @@ static int msix_capability_init(struct pci_dev *dev,
                         break;
             if ( d )
             {
-                if ( !IS_PRIV(d) && dev->msix_warned !=
d->domain_id )
+                if ( !is_hardware_domain(d) && dev->msix_warned !=
d->domain_id )
                 {
                     dev->msix_warned = d->domain_id;
                     printk(XENLOG_ERR
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 876ac9d..226905d 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -128,7 +128,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index,
int *pirq_p,
         irq = domain_pirq_to_irq(current->domain, *index);
         if ( irq <= 0 )
         {
-            if ( IS_PRIV(current->domain) )
+            if ( is_hardware_domain(current->domain) )
                 irq = *index;
             else {
                 dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect
irq!\n",
@@ -691,7 +691,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
arg)
     case PHYSDEVOP_dbgp_op: {
         struct physdev_dbgp_op op;
 
-        if ( !IS_PRIV(v->domain) )
+        if ( !is_hardware_domain(v->domain) )
             ret = -EPERM;
         else if ( copy_from_guest(&op, arg, 1) )
             ret = -EFAULT;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index d36eddd..80d7892 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1303,7 +1303,7 @@ static int fixup_page_fault(unsigned long addr, struct
cpu_user_regs *regs)
              ptwr_do_page_fault(v, addr, regs) )
             return EXCRET_fault_fixed;
 
-        if ( IS_PRIV(d) && (regs->error_code &
PFEC_page_present) &&
+        if ( is_hardware_domain(d) && (regs->error_code &
PFEC_page_present) &&
              mmio_ro_do_page_fault(v, addr, regs) )
             return EXCRET_fault_fixed;
     }
@@ -1623,7 +1623,7 @@ static int pci_cfg_ok(struct domain *d, int write, int
size)
 {
     uint32_t machine_bdf;
     uint16_t start, end;
-    if (!IS_PRIV(d))
+    if (!is_hardware_domain(d))
         return 0;
 
     machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
@@ -2404,7 +2404,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
             if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
                  boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 )
                 goto fail;
-            if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+            if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
                 break;
             if ( (rdmsr_safe(MSR_AMD64_NB_CFG, val) != 0) ||
                  (eax != (uint32_t)val) ||
@@ -2417,7 +2417,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
             if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
                  boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 )
                 goto fail;
-            if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+            if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
                 break;
             if ( (rdmsr_safe(MSR_FAM10H_MMIO_CONF_BASE, val) != 0) )
                 goto fail;
@@ -2437,7 +2437,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
         case MSR_IA32_UCODE_REV:
             if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
                 goto fail;
-            if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+            if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
                 break;
             if ( rdmsr_safe(regs->ecx, val) )
                 goto fail;
@@ -2473,7 +2473,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
         case MSR_IA32_ENERGY_PERF_BIAS:
             if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
                 goto fail;
-            if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+            if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
                 break;
             if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
                 goto fail;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 723885c..bd1c7dc 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -705,8 +705,13 @@ uint64_t get_cpu_idle_time(unsigned int cpu);
 void watchdog_domain_init(struct domain *d);
 void watchdog_domain_destroy(struct domain *d);
 
-#define IS_PRIV(_d) ((_d)->is_privileged)
-#define IS_PRIV_FOR(_d, _t) (IS_PRIV(_d) || ((_d)->target &&
(_d)->target == (_t)))
+/* 
+ * Use this check when the following are both true:
+ *  - Using this feature or interface requires full access to the hardware
+ *    (that is, this is would not be suitable for a driver domain)
+ *  - There is never a reason to deny dom0 access to this
+ */
+#define is_hardware_domain(_d) ((_d)->is_privileged)
 
 #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
 
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 3912bd9..a872056 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -60,17 +60,23 @@ static always_inline int xsm_default_action(
     case XSM_HOOK:
         return 0;
     case XSM_DM_PRIV:
-        if ( !IS_PRIV_FOR(src, target) )
-            return -EPERM;
-        return 0;
+        if ( src->is_privileged )
+            return 0;
+        if ( target && src->target == target )
+            return 0;
+        return -EPERM;
     case XSM_TARGET:
-        if ( src != target && !IS_PRIV_FOR(src, target) )
-            return -EPERM;
-        return 0;
+        if ( src == target )
+            return 0;
+        if ( src->is_privileged )
+            return 0;
+        if ( target && src->target == target )
+            return 0;
+        return -EPERM;
     case XSM_PRIV:
-        if ( !IS_PRIV(src) )
-            return -EPERM;
-        return 0;
+        if ( src->is_privileged )
+            return 0;
+        return -EPERM;
     default:
         LINKER_BUG_ON(1);
         return -EPERM;
@@ -567,10 +573,12 @@ static XSM_INLINE int
xsm_domain_memory_map(XSM_DEFAULT_ARG struct domain *d)
 static XSM_INLINE int xsm_mmu_update(XSM_DEFAULT_ARG struct domain *d, struct
domain *t,
                                      struct domain *f, uint32_t flags)
 {
+    int rc;
     XSM_ASSERT_ACTION(XSM_TARGET);
-    if ( t && d != t && !IS_PRIV_FOR(d, t) )
-        return -EPERM;
-    return xsm_default_action(action, d, f);
+    rc = xsm_default_action(action, d, f);
+    if ( t && !rc )
+        rc = xsm_default_action(action, d, t);
+    return rc;
 }
 
 static XSM_INLINE int xsm_mmuext_op(XSM_DEFAULT_ARG struct domain *d, struct
domain *f)
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 7/9] xen: use domid check in is_hardware_domain

Instead of checking is_privileged to determine if a domain should
control the hardware, check that the domain_id is equal to zero (which
is currently the only domain for which is_privileged is true).  This
allows other places where domain_id is checked for zero to be replaced
with is_hardware_domain.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/cpupool.c    |  9 ++++++---
 xen/common/domain.c     | 10 +++++-----
 xen/include/xen/sched.h |  2 +-
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 10b10f8..0ecbbda 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -546,13 +546,16 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
     {
         struct domain *d;
 
-        ret = -EINVAL;
-        if ( op->domid == 0 )
-            break;
         ret = -ESRCH;
         d = rcu_lock_domain_by_id(op->domid);
         if ( d == NULL )
             break;
+        if ( is_hardware_domain(d) )
+        {
+            ret = -EINVAL;
+            rcu_unlock_domain(d);
+            break;
+        }
         if ( d->cpupool == NULL )
         {
             ret = -EINVAL;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ce6747c..02b631f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -235,7 +235,7 @@ struct domain *domain_create(
     if ( domcr_flags & DOMCRF_hvm )
         d->is_hvm = 1;
 
-    if ( domid == 0 )
+    if ( is_hardware_domain(d) )
     {
         d->is_pinned = opt_dom0_vcpus_pin;
         d->disable_migrate = 1;
@@ -260,10 +260,10 @@ struct domain *domain_create(
         d->is_paused_by_controller = 1;
         atomic_inc(&d->pause_count);
 
-        if ( domid )
-            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
-        else
+        if ( is_hardware_domain(d) )
             d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
+        else
+            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
         if ( d->nr_pirqs > nr_irqs )
             d->nr_pirqs = nr_irqs;
 
@@ -543,7 +543,7 @@ void domain_shutdown(struct domain *d, u8 reason)
         d->shutdown_code = reason;
     reason = d->shutdown_code;
 
-    if ( d->domain_id == 0 )
+    if ( is_hardware_domain(d) )
         dom0_shutdown(reason);
 
     if ( d->is_shutting_down )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index bd1c7dc..45a5d7d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -711,7 +711,7 @@ void watchdog_domain_destroy(struct domain *d);
  *    (that is, this is would not be suitable for a driver domain)
  *  - There is never a reason to deny dom0 access to this
  */
-#define is_hardware_domain(_d) ((_d)->is_privileged)
+#define is_hardware_domain(_d) ((_d)->domain_id == 0)
 
 #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
 
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

This makes checks for dom0 more explicit than checking the domain ID.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domain.c    | 2 +-
 xen/arch/x86/hvm/i8254.c | 2 +-
 xen/arch/x86/time.c      | 4 ++--
 xen/arch/x86/traps.c     | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0d7a40c..b4b0f3f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
         }
 
         set_cpuid_faulting(!is_hvm_vcpu(next) &&
-                           (next->domain->domain_id != 0));
+                           !is_hardware_domain(next->domain));
     }
 
     context_saved(prev);
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index c0d6bc2..add61e3 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write)
         .data = data
     };
 
-    if ( (current->domain->domain_id == 0) &&
dom0_pit_access(&ioreq) )
+    if ( is_hardware_domain(current->domain) &&
dom0_pit_access(&ioreq) )
     {
         /* nothing to do */;
     }
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 6e94847..72ca176 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d,
                   uint32_t tsc_mode, uint64_t elapsed_nsec,
                   uint32_t gtsc_khz, uint32_t incarnation)
 {
-    if ( is_idle_domain(d) || (d->domain_id == 0) )
+    if ( is_idle_domain(d) || is_hardware_domain(d) )
     {
         d->arch.vtsc = 0;
         return;
@@ -2005,7 +2005,7 @@ static void dump_softtsc(unsigned char key)
                "warp=%lu (count=%lu)\n", tsc_max_warp,
tsc_check_count);
     for_each_domain ( d )
     {
-        if ( d->domain_id == 0 && d->arch.tsc_mode ==
TSC_MODE_DEFAULT )
+        if ( is_hardware_domain(d) && d->arch.tsc_mode ==
TSC_MODE_DEFAULT )
             continue;
         printk("dom%u%s: mode=%d",d->domain_id,
                 is_hvm_domain(d) ? "(hvm)" : "",
d->arch.tsc_mode);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 80d7892..9e4dd75 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
     c = regs->ecx;
     d = regs->edx;
 
-    if ( current->domain->domain_id != 0 )
+    if ( !is_hardware_domain(current->domain) )
     {
         unsigned int cpuid_leaf = a, sub_leaf = c;
 
@@ -1828,7 +1828,7 @@ static inline uint64_t guest_misc_enable(uint64_t val)
 static int is_cpufreq_controller(struct domain *d)
 {
     return ((cpufreq_controller == FREQCTL_dom0_kernel) &&
-            (d->domain_id == 0));
+            is_hardware_domain(d));
 }
 
 #include "x86_64/mmconfig.h"
-- 
1.8.1.4

Daniel De Graaf

2013-Apr-11 20:13 UTC

head link

[PATCH 9/9] IOMMU: use is_hardware_domain instead of domid == 0

This makes checks for dom0 more explicit than checking the domain ID.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Jacob Shin <jacob.shin@amd.com>
Cc: Xiantao Zhang <xiantao.zhang@intel.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +-
 xen/drivers/passthrough/vtd/iommu.c         | 8 ++++----
 xen/drivers/passthrough/vtd/x86/vtd.c       | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 60696d7..687f1a3 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -122,7 +122,7 @@ static void amd_iommu_setup_domain_device(
 
     BUG_ON( !hd->root_table || !hd->paging_mode ||
!iommu->dev_table.buffer );
 
-    if ( iommu_passthrough && (domain->domain_id == 0) )
+    if ( iommu_passthrough && is_hardware_domain(domain) )
         valid = 0;
 
     if ( ats_enabled )
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 0fc10de..8d5c43d 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1336,7 +1336,7 @@ int domain_context_mapping_one(
         return res;
     }
 
-    if ( iommu_passthrough && (domain->domain_id == 0) )
+    if ( iommu_passthrough && is_hardware_domain(domain) )
     {
         context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
         agaw = level_to_agaw(iommu->nr_pt_levels);
@@ -1710,7 +1710,7 @@ static int intel_iommu_map_page(
         return 0;
 
     /* do nothing if dom0 and iommu supports pass thru */
-    if ( iommu_passthrough && (d->domain_id == 0) )
+    if ( iommu_passthrough && is_hardware_domain(d) )
         return 0;
 
     spin_lock(&hd->mapping_lock);
@@ -1754,7 +1754,7 @@ static int intel_iommu_map_page(
 static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
 {
     /* Do nothing if dom0 and iommu supports pass thru. */
-    if ( iommu_passthrough && (d->domain_id == 0) )
+    if ( iommu_passthrough && is_hardware_domain(d) )
         return 0;
 
     dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
@@ -1927,7 +1927,7 @@ static int intel_iommu_remove_device(u8 devfn, struct
pci_dev *pdev)
     /* If the device belongs to dom0, and it has RMRR, don''t remove it
      * from dom0, because BIOS may use RMRR at booting time.
      */
-    if ( pdev->domain->domain_id == 0 )
+    if ( is_hardware_domain(pdev->domain) )
     {
         for_each_rmrr_device ( rmrr, bdf, i )
         {
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
b/xen/drivers/passthrough/vtd/x86/vtd.c
index 875b033..eb9e52a 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -117,7 +117,7 @@ void __init iommu_set_dom0_mapping(struct domain *d)
 {
     unsigned long i, j, tmp, top;
 
-    BUG_ON(d->domain_id != 0);
+    BUG_ON(!is_hardware_domain(d));
 
     top = max(max_pdx, pfn_to_pdx(0xffffffffUL >> PAGE_SHIFT) + 1);
 
-- 
1.8.1.4

Jan Beulich

2013-Apr-12 08:00 UTC

head link

Re: [PATCH 7/9] xen: use domid check in is_hardware_domain

>>> On 11.04.13 at 22:13, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
> Instead of checking is_privileged to determine if a domain should
> control the hardware, check that the domain_id is equal to zero (which
> is currently the only domain for which is_privileged is true).  This
> allows other places where domain_id is checked for zero to be replaced
> with is_hardware_domain.
Isn''t this moving in the wrong direction? I.e. wouldn''t you
rather
want to see the domid == 0 checks go away, rather than replacing
the ->is_privileged one in is_hardware_domain()?
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -546,13 +546,16 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op
*op)
>      {
>          struct domain *d;
>  
> -        ret = -EINVAL;
> -        if ( op->domid == 0 )
> -            break;
>          ret = -ESRCH;
>          d = rcu_lock_domain_by_id(op->domid);
>          if ( d == NULL )
>              break;
> +        if ( is_hardware_domain(d) )
Here this is surely wrong - the operation has nothing to do with
hardware control. This would need something like is_control_domain().

Which puts under question the earlier changes in this series: Wouldn''t
you be better of having is_control_domain() resolving to
->is_privileged and is_hardware_domain() resolving to domid == 0
(for the time being), and use the two according to the respective
contexts?
> +        {
> +            ret = -EINVAL;
> +            rcu_unlock_domain(d);
> +            break;
> +        }
>          if ( d->cpupool == NULL )
>          {
>              ret = -EINVAL;
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -235,7 +235,7 @@ struct domain *domain_create(
>      if ( domcr_flags & DOMCRF_hvm )
>          d->is_hvm = 1;
>  
> -    if ( domid == 0 )
> +    if ( is_hardware_domain(d) )
And this one isn''t hardware related either. While the subsequent ones
both are.

Jan

Jan Beulich

2013-Apr-12 08:07 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

>>> On 11.04.13 at 22:13, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct vcpu
*next)
>          }
>  
>          set_cpuid_faulting(!is_hvm_vcpu(next) &&
> -                           (next->domain->domain_id != 0));
> +                           !is_hardware_domain(next->domain));
This one is highly questionable. Following the comments on the
previous patch, I''d think is_control_domain() here is more
appropriate, but maybe it would really need to become the or
of both? The question really is which domains we specifically
don''t want CPUID faulting for.
> --- a/xen/arch/x86/hvm/i8254.c
> +++ b/xen/arch/x86/hvm/i8254.c
> @@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write)
>          .data = data
>      };
>  
> -    if ( (current->domain->domain_id == 0) &&
dom0_pit_access(&ioreq) )
> +    if ( is_hardware_domain(current->domain) &&
dom0_pit_access(&ioreq) )
And this one is now becoming inconsistent in itself: You remove the
explicit 0 on the left side, but leave the "dom0" in place on the
right
side. If we drop the association with domid == 0 being the special
one, then all uses of "dom0" would logically also need to go away.
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d,
>                    uint32_t tsc_mode, uint64_t elapsed_nsec,
>                    uint32_t gtsc_khz, uint32_t incarnation)
>  {
> -    if ( is_idle_domain(d) || (d->domain_id == 0) )
> +    if ( is_idle_domain(d) || is_hardware_domain(d) )
Just like with the CPUID faulting, it is unclear (without a proper
model described end established first) which domains we really
want to special case here. Hence, rather than papering over the
issue, I''d prefer keeping the explicit domid check in place until
such a model can get established.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
>      c = regs->ecx;
>      d = regs->edx;
>  
> -    if ( current->domain->domain_id != 0 )
> +    if ( !is_hardware_domain(current->domain) )
Again, if anything this is more likely is_control_domain() or the or
of both.

Jan

Jan Beulich

2013-Apr-12 08:17 UTC

head link

Re: [PATCH v2 0/9] Rename/remove IS_PRIV

>>> On 11.04.13 at 22:13, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
Daniel,

in the context of your work on XSM over the past several months,
wouldn''t you want to become listed as the formal maintainer for
the respective code? Assuming that such a proposal would be
relatively uncontroversial, that would simplify patch application
quite a bit, because then it wouldn''t be necessary to have
Keir''s
ack on every patch that touches XSM bits.

If you''re up to this, then you may want to insert a patch at the
beginning of this series (or send a standalone one) adding a
respective entry to ./MAINTAINERS.

Jan

Daniel De Graaf

2013-Apr-12 14:11 UTC

head link

Re: [PATCH 7/9] xen: use domid check in is_hardware_domain

On 04/12/2013 04:00 AM, Jan Beulich wrote:>>>> On 11.04.13 at 22:13, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>> Instead of checking is_privileged to determine if a domain should
>> control the hardware, check that the domain_id is equal to zero (which
>> is currently the only domain for which is_privileged is true).  This
>> allows other places where domain_id is checked for zero to be replaced
>> with is_hardware_domain.
>
> Isn''t this moving in the wrong direction? I.e. wouldn''t
you rather
> want to see the domid == 0 checks go away, rather than replacing
> the ->is_privileged one in is_hardware_domain()?
I don''t have a strong opinion on the difference between (domid == 0)
and
->is_privileged; I chose domid == 0 in part because more of the changes
I was making were already comparing domid, and in part because it makes
changing "0" to a variable hardware domain ID simpler. (I have working
patches that do this, but they aren''t yet ready for upstreaming).

Either way it is done, some of the code paths controlled by this check
can only be allowed to one Linux domain at a time - the PCI accesses in
traps.c are the most obvious example of this, since multiple Linux domains
will all try to access the PCI config space and step on one another.
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -546,13 +546,16 @@ int cpupool_do_sysctl(struct
xen_sysctl_cpupool_op *op)
>>       {
>>           struct domain *d;
>>
>> -        ret = -EINVAL;
>> -        if ( op->domid == 0 )
>> -            break;
>>           ret = -ESRCH;
>>           d = rcu_lock_domain_by_id(op->domid);
>>           if ( d == NULL )
>>               break;
>> +        if ( is_hardware_domain(d) )
>
> Here this is surely wrong - the operation has nothing to do with
> hardware control. This would need something like is_control_domain().
>
> Which puts under question the earlier changes in this series:
Wouldn''t
> you be better of having is_control_domain() resolving to
> ->is_privileged and is_hardware_domain() resolving to domid == 0
> (for the time being), and use the two according to the respective
> contexts?
That is what I was aiming for, but with is_control_domain not needed due to
every occurrence being covered by an XSM hook. I assumed that the reason
this sysctl was not allowed on dom0 was to avoid moving the possibly-pinned
hardware domain''s CPUs; should this instead be
rcu_lock_remote_domain_by_id
to avoid a domain moving itself, or is there another reason why a control
domain''s cpupool should not be changed?
>> +        {
>> +            ret = -EINVAL;
>> +            rcu_unlock_domain(d);
>> +            break;
>> +        }
>>           if ( d->cpupool == NULL )
>>           {
>>               ret = -EINVAL;
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -235,7 +235,7 @@ struct domain *domain_create(
>>       if ( domcr_flags & DOMCRF_hvm )
>>           d->is_hvm = 1;
>>
>> -    if ( domid == 0 )
>> +    if ( is_hardware_domain(d) )
>
> And this one isn''t hardware related either. While the subsequent
ones
> both are.
>
> Jan
I had assumed this one was hardware related: we can''t migrate the
hardware
domain and pinning its vcpus makes sense for things like MSRs or frequency
control by that domain. I don''t see any reason to pin the vcpus of a
control
domain, and while I doubt it is useful to migrate it, there is no reason to
forbid migration unlike the hardware domain.


-- 
Daniel De Graaf
National Security Agency

Daniel De Graaf

2013-Apr-12 14:24 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

On 04/12/2013 04:07 AM, Jan Beulich wrote:>>>> On 11.04.13 at 22:13, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct
vcpu *next)
>>           }
>>
>>           set_cpuid_faulting(!is_hvm_vcpu(next) &&
>> -                           (next->domain->domain_id != 0));
>> +                           !is_hardware_domain(next->domain));
>
> This one is highly questionable. Following the comments on the
> previous patch, I''d think is_control_domain() here is more
> appropriate, but maybe it would really need to become the or
> of both? The question really is which domains we specifically
> don''t want CPUID faulting for.
My impression was that this would be the hardware domain since it is
the one that would be parsing ACPI tables and similar things that need
CPUID to work. The control domain would just be involved in creating
and managing domains, which doesn''t need anything out of CPUID that a
normal PV domain would need.
>> --- a/xen/arch/x86/hvm/i8254.c
>> +++ b/xen/arch/x86/hvm/i8254.c
>> @@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write)
>>           .data = data
>>       };
>>
>> -    if ( (current->domain->domain_id == 0) &&
dom0_pit_access(&ioreq) )
>> +    if ( is_hardware_domain(current->domain) &&
dom0_pit_access(&ioreq) )
>
> And this one is now becoming inconsistent in itself: You remove the
> explicit 0 on the left side, but leave the "dom0" in place on the
right
> side. If we drop the association with domid == 0 being the special
> one, then all uses of "dom0" would logically also need to go
away.
I didn''t want to take on the mass function rename that this would
imply. I
had considered naming the function "is_dom0_domain" but this seemed
less
clear given that I was trying to isolate hardware accesses.
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d,
>>                     uint32_t tsc_mode, uint64_t elapsed_nsec,
>>                     uint32_t gtsc_khz, uint32_t incarnation)
>>   {
>> -    if ( is_idle_domain(d) || (d->domain_id == 0) )
>> +    if ( is_idle_domain(d) || is_hardware_domain(d) )
>
> Just like with the CPUID faulting, it is unclear (without a proper
> model described end established first) which domains we really
> want to special case here. Hence, rather than papering over the
> issue, I''d prefer keeping the explicit domid check in place until
> such a model can get established.
OK; my logic here was the same as for CPUID faulting.
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
>>       c = regs->ecx;
>>       d = regs->edx;
>>
>> -    if ( current->domain->domain_id != 0 )
>> +    if ( !is_hardware_domain(current->domain) )
>
> Again, if anything this is more likely is_control_domain() or the or
> of both.
>
> Jan
>
>

-- 
Daniel De Graaf
National Security Agency

Jan Beulich

2013-Apr-12 14:45 UTC

head link

Re: [PATCH 7/9] xen: use domid check in is_hardware_domain

>>> On 12.04.13 at 16:11, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
> On 04/12/2013 04:00 AM, Jan Beulich wrote:
>>>>> On 11.04.13 at 22:13, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>>> Instead of checking is_privileged to determine if a domain should
>>> control the hardware, check that the domain_id is equal to zero
(which
>>> is currently the only domain for which is_privileged is true). 
This
>>> allows other places where domain_id is checked for zero to be
replaced
>>> with is_hardware_domain.
>>
>> Isn''t this moving in the wrong direction? I.e.
wouldn''t you rather
>> want to see the domid == 0 checks go away, rather than replacing
>> the ->is_privileged one in is_hardware_domain()?
> 
> I don''t have a strong opinion on the difference between (domid ==
0) and
> ->is_privileged; I chose domid == 0 in part because more of the changes
> I was making were already comparing domid, and in part because it makes
> changing "0" to a variable hardware domain ID simpler. (I have
working
> patches that do this, but they aren''t yet ready for upstreaming).
> 
> Either way it is done, some of the code paths controlled by this check
> can only be allowed to one Linux domain at a time - the PCI accesses in
> traps.c are the most obvious example of this, since multiple Linux domains
> will all try to access the PCI config space and step on one another.
Right. But my point was that a change like this should make things
more clear and easier to change in the future (ideally without
having to touch all the same places again that you touch now). And
I don''t think the way it is done either of this is being achieved.
>>> --- a/xen/common/cpupool.c
>>> +++ b/xen/common/cpupool.c
>>> @@ -546,13 +546,16 @@ int cpupool_do_sysctl(struct
xen_sysctl_cpupool_op *op)
>>>       {
>>>           struct domain *d;
>>>
>>> -        ret = -EINVAL;
>>> -        if ( op->domid == 0 )
>>> -            break;
>>>           ret = -ESRCH;
>>>           d = rcu_lock_domain_by_id(op->domid);
>>>           if ( d == NULL )
>>>               break;
>>> +        if ( is_hardware_domain(d) )
>>
>> Here this is surely wrong - the operation has nothing to do with
>> hardware control. This would need something like is_control_domain().
>>
>> Which puts under question the earlier changes in this series:
Wouldn''t
>> you be better of having is_control_domain() resolving to
>> ->is_privileged and is_hardware_domain() resolving to domid == 0
>> (for the time being), and use the two according to the respective
>> contexts?
> 
> That is what I was aiming for, but with is_control_domain not needed due to
> every occurrence being covered by an XSM hook. I assumed that the reason
> this sysctl was not allowed on dom0 was to avoid moving the possibly-pinned
> hardware domain''s CPUs; should this instead be
rcu_lock_remote_domain_by_id
> to avoid a domain moving itself, or is there another reason why a control
> domain''s cpupool should not be changed?
Yes, afaiu the main point here is to disallow a domain to affect
itself (due to deadlock potential). You may want to double check
with Juergen though.
>>> +        {
>>> +            ret = -EINVAL;
>>> +            rcu_unlock_domain(d);
>>> +            break;
>>> +        }
>>>           if ( d->cpupool == NULL )
>>>           {
>>>               ret = -EINVAL;
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -235,7 +235,7 @@ struct domain *domain_create(
>>>       if ( domcr_flags & DOMCRF_hvm )
>>>           d->is_hvm = 1;
>>>
>>> -    if ( domid == 0 )
>>> +    if ( is_hardware_domain(d) )
>>
>> And this one isn''t hardware related either. While the
subsequent ones
>> both are.
> 
> I had assumed this one was hardware related: we can''t migrate the
hardware
> domain and pinning its vcpus makes sense for things like MSRs or frequency
> control by that domain. I don''t see any reason to pin the vcpus of
a control
> domain, and while I doubt it is useful to migrate it, there is no reason to
> forbid migration unlike the hardware domain.
Hmm, right, looks light I didn''t pay enough attention to the patch
context, and judged merely by the function you change.

Jan

Jan Beulich

2013-Apr-12 14:49 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

>>> On 12.04.13 at 16:24, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
> On 04/12/2013 04:07 AM, Jan Beulich wrote:
>>>>> On 11.04.13 at 22:13, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct
vcpu
> *next)
>>>           }
>>>
>>>           set_cpuid_faulting(!is_hvm_vcpu(next) &&
>>> -                           (next->domain->domain_id != 0));
>>> +                           !is_hardware_domain(next->domain));
>>
>> This one is highly questionable. Following the comments on the
>> previous patch, I''d think is_control_domain() here is more
>> appropriate, but maybe it would really need to become the or
>> of both? The question really is which domains we specifically
>> don''t want CPUID faulting for.
> 
> My impression was that this would be the hardware domain since it is
> the one that would be parsing ACPI tables and similar things that need
> CPUID to work. The control domain would just be involved in creating
> and managing domains, which doesn''t need anything out of CPUID
that a
> normal PV domain would need.
Parsing ACPI tables is unrelated to CPUID, as is most other
hardware management. The only reason I could see to override
this would be to have some domains know the true capabilities of
the processor. And that would be the control domain(s), in order
to be able to sensibly set up CPUID emulation for the guests.
>>> --- a/xen/arch/x86/hvm/i8254.c
>>> +++ b/xen/arch/x86/hvm/i8254.c
>>> @@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int
write)
>>>           .data = data
>>>       };
>>>
>>> -    if ( (current->domain->domain_id == 0) &&
dom0_pit_access(&ioreq) )
>>> +    if ( is_hardware_domain(current->domain) &&
dom0_pit_access(&ioreq) )
>>
>> And this one is now becoming inconsistent in itself: You remove the
>> explicit 0 on the left side, but leave the "dom0" in place on
the right
>> side. If we drop the association with domid == 0 being the special
>> one, then all uses of "dom0" would logically also need to go
away.
> 
> I didn''t want to take on the mass function rename that this would
imply. I
> had considered naming the function "is_dom0_domain" but this
seemed less
> clear given that I was trying to isolate hardware accesses.
I understood this would have been the main reason, but then we
can as well leave the domid == 0 check in place...

Jan

Keir Fraser

2013-Apr-12 15:41 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

On 12/04/2013 15:49, "Jan Beulich" <JBeulich@suse.com> wrote:
>> My impression was that this would be the hardware domain since it is
>> the one that would be parsing ACPI tables and similar things that need
>> CPUID to work. The control domain would just be involved in creating
>> and managing domains, which doesn''t need anything out of CPUID
that a
>> normal PV domain would need.
> 
> Parsing ACPI tables is unrelated to CPUID, as is most other
> hardware management. The only reason I could see to override
> this would be to have some domains know the true capabilities of
> the processor. And that would be the control domain(s), in order
> to be able to sensibly set up CPUID emulation for the guests.
Yes, it''s subtle but this would break tools/libxc/xc_cpuid_x86.c which
expects to see the real CPUID, not the virtualised one. For example, it will
look for host 64-bit capabilities this way, which would be hidden in the
virtualised CPUID space if the control domain is 32-bit.

We could get rid of this not-always-faulting-cpuid hack in the hypervisor
altogether, if we could provide a paravirtualised interface that guarantees
access to the real CPUID space, for the toolstack to use.

But anyhow, the hack is for the toolstack only, and hence is for
control_domain only.

 -- Keir

Ian Campbell

2013-Apr-12 15:49 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

On Fri, 2013-04-12 at 16:41 +0100, Keir Fraser wrote:> We could get rid of this not-always-faulting-cpuid hack in the
> hypervisor altogether, if we could provide a paravirtualised interface
> that guarantees access to the real CPUID space, for the toolstack to
> use. 
Doesn''t XCP have a patch which allows access to the non-levelled cpuid
values for the toolstacks benefit? Andy C CCd...

Andrew Cooper

2013-Apr-12 16:48 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

On 12/04/13 16:49, Ian Campbell wrote:> On Fri, 2013-04-12 at 16:41 +0100, Keir Fraser wrote:
>> We could get rid of this not-always-faulting-cpuid hack in the
>> hypervisor altogether, if we could provide a paravirtualised interface
>> that guarantees access to the real CPUID space, for the toolstack to
>> use. 
> Doesn''t XCP have a patch which allows access to the non-levelled
cpuid
> values for the toolstacks benefit? Andy C CCd...
>
>
XCP/XenServer has a specific patch which records the real CPUID values
before applying the MSRs for feature masking, which can then subsequently

This allows the toolstack to recalculate the host level parity bits
correctly whenever a host is added or removed from the pool, without
suffering the problems of getting into a diminishing subset of features
as slightly heterogeneous hardware is moved in and out of the pool.

The patch is currently in the state of "Needing to be cleaned up for
upstream" and is in my todo list (albeit quite far down given some
recent Xen crashes found during stress testing)

I can up its priority if there is a need for it.

~Andrew

Andrew Cooper

2013-Apr-12 16:50 UTC

head link

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

On 12/04/13 17:48, Andrew Cooper wrote:> On 12/04/13 16:49, Ian Campbell wrote:
>> On Fri, 2013-04-12 at 16:41 +0100, Keir Fraser wrote:
>>> We could get rid of this not-always-faulting-cpuid hack in the
>>> hypervisor altogether, if we could provide a paravirtualised
interface
>>> that guarantees access to the real CPUID space, for the toolstack
to
>>> use. 
>> Doesn''t XCP have a patch which allows access to the
non-levelled cpuid
>> values for the toolstacks benefit? Andy C CCd...
>>
>>
> XCP/XenServer has a specific patch which records the real CPUID values
> before applying the MSRs for feature masking, which can then subsequentlyqueried by the toolstack.

(Apologies - I sent mid-sentence)

~Andrew
>
> This allows the toolstack to recalculate the host level parity bits
> correctly whenever a host is added or removed from the pool, without
> suffering the problems of getting into a diminishing subset of features
> as slightly heterogeneous hardware is moved in and out of the pool.
>
> The patch is currently in the state of "Needing to be cleaned up for
> upstream" and is in my todo list (albeit quite far down given some
> recent Xen crashes found during stress testing)
>
> I can up its priority if there is a need for it.
>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Xen devel - Apr 2013 - [PATCH v2 0/9] Rename/remove IS_PRIV

[PATCH v2 0/9] Rename/remove IS_PRIV

[PATCH 1/9] xen/arch/x86: remove IS_PRIV access check bypasses

[PATCH 2/9] xen/xsm: add hooks for claim

[PATCH 3/9] hvm: convert access check for nested HVM to XSM

[PATCH 4/9] xen/arch/x86: remove IS_PRIV_FOR references

[PATCH 5/9] xen/arch/arm: remove rcu_lock_target_domain_by_id

[PATCH 6/9] xen: rename IS_PRIV to is_hardware_domain

[PATCH 7/9] xen: use domid check in is_hardware_domain

[PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

[PATCH 9/9] IOMMU: use is_hardware_domain instead of domid == 0

Re: [PATCH 7/9] xen: use domid check in is_hardware_domain

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

Re: [PATCH v2 0/9] Rename/remove IS_PRIV

Re: [PATCH 7/9] xen: use domid check in is_hardware_domain

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

Re: [PATCH 7/9] xen: use domid check in is_hardware_domain

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0