[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2] xen: use domid check in is_hardware_domain



On 09/07/13 21:28, Daniel De Graaf 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.

The distinction between is_hardware_domain, is_control_domain, and
domain 0 is based on the following disaggregation model:

Domain 0 bootstraps the system.  It may remain to perform requested
builds of domains that need a minimal trust chain (i.e. vTPM domains).
Other than being built by the hypervisor, nothing is special about this
domain - although it may be useful to have is_control_domain() return
true depending on the toolstack it uses to build other domains.

The hardware domain manages devices for PCI pass-through to driver
domains or can act as a driver domain itself, depending on the desired
degree of disaggregation.  It is also the domain managing devices that
do not support pass-through: PCI configuration space access, parsing the
hardware ACPI tables and system power or machine check events.  This is
the only domain where is_hardware_domain() is true.  The return of
is_control_domain() is false for this domain.

The control domain manages other domains, controls guest launch and
shutdown, and manages resource constraints; is_control_domain() returns
true.  The functionality guarded by is_control_domain may in the future
be adapted to use explicit hypercalls, eliminating the special treatment
of this domain.  It may be reasonable to have multiple control domains
on a multi-tenant system.

Guest domains and other service or driver domains are all treated
identically by the hypervisor; the security policy may further constrain
administrative actions on or communication between these domains.

Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
---

Changes from v1: Collapse patches and improve commit message.

  xen/arch/arm/domain.c                       |  2 +-
  xen/arch/arm/vgic.c                         |  2 +-
  xen/arch/arm/vpl011.c                       |  4 ++--
  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 ++--
  xen/common/domain.c                         | 10 +++++-----
  xen/drivers/passthrough/amd/pci_amd_iommu.c |  2 +-
  xen/drivers/passthrough/vtd/iommu.c         |  8 ++++----
  xen/drivers/passthrough/vtd/x86/vtd.c       |  2 +-
  xen/include/xen/sched.h                     |  4 ++--
  12 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index f465ab7..c7dc69a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -504,7 +504,7 @@ int arch_domain_create(struct domain *d, unsigned int 
domcr_flags)
          goto fail;
/* Domain 0 gets a real UART not an emulated one */
-    if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 )
+    if ( !is_hardware_domain(d) && (rc = domain_uart0_init(d)) != 0 )
          goto fail;
return 0;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2e4b11f..d9c73be 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -82,7 +82,7 @@ int domain_vgic_init(struct domain *d)
      /* Currently nr_lines in vgic and gic doesn't have the same meanings
       * Here nr_lines = number of SPIs
       */
-    if ( d->domain_id == 0 )
+    if ( is_hardware_domain(d) )
          d->arch.vgic.nr_lines = gic_number_lines() - 32;
      else
          d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 13ba623..0e9454f 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -43,7 +43,7 @@
int domain_uart0_init(struct domain *d)
  {
-    ASSERT( d->domain_id );
+    ASSERT( !is_hardware_domain(d) );
spin_lock_init(&d->arch.uart0.lock);
      d->arch.uart0.idx = 0;
@@ -87,7 +87,7 @@ static int uart0_mmio_check(struct vcpu *v, paddr_t addr)
  {
      struct domain *d = v->domain;
- return d->domain_id != 0 && addr >= UART0_START && addr < UART0_END;
+    return !is_hardware_domain(d) && addr >= UART0_START && addr < UART0_END;
  }
static int uart0_mmio_read(struct vcpu *v, mmio_info_t *info)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 874742c..51d0ea6 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1459,7 +1459,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
          }
set_cpuid_faulting(!is_hvm_vcpu(next) &&
-                           (next->domain->domain_id != 0));
+                           !is_control_domain(next->domain));

Won't this mean that in your separate hardware/control domain model that the hardware domain *will* fault on cpuid? Is this what we want?

      }
if (is_hvm_vcpu(next) && (prev != next) )
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) )

Ack.

      {
          /* nothing to do */;
      }
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index cf8bc78..bacc8ef 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1885,7 +1885,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;
@@ -1962,7 +1962,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);

Why have direct access to the tsc for the hardware domain and not the control domain?

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 57dbd0c..8e8d3d1 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -745,7 +745,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
      c = regs->ecx;
      d = regs->edx;
- if ( current->domain->domain_id != 0 )
+    if ( !is_control_domain(current->domain) )
      {
          unsigned int cpuid_leaf = a, sub_leaf = c;

Same as above re cpuid.

@@ -1836,7 +1836,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"
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6c264a5..692372a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -238,7 +238,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;

At some point this will have to be thought about a bit more -- which of the disaggregated domains do we actually want pinned here? But I think this is fine for now.

Everything else looks reasonable to me, but obviously needs acks from various maintainers.

 -George

@@ -263,10 +263,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;
@@ -600,7 +600,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/drivers/passthrough/amd/pci_amd_iommu.c 
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 5ea1a1d..e8ec695 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); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ae6a3b8..4e6edf5 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -722,10 +722,10 @@ void watchdog_domain_destroy(struct domain *d);
  /*
   * Use this check when the following are both true:
   *  - Using this feature or interface requires full access to the hardware
- *    (that is, this is would not be suitable for a driver domain)
+ *    (that is, this 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)
/* This check is for functionality specific to a control domain */
  #define is_control_domain(_d) ((_d)->is_privileged)


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.