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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@tycho.nsa.gov>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> ---
>
> 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)