On 04/11/2014 11:20 AM, Jan Beulich wrote: >>>> On 11.04.14 at 17:07, wrote: >> On 04/11/2014 05:13 AM, Jan Beulich wrote: >>>>>> On 27.03.14 at 12:52, wrote: >>>> @@ -794,7 +795,7 @@ void watchdog_domain_destroy(struct domain *d); >>>> * (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)->domain_id == 0) >>>> +#define is_hardware_domain(d) ((d)->domain_id == hardware_domid) >>> >>> This macro should imo evaluate to true for Dom0 until the hardware >>> domain go created, i.e. you should compare _d with hardware_domain >>> rather than their IDs. With that the definition of hardware_domid can >>> then also be moved inside the #ifdef requested above. >> >> This isn't quite as simple as changing the function since there are >> some places where is_hardware_domain needs to return false for domain 0 >> when a hardware domain is used. Also, the hardware_domain variable is >> not set until domain_create returns, so there are a few places where the >> domain ID still needs to be checked explicitly. It should be possible >> to create an is_hardware_domid function for those cases, if comparing to >> hardware_domain is preferred for most cases; I think that would belong in >> a new patch 5.5/7 (i.e. 6/8 in v4). >> >> Otherwise, I think the is_hardware_domain definition should be: >> >> #ifdef CONFIG_LATE_HWDOM >> #define is_hardware_domain(_d) ((_d)->domain_id == hardware_domid) >> #else >> #define is_hardware_domain(_d) ((_d)->domain_id == 0) >> #endif >> >> This also allows hardware_domid to be declared inside the #ifdef. > > But that still wouldn't necessarily do the correct thing for any use of > the macro before that new special case code in domain_create() got > run. Maybe my thinking of this is wrong, but as I tried to state above, > I would expect Dom0 to be the hardware domain up to the point > where the intended hardware domain gets created, at which point all > state Dom0 obtained because of having been the de-facto hardware > domain get transferred to hardware_domain. I agree with this in most cases, and I think the few places where that is not true should be changed to make them more explicit. This must include all checks in domain_create and those in functions called from domain_create, because (d == hardware_domain) is always false inside domain_create. An initial version of this patch is below, but unless there are objections I plan to integrate it into patch 1 to avoid doing (d->domain_id == 0) => is_hardware_domain => is_hardware_domain_by_id for the ARM code. ------------------------------>8------------------------ Subject: [PATCH RFC 6/8] xen: introduce is_hardware_domain_by_id In order to better handle the period when domain 0 is running but has not yet built a late hardware domain, change is_hardware_domain to do what its name implies: check that the passed domain is the hardware domain. To address some cases where this variable is not set (in particular, during the creation of domain 0 this variable will still be NULL), the old check is retained with the name is_hardware_domain_by_id. Signed-off-by: Daniel De Graaf --- xen/arch/arm/domain.c | 2 +- xen/arch/arm/gic.c | 2 +- xen/arch/arm/vgic.c | 2 +- xen/arch/arm/vtimer.c | 2 +- xen/arch/arm/vuart.c | 2 +- xen/common/domain.c | 4 ++-- xen/include/xen/sched.h | 9 ++++++++- 7 files changed, 15 insertions(+), 8 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ccccb77..4f235e4 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -528,7 +528,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) * Only use it for the hardware domain because the linux kernel may not * support multi-platform. */ - if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) + if ( is_hardware_domain_by_id(d) && (rc = domain_vuart_init(d)) ) goto fail; return 0; diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 8168b7b..5c16aba 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -868,7 +868,7 @@ int gicv_setup(struct domain *d) * The hardware domain gets the hardware address. * Guests get the virtual platform layout. */ - if ( is_hardware_domain(d) ) + if ( is_hardware_domain_by_id(d) ) { d->arch.vgic.dbase = gic.dbase; d->arch.vgic.cbase = gic.cbase; diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 4a7f8c0..cc795e0 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 ( is_hardware_domain(d) ) + if ( is_hardware_domain_by_id(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/vtimer.c b/xen/arch/arm/vtimer.c index cb690bb..4dcb80d 100644 --- a/xen/arch/arm/vtimer.c +++ b/xen/arch/arm/vtimer.c @@ -54,7 +54,7 @@ int vcpu_domain_init(struct domain *d) int vcpu_vtimer_init(struct vcpu *v) { struct vtimer *t = &v->arch.phys_timer; - bool_t d0 = is_hardware_domain(v->domain); + bool_t d0 = is_hardware_domain_by_id(v->domain); /* * Hardware domain uses the hardware interrupts, guests get the virtual diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c index c02a8a9..cf9745c 100644 --- a/xen/arch/arm/vuart.c +++ b/xen/arch/arm/vuart.c @@ -46,7 +46,7 @@ int domain_vuart_init(struct domain *d) { - ASSERT( is_hardware_domain(d) ); + ASSERT( is_hardware_domain_by_id(d) ); d->arch.vuart.info = serial_vuart_info(SERHND_DTUART); if ( !d->arch.vuart.info ) diff --git a/xen/common/domain.c b/xen/common/domain.c index c4720a9..4036e69 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -237,7 +237,7 @@ struct domain *domain_create( else if ( domcr_flags & DOMCRF_pvh ) d->guest_type = guest_type_pvh; - if ( is_hardware_domain(d) ) + if ( is_hardware_domain_by_id(d) ) { d->is_pinned = opt_dom0_vcpus_pin; d->disable_migrate = 1; @@ -262,7 +262,7 @@ struct domain *domain_create( d->is_paused_by_controller = 1; atomic_inc(&d->pause_count); - if ( !is_hardware_domain(d) ) + if ( !is_hardware_domain_by_id(d) ) d->nr_pirqs = nr_static_irqs + extra_domU_irqs; else d->nr_pirqs = nr_static_irqs + extra_dom0_irqs; diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index cbbe8a4..34447f1 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -797,7 +797,14 @@ void watchdog_domain_destroy(struct domain *d); * (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)->domain_id == 0) +#define is_hardware_domain(_d) ((_d) == hardware_domain) + +/* + * Check for the hardware domain based on domain ID, not using the global + * hardware_domain variable. This is necessary inside domain_create where + * hardware_domain is NULL and for some checks implementing CONFIG_LATE_HWDOM. + */ +#define is_hardware_domain_by_id(_d) ((_d)->domain_id == 0) /* This check is for functionality specific to a control domain */ #define is_control_domain(_d) ((_d)->is_privileged) -- 1.9.0