From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2] xen: use domid check in is_hardware_domain Date: Wed, 10 Jul 2013 11:57:00 +0100 Message-ID: <51DD3DFC.4090300@eu.citrix.com> References: <1373401725-3815-1-git-send-email-dgdegra@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1373401725-3815-1-git-send-email-dgdegra@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel De Graaf Cc: Stefano Stabellini , Tim Deegan , Keir Fraser , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > Acked-by: Jan Beulich > Cc: Keir Fraser > Cc: Ian Campbell > Cc: Stefano Stabellini > Cc: Tim Deegan > Cc: George Dunlap > --- > > 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)