Re: [Xen-devel] [PATCH 6/7] xen: Allow hardare domain != dom0

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 04/11/2014 11:20 AM, Jan Beulich wrote:
> > > > On 11.04.14 at 17:07, <dgdegra@xxxxxxxxxxxxx> wrote:
> > On 04/11/2014 05:13 AM, Jan Beulich wrote:
> > > > > > On 27.03.14 at 12:52, <dgdegra@xxxxxxxxxxxxx> 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 <dgdegra@xxxxxxxxxxxxx>
---
 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

Attachment: 0006-xen-introduce-is_hardware_domain_by_id.patch
Description: Text Data

Attachment: 0007-xen-Allow-hardware-domain-dom0.patch
Description: Text Data

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