[PATCH v2 0/4] xen/arm: Prevent Dom0 to be loaded when using dom0less

[PATCH v2 0/4] xen/arm: Prevent Dom0 to be loaded when using dom0less

[Luca Fancellu]

This is the v2 of xen/arm: Prevent Dom0 to be loaded when using dom0less
previously pushed.

The aim of this serie is to prevent the creation and run of the domain 0 (Dom0)
when the system configuration to be used is dom0less, that is when the device
tree declares at least one domU and no Dom0.

Changes in v2 are:
 - Moving the dom0 creation in a proper function
 - Handle the case of having an hardware_domain NULL
 - Be sure that the domain id 0 is never allocated
 - Modify documentation about dom0less

Luca Fancellu (4):
  xen/arm: Move dom0 creation in domain_build.c
  xen/arm: Handle cases when hardware_domain is NULL
  xen/arm: Reserve domid 0 for Dom0
  xen/arm: Prevent Dom0 to be loaded when using dom0less

 docs/features/dom0less.pandoc            |  7 +--
 xen/arch/arm/domain_build.c              | 41 ++++++++++++++
 xen/arch/arm/irq.c                       |  2 +-
 xen/arch/arm/setup.c                     | 69 +++++++++++++-----------
 xen/common/domain.c                      |  4 +-
 xen/common/domctl.c                      |  2 +-
 xen/drivers/passthrough/arm/ipmmu-vmsa.c |  2 +-
 xen/drivers/passthrough/arm/smmu-v3.c    |  2 +-
 xen/drivers/passthrough/arm/smmu.c       |  2 +-
 xen/include/asm-arm/domain.h             |  2 +-
 xen/include/asm-arm/setup.h              |  1 +
 xen/include/xen/sched.h                  |  2 +-
 12 files changed, 94 insertions(+), 42 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] xen/arm: Move dom0 creation in domain_build.c

[Luca Fancellu]

Move dom0 creation and start from setup.c to domain_build.c
on a dedicate function.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/domain_build.c | 36 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/setup.c        | 29 +----------------------------
 xen/include/asm-arm/setup.h |  1 +
 3 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 374bf655ee..d7c9c7f4d1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -21,6 +21,7 @@
 #include <asm/device.h>
 #include <asm/kernel.h>
 #include <asm/setup.h>
+#include <asm/tee/tee.h>
 #include <asm/platform.h>
 #include <asm/psci.h>
 #include <asm/setup.h>
@@ -2520,6 +2521,41 @@ void __init create_domUs(void)
     }
 }
 
+struct domain* __init create_dom0(void)
+{
+    struct domain *dom0;
+    struct xen_domctl_createdomain dom0_cfg = {
+        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
+        .max_evtchn_port = -1,
+        .max_grant_frames = gnttab_dom0_frames(),
+        .max_maptrack_frames = -1,
+    };
+
+    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
+    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
+    /*
+     * Xen vGIC supports a maximum of 992 interrupt lines.
+     * 32 are substracted to cover local IRQs.
+     */
+    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
+    if ( gic_number_lines() > 992 )
+        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
+    dom0_cfg.arch.tee_type = tee_get_type();
+    dom0_cfg.max_vcpus = dom0_max_vcpus();
+
+    if ( iommu_enabled )
+        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+
+    dom0 = domain_create(0, &dom0_cfg, true);
+    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
+        panic("Error creating domain 0\n");
+
+    if ( construct_dom0(dom0) != 0)
+        panic("Could not set up DOM0 guest OS\n");
+
+    return dom0;
+}
+
 int __init construct_dom0(struct domain *d)
 {
     struct kernel_info kinfo = {};
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2532ec9739..b405f58996 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -51,7 +51,6 @@
 #include <asm/platform.h>
 #include <asm/procinfo.h>
 #include <asm/setup.h>
-#include <asm/tee/tee.h>
 #include <xsm/xsm.h>
 #include <asm/acpi.h>
 
@@ -805,12 +804,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     const char *cmdline;
     struct bootmodule *xen_bootmodule;
     struct domain *dom0;
-    struct xen_domctl_createdomain dom0_cfg = {
-        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
-        .max_evtchn_port = -1,
-        .max_grant_frames = gnttab_dom0_frames(),
-        .max_maptrack_frames = -1,
-    };
     int rc;
 
     dcache_line_bytes = read_dcache_line_bytes();
@@ -965,27 +958,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     enable_errata_workarounds();
 
     /* Create initial domain 0. */
-    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
-    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
-    /*
-     * Xen vGIC supports a maximum of 992 interrupt lines.
-     * 32 are substracted to cover local IRQs.
-     */
-    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
-    if ( gic_number_lines() > 992 )
-        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
-    dom0_cfg.arch.tee_type = tee_get_type();
-    dom0_cfg.max_vcpus = dom0_max_vcpus();
-
-    if ( iommu_enabled )
-        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
-
-    dom0 = domain_create(0, &dom0_cfg, true);
-    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
-        panic("Error creating domain 0\n");
-
-    if ( construct_dom0(dom0) != 0)
-        panic("Could not set up DOM0 guest OS\n");
+    dom0 = create_dom0();
 
     heap_init_late();
 
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 28bf622aa1..e5f5c7ebc6 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -95,6 +95,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
 
 int construct_dom0(struct domain *d);
 void create_domUs(void);
+struct domain* create_dom0(void);
 
 void discard_initial_modules(void);
 void fw_unreserved_regions(paddr_t s, paddr_t e,
-- 
2.17.1

[PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Luca Fancellu]

The function is_hardware_domain() returns true if the
hardware_domain and the passed domain is NULL, here we
add a check to return false if there is no hardware_domain.

Among the common and arm codebase there are few cases where
the hardware_domain variable is checked to see if the current
domain is equal to the hardware_domain, change this cases to
use is_hardware_domain() function instead.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/irq.c                       | 2 +-
 xen/common/domain.c                      | 4 ++--
 xen/drivers/passthrough/arm/ipmmu-vmsa.c | 2 +-
 xen/drivers/passthrough/arm/smmu-v3.c    | 2 +-
 xen/drivers/passthrough/arm/smmu.c       | 2 +-
 xen/include/asm-arm/domain.h             | 2 +-
 xen/include/xen/sched.h                  | 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index b71b099e6f..b761d90c40 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -412,7 +412,7 @@ bool is_assignable_irq(unsigned int irq)
  */
 bool irq_type_set_by_domain(const struct domain *d)
 {
-    return (d == hardware_domain);
+    return is_hardware_domain(d);
 }
 
 /*
diff --git a/xen/common/domain.c b/xen/common/domain.c
index d85984638a..e8ec3ba445 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
     struct domain *dom0;
     int rv;
 
-    if ( d != hardware_domain || d->domain_id == 0 )
+    if ( !is_hardware_domain(d) || d->domain_id == 0 )
         return 0;
 
     rv = xsm_init_hardware_domain(XSM_HOOK, d);
@@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
     err = err ?: -EILSEQ; /* Release build safety. */
 
     d->is_dying = DOMDYING_dead;
-    if ( hardware_domain == d )
+    if ( is_hardware_domain(d) )
         hardware_domain = old_hwdom;
     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
 
diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index aef358d880..8b8e3a00ba 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -1168,7 +1168,7 @@ static int ipmmu_reassign_device(struct domain *s, struct domain *t,
     int ret = 0;
 
     /* Don't allow remapping on other domain than hwdom */
-    if ( t && t != hardware_domain )
+    if ( t && !is_hardware_domain(t) )
         return -EPERM;
 
     if ( t == s )
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 53d150cdb6..d115df7320 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -3366,7 +3366,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
 	int ret = 0;
 
 	/* Don't allow remapping on other domain than hwdom */
-	if (t && t != hardware_domain)
+	if ( t && !is_hardware_domain(t) )
 		return -EPERM;
 
 	if (t == s)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3e8aa37866..932fdfd6dd 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2670,7 +2670,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
 	int ret = 0;
 
 	/* Don't allow remapping on other domain than hwdom */
-	if (t && t != hardware_domain)
+	if ( t && !is_hardware_domain(t) )
 		return -EPERM;
 
 	if (t == s)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 1da90f207d..738bda5ef3 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -30,7 +30,7 @@ enum domain_type {
 #endif
 
 /* The hardware domain has always its memory direct mapped. */
-#define is_domain_direct_mapped(d) ((d) == hardware_domain)
+#define is_domain_direct_mapped(d) (is_hardware_domain(d))
 
 struct vtimer {
     struct vcpu *v;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 5485d08afb..bfc9d2577c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1022,7 +1022,7 @@ static always_inline bool is_hardware_domain(const struct domain *d)
     if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
         return false;
 
-    return evaluate_nospec(d == hardware_domain);
+    return evaluate_nospec((hardware_domain != NULL) && (d == hardware_domain));
 }
 
 /* This check is for functionality specific to a control domain */
-- 
2.17.1

[PATCH v2 3/4] xen/arm: Reserve domid 0 for Dom0

[Luca Fancellu]

This patch ensure that the domid 0 is allocated only during
start_xen() function by the create_dom0().
Add a comment in create_domUs() right before domain_create()
to explain the importance of the pre-increment operator
on the variable max_init_domid.
Add an additional check in do_domctl() to make sure domid 0
is never used when calling domain_create().

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/domain_build.c | 5 +++++
 xen/common/domctl.c         | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d7c9c7f4d1..3fa5c8e54c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2508,6 +2508,11 @@ void __init create_domUs(void)
                                          GUEST_VPL011_SPI - 32 + 1);
         }
 
+        /*
+         * The variable max_init_domid is initialized with zero, so here it's
+         * very important to use the pre-increment operator to call
+         * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
+         */
         d = domain_create(++max_init_domid, &d_cfg, false);
         if ( IS_ERR(d) )
             panic("Error creating domain %s\n", dt_node_name(node));
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af044e2eda..8258f157ef 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -419,7 +419,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             {
                 if ( dom == DOMID_FIRST_RESERVED )
                     dom = 1;
-                if ( is_free_domid(dom) )
+                if ( (dom != 0) && is_free_domid(dom) )
                     break;
             }
 
-- 
2.17.1

[PATCH v2 4/4] xen/arm: Prevent Dom0 to be loaded when using dom0less

[Luca Fancellu]

This patch prevents the dom0 to be loaded skipping its
building and going forward to build domUs when the dom0
kernel is not found and at least one domU is present.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 docs/features/dom0less.pandoc |  7 +++---
 xen/arch/arm/setup.c          | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
index d798596cdf..a5eb5bcda0 100644
--- a/docs/features/dom0less.pandoc
+++ b/docs/features/dom0less.pandoc
@@ -16,9 +16,10 @@ Multiboot specification has been extended to allow for multiple domains
 to be passed to Xen. See docs/misc/arm/device-tree/booting.txt for more
 information about the Multiboot specification and how to use it.
 
-Currently, a control domain ("dom0") is still required, but in the
-future it will become unnecessary when all domains are created
-directly from Xen. Instead of waiting for the control domain to be fully
+Currently, a control domain ("dom0") is still required to manage the DomU
+domains, but the system can start also without dom0 if the hypervisor
+Device Tree doesn't specify it and it declares one or more domUs.
+Instead of waiting for the control domain (when declared) to be fully
 booted and the Xen tools to become available, domains created by Xen
 this way are started right away in parallel. Hence, their boot time is
 typically much shorter.
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index b405f58996..ecc4f0ae98 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -793,6 +793,38 @@ static void __init setup_mm(void)
 }
 #endif
 
+static bool __init is_dom0less_mode(void)
+{
+    struct bootmodules *mods = &bootinfo.modules;
+    struct bootmodule *mod;
+    unsigned int i;
+    bool dom0found = false;
+    bool domUfound = false;
+
+    /* Look into the bootmodules */
+    for ( i = 0 ; i < mods->nr_mods ; i++ )
+    {
+        mod = &mods->module[i];
+        /* Find if dom0 and domU kernels are present */
+        if ( mod->kind == BOOTMOD_KERNEL )
+        {
+            if ( mod->domU == false )
+            {
+                dom0found = true;
+                break;
+            }
+            else
+                domUfound = true;
+        }
+    }
+
+    /*
+     * If there is no dom0 kernel but at least one domU, then we are in
+     * dom0less mode
+     */
+    return ( !dom0found && domUfound );
+}
+
 size_t __read_mostly dcache_line_bytes;
 
 /* C entry point for boot CPU */
@@ -803,7 +835,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     int cpus, i;
     const char *cmdline;
     struct bootmodule *xen_bootmodule;
-    struct domain *dom0;
+    struct domain *dom0 = NULL;
     int rc;
 
     dcache_line_bytes = read_dcache_line_bytes();
@@ -958,7 +990,10 @@ void __init start_xen(unsigned long boot_phys_offset,
     enable_errata_workarounds();
 
     /* Create initial domain 0. */
-    dom0 = create_dom0();
+    if ( !is_dom0less_mode() )
+        dom0 = create_dom0();
+    else
+        printk(XENLOG_INFO "Xen dom0less mode detected\n");
 
     heap_init_late();
 
@@ -976,7 +1011,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     if ( acpi_disabled )
         create_domUs();
 
-    domain_unpause_by_systemcontroller(dom0);
+    if ( dom0 )
+        domain_unpause_by_systemcontroller(dom0);
 
     /* Switch on to the dynamically allocated stack for the idle vcpu
      * since the static one we're running on is about to be freed. */
-- 
2.17.1

Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Jan Beulich]

On 08.04.2021 11:48, Luca Fancellu wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
>      struct domain *dom0;
>      int rv;
>  
> -    if ( d != hardware_domain || d->domain_id == 0 )
> +    if ( !is_hardware_domain(d) || d->domain_id == 0 )
>          return 0;
>  
>      rv = xsm_init_hardware_domain(XSM_HOOK, d);
> @@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
>      err = err ?: -EILSEQ; /* Release build safety. */
>  
>      d->is_dying = DOMDYING_dead;
> -    if ( hardware_domain == d )
> +    if ( is_hardware_domain(d) )
>          hardware_domain = old_hwdom;
>      atomic_set(&d->refcnt, DOMAIN_DESTROYED);

While these may seem like open-coding of is_hardware_domain(), I
think it would be better to leave them alone. In neither of the two
cases is it possible for d to be NULL afaics, and hence your
addition to is_hardware_domain() doesn't matter here.

> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -30,7 +30,7 @@ enum domain_type {
>  #endif
>  
>  /* The hardware domain has always its memory direct mapped. */
> -#define is_domain_direct_mapped(d) ((d) == hardware_domain)
> +#define is_domain_direct_mapped(d) (is_hardware_domain(d))

Nit: If this was code I'm a maintainer of, I'd ask for the unneeded
parentheses to be dropped.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1022,7 +1022,7 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>      if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>          return false;
>  
> -    return evaluate_nospec(d == hardware_domain);
> +    return evaluate_nospec((hardware_domain != NULL) && (d == hardware_domain));
>  }

This would be the first instance in the tree of an && expression
inside evaluate_nospec(). I think the generated code will still be
okay, but I wonder whether this is really needed. Can you point
out code paths where d may actually be NULL, and where

static always_inline bool is_hardware_domain(const struct domain *d)
{
    if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
        return false;

    if ( !d )
        return false;

    return evaluate_nospec(d == hardware_domain);
}

would not behave as intended (i.e. where bad speculation would
result)? (In any event I think checking d against NULL is preferable
over checking hardware_domain.)

Jan

Re: [PATCH v2 3/4] xen/arm: Reserve domid 0 for Dom0

[Jan Beulich]

On 08.04.2021 11:48, Luca Fancellu wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -419,7 +419,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              {
>                  if ( dom == DOMID_FIRST_RESERVED )
>                      dom = 1;
> -                if ( is_free_domid(dom) )
> +                if ( (dom != 0) && is_free_domid(dom) )
>                      break;
>              }
>  

I don't think this change is needed - I don't see how dom could
ever end up being zero. The code is already intended to be safe
wrt accidentally creating a domain with ID zero. (Granted "rover"
would benefit from being moved into the yet more narrow scope,
which would make this even more obvious.)

Jan

Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Luca Fancellu]

> On 8 Apr 2021, at 11:17, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 08.04.2021 11:48, Luca Fancellu wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
>>     struct domain *dom0;
>>     int rv;
>> 
>> -    if ( d != hardware_domain || d->domain_id == 0 )
>> +    if ( !is_hardware_domain(d) || d->domain_id == 0 )
>>         return 0;
>> 
>>     rv = xsm_init_hardware_domain(XSM_HOOK, d);
>> @@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
>>     err = err ?: -EILSEQ; /* Release build safety. */
>> 
>>     d->is_dying = DOMDYING_dead;
>> -    if ( hardware_domain == d )
>> +    if ( is_hardware_domain(d) )
>>         hardware_domain = old_hwdom;
>>     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
> 
> While these may seem like open-coding of is_hardware_domain(), I
> think it would be better to leave them alone. In neither of the two
> cases is it possible for d to be NULL afaics, and hence your
> addition to is_hardware_domain() doesn't matter here.

Yes that is right, the only thing is that we have a nice function
“Is_hardware_domain” and we and up comparing “manually”.
It looks weird to me, but I can change it back if you don’t agree.

> 
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -30,7 +30,7 @@ enum domain_type {
>> #endif
>> 
>> /* The hardware domain has always its memory direct mapped. */
>> -#define is_domain_direct_mapped(d) ((d) == hardware_domain)
>> +#define is_domain_direct_mapped(d) (is_hardware_domain(d))
> 
> Nit: If this was code I'm a maintainer of, I'd ask for the unneeded
> parentheses to be dropped.

Sure I can do that on the next version of the patch

> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1022,7 +1022,7 @@ static always_inline bool is_hardware_domain(const struct domain *d)
>>     if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>         return false;
>> 
>> -    return evaluate_nospec(d == hardware_domain);
>> +    return evaluate_nospec((hardware_domain != NULL) && (d == hardware_domain));
>> }
> 
> This would be the first instance in the tree of an && expression
> inside evaluate_nospec(). I think the generated code will still be
> okay, but I wonder whether this is really needed. Can you point
> out code paths where d may actually be NULL, and where
> 
> static always_inline bool is_hardware_domain(const struct domain *d)
> {
>    if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>        return false;
> 
>    if ( !d )
>        return false;
> 
>    return evaluate_nospec(d == hardware_domain);
> }
> 
> would not behave as intended (i.e. where bad speculation would
> result)? (In any event I think checking d against NULL is preferable
> over checking hardware_domain.)

I agree with you, I will change the code checking if d is NULL the
way it’s written above

Cheers,
Luca

> 
> Jan

Re: [PATCH v2 3/4] xen/arm: Reserve domid 0 for Dom0

[Luca Fancellu]

> On 8 Apr 2021, at 11:46, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 08.04.2021 11:48, Luca Fancellu wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -419,7 +419,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>             {
>>                 if ( dom == DOMID_FIRST_RESERVED )
>>                     dom = 1;
>> -                if ( is_free_domid(dom) )
>> +                if ( (dom != 0) && is_free_domid(dom) )
>>                     break;
>>             }
>> 
> 
> I don't think this change is needed - I don't see how dom could
> ever end up being zero. The code is already intended to be safe
> wrt accidentally creating a domain with ID zero. (Granted "rover"
> would benefit from being moved into the yet more narrow scope,
> which would make this even more obvious.)

Yes I agree, I will remove the check in the next version patch.

Cheers,
Luca

> 
> Jan

Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Jan Beulich]

On 08.04.2021 15:11, Luca Fancellu wrote:
> 
> 
>> On 8 Apr 2021, at 11:17, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 08.04.2021 11:48, Luca Fancellu wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
>>>     struct domain *dom0;
>>>     int rv;
>>>
>>> -    if ( d != hardware_domain || d->domain_id == 0 )
>>> +    if ( !is_hardware_domain(d) || d->domain_id == 0 )
>>>         return 0;
>>>
>>>     rv = xsm_init_hardware_domain(XSM_HOOK, d);
>>> @@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
>>>     err = err ?: -EILSEQ; /* Release build safety. */
>>>
>>>     d->is_dying = DOMDYING_dead;
>>> -    if ( hardware_domain == d )
>>> +    if ( is_hardware_domain(d) )
>>>         hardware_domain = old_hwdom;
>>>     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
>>
>> While these may seem like open-coding of is_hardware_domain(), I
>> think it would be better to leave them alone. In neither of the two
>> cases is it possible for d to be NULL afaics, and hence your
>> addition to is_hardware_domain() doesn't matter here.
> 
> Yes that is right, the only thing is that we have a nice function
> “Is_hardware_domain” and we and up comparing “manually”.
> It looks weird to me, but I can change it back if you don’t agree.

Well, from the time when late-hwdom was introduced I seem to vaguely
recall that the way it's done was on purpose. It pretty certainly was
also at that time when is_hardware_domain() (or whatever predecessor
predicate) was introduced, which suggests to me that if the above
were meant to use it, they would have been switched at the same time.

Jan

Re: [PATCH v2 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Luca Fancellu]

> On 8 Apr 2021, at 15:36, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 08.04.2021 15:11, Luca Fancellu wrote:
>> 
>> 
>>> On 8 Apr 2021, at 11:17, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 08.04.2021 11:48, Luca Fancellu wrote:
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -308,7 +308,7 @@ static int late_hwdom_init(struct domain *d)
>>>>    struct domain *dom0;
>>>>    int rv;
>>>> 
>>>> -    if ( d != hardware_domain || d->domain_id == 0 )
>>>> +    if ( !is_hardware_domain(d) || d->domain_id == 0 )
>>>>        return 0;
>>>> 
>>>>    rv = xsm_init_hardware_domain(XSM_HOOK, d);
>>>> @@ -705,7 +705,7 @@ struct domain *domain_create(domid_t domid,
>>>>    err = err ?: -EILSEQ; /* Release build safety. */
>>>> 
>>>>    d->is_dying = DOMDYING_dead;
>>>> -    if ( hardware_domain == d )
>>>> +    if ( is_hardware_domain(d) )
>>>>        hardware_domain = old_hwdom;
>>>>    atomic_set(&d->refcnt, DOMAIN_DESTROYED);
>>> 
>>> While these may seem like open-coding of is_hardware_domain(), I
>>> think it would be better to leave them alone. In neither of the two
>>> cases is it possible for d to be NULL afaics, and hence your
>>> addition to is_hardware_domain() doesn't matter here.
>> 
>> Yes that is right, the only thing is that we have a nice function
>> “Is_hardware_domain” and we and up comparing “manually”.
>> It looks weird to me, but I can change it back if you don’t agree.
> 
> Well, from the time when late-hwdom was introduced I seem to vaguely
> recall that the way it's done was on purpose. It pretty certainly was
> also at that time when is_hardware_domain() (or whatever predecessor
> predicate) was introduced, which suggests to me that if the above
> were meant to use it, they would have been switched at the same time.

Perfect, I will change them back and add all the modification we discussed
In the v3.

Thank you for your feedback.

Cheers,
Luca

> 
> Jan

Re: [PATCH v2 1/4] xen/arm: Move dom0 creation in domain_build.c

[Julien Grall]

Hi Luca,

On 08/04/2021 10:48, Luca Fancellu wrote:
> Move dom0 creation and start from setup.c to domain_build.c
> on a dedicate function.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 36 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/setup.c        | 29 +----------------------------
>   xen/include/asm-arm/setup.h |  1 +
>   3 files changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 374bf655ee..d7c9c7f4d1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -21,6 +21,7 @@
>   #include <asm/device.h>
>   #include <asm/kernel.h>
>   #include <asm/setup.h>
> +#include <asm/tee/tee.h>
>   #include <asm/platform.h>
>   #include <asm/psci.h>
>   #include <asm/setup.h>
> @@ -2520,6 +2521,41 @@ void __init create_domUs(void)
>       }
>   }
>   
> +struct domain* __init create_dom0(void)
> +{
> +    struct domain *dom0;
> +    struct xen_domctl_createdomain dom0_cfg = {
> +        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> +        .max_evtchn_port = -1,
> +        .max_grant_frames = gnttab_dom0_frames(),
> +        .max_maptrack_frames = -1,
> +    };
> +
> +    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> +    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> +    /*
> +     * Xen vGIC supports a maximum of 992 interrupt lines.
> +     * 32 are substracted to cover local IRQs.
> +     */
> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
> +    if ( gic_number_lines() > 992 )
> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
> +    dom0_cfg.arch.tee_type = tee_get_type();
> +    dom0_cfg.max_vcpus = dom0_max_vcpus();
> +
> +    if ( iommu_enabled )
> +        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
> +    dom0 = domain_create(0, &dom0_cfg, true);
> +    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> +        panic("Error creating domain 0\n");
> +
> +    if ( construct_dom0(dom0) != 0)
> +        panic("Could not set up DOM0 guest OS\n");
> +
> +    return dom0;
> +}
> +

I would move the function after...

>   int __init construct_dom0(struct domain *d)

... this function so we can mark construct_dom0() static as 
create_dom0() is the only caller.

>   {
>       struct kernel_info kinfo = {};
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2532ec9739..b405f58996 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -51,7 +51,6 @@
>   #include <asm/platform.h>
>   #include <asm/procinfo.h>
>   #include <asm/setup.h>
> -#include <asm/tee/tee.h>
>   #include <xsm/xsm.h>
>   #include <asm/acpi.h>
>   
> @@ -805,12 +804,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>       const char *cmdline;
>       struct bootmodule *xen_bootmodule;
>       struct domain *dom0;
> -    struct xen_domctl_createdomain dom0_cfg = {
> -        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> -        .max_evtchn_port = -1,
> -        .max_grant_frames = gnttab_dom0_frames(),
> -        .max_maptrack_frames = -1,
> -    };
>       int rc;
>   
>       dcache_line_bytes = read_dcache_line_bytes();
> @@ -965,27 +958,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       enable_errata_workarounds();
>   
>       /* Create initial domain 0. */
> -    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> -    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> -    /*
> -     * Xen vGIC supports a maximum of 992 interrupt lines.
> -     * 32 are substracted to cover local IRQs.
> -     */
> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
> -    if ( gic_number_lines() > 992 )
> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
> -    dom0_cfg.arch.tee_type = tee_get_type();
> -    dom0_cfg.max_vcpus = dom0_max_vcpus();
> -
> -    if ( iommu_enabled )
> -        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> -
> -    dom0 = domain_create(0, &dom0_cfg, true);
> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> -        panic("Error creating domain 0\n");
> -
> -    if ( construct_dom0(dom0) != 0)
> -        panic("Could not set up DOM0 guest OS\n");
> +    dom0 = create_dom0();
>   
>       heap_init_late();
>   
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 28bf622aa1..e5f5c7ebc6 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -95,6 +95,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
>   
>   int construct_dom0(struct domain *d);
>   void create_domUs(void);
> +struct domain* create_dom0(void);
>   
>   void discard_initial_modules(void);
>   void fw_unreserved_regions(paddr_t s, paddr_t e,
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v2 4/4] xen/arm: Prevent Dom0 to be loaded when using dom0less

[Julien Grall]

Hi Luca,

On 08/04/2021 10:48, Luca Fancellu wrote:
> This patch prevents the dom0 to be loaded skipping its
> building and going forward to build domUs when the dom0
> kernel is not found and at least one domU is present.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>   docs/features/dom0less.pandoc |  7 +++---
>   xen/arch/arm/setup.c          | 42 ++++++++++++++++++++++++++++++++---
>   2 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
> index d798596cdf..a5eb5bcda0 100644
> --- a/docs/features/dom0less.pandoc
> +++ b/docs/features/dom0less.pandoc
> @@ -16,9 +16,10 @@ Multiboot specification has been extended to allow for multiple domains
>   to be passed to Xen. See docs/misc/arm/device-tree/booting.txt for more
>   information about the Multiboot specification and how to use it.
>   
> -Currently, a control domain ("dom0") is still required, but in the
> -future it will become unnecessary when all domains are created
> -directly from Xen. Instead of waiting for the control domain to be fully
> +Currently, a control domain ("dom0") is still required to manage the DomU
> +domains, but the system can start also without dom0 if the hypervisor

"hypervisor Device Tree" sounds a bit strange to me. I would either drop 
"hypervisor" or say "host Devicet Tree".

> +Device Tree doesn't specify it and it declares one or more domUs.

AFAICT, the first "it" refer to dom0 but it is not clear what exact 
property will used to do the decision.

Also you have two 'it' in a row that refers to two different entities. I 
would name it to avoid confusion.

> +Instead of waiting for the control domain (when declared) to be fully
>   booted and the Xen tools to become available, domains created by Xen
>   this way are started right away in parallel. Hence, their boot time is
>   typically much shorter.
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index b405f58996..ecc4f0ae98 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -793,6 +793,38 @@ static void __init setup_mm(void)
>   }
>   #endif
>   
> +static bool __init is_dom0less_mode(void)
> +{
> +    struct bootmodules *mods = &bootinfo.modules;
> +    struct bootmodule *mod;
> +    unsigned int i;
> +    bool dom0found = false;
> +    bool domUfound = false;
> +
> +    /* Look into the bootmodules */
> +    for ( i = 0 ; i < mods->nr_mods ; i++ )
> +    {
> +        mod = &mods->module[i];
> +        /* Find if dom0 and domU kernels are present */
> +        if ( mod->kind == BOOTMOD_KERNEL )
> +        {
> +            if ( mod->domU == false )
> +            {
> +                dom0found = true;
> +                break;
> +            }

NIT: You can directly return false here because if you have dom0 the it 
can't be dom0less.

> +            else
> +                domUfound = true;
> +        }
> +    }
> +
> +    /*
> +     * If there is no dom0 kernel but at least one domU, then we are in
> +     * dom0less mode
> +     */
> +    return ( !dom0found && domUfound );
> +}
> +
>   size_t __read_mostly dcache_line_bytes;
>   
>   /* C entry point for boot CPU */
> @@ -803,7 +835,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       int cpus, i;
>       const char *cmdline;
>       struct bootmodule *xen_bootmodule;
> -    struct domain *dom0;
> +    struct domain *dom0 = NULL;
>       int rc;
>   
>       dcache_line_bytes = read_dcache_line_bytes();
> @@ -958,7 +990,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>       enable_errata_workarounds();
>   
>       /* Create initial domain 0. */
> -    dom0 = create_dom0();
> +    if ( !is_dom0less_mode() )
> +        dom0 = create_dom0();
> +    else
> +        printk(XENLOG_INFO "Xen dom0less mode detected\n");
>   
>       heap_init_late();
>   
> @@ -976,7 +1011,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>       if ( acpi_disabled )
>           create_domUs();
>   
> -    domain_unpause_by_systemcontroller(dom0);
> +    if ( dom0 )
> +        domain_unpause_by_systemcontroller(dom0);
>   
>       /* Switch on to the dynamically allocated stack for the idle vcpu
>        * since the static one we're running on is about to be freed. */
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/4] xen/arm: Move dom0 creation in domain_build.c

[Luca Fancellu]

> On 9 Apr 2021, at 09:30, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 08/04/2021 10:48, Luca Fancellu wrote:
>> Move dom0 creation and start from setup.c to domain_build.c
>> on a dedicate function.
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>>  xen/arch/arm/domain_build.c | 36 ++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/setup.c        | 29 +----------------------------
>>  xen/include/asm-arm/setup.h |  1 +
>>  3 files changed, 38 insertions(+), 28 deletions(-)
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 374bf655ee..d7c9c7f4d1 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -21,6 +21,7 @@
>>  #include <asm/device.h>
>>  #include <asm/kernel.h>
>>  #include <asm/setup.h>
>> +#include <asm/tee/tee.h>
>>  #include <asm/platform.h>
>>  #include <asm/psci.h>
>>  #include <asm/setup.h>
>> @@ -2520,6 +2521,41 @@ void __init create_domUs(void)
>>      }
>>  }
>>  +struct domain* __init create_dom0(void)
>> +{
>> +    struct domain *dom0;
>> +    struct xen_domctl_createdomain dom0_cfg = {
>> +        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>> +        .max_evtchn_port = -1,
>> +        .max_grant_frames = gnttab_dom0_frames(),
>> +        .max_maptrack_frames = -1,
>> +    };
>> +
>> +    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
>> +    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>> +    /*
>> +     * Xen vGIC supports a maximum of 992 interrupt lines.
>> +     * 32 are substracted to cover local IRQs.
>> +     */
>> +    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>> +    if ( gic_number_lines() > 992 )
>> +        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>> +    dom0_cfg.arch.tee_type = tee_get_type();
>> +    dom0_cfg.max_vcpus = dom0_max_vcpus();
>> +
>> +    if ( iommu_enabled )
>> +        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>> +
>> +    dom0 = domain_create(0, &dom0_cfg, true);
>> +    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>> +        panic("Error creating domain 0\n");
>> +
>> +    if ( construct_dom0(dom0) != 0)
>> +        panic("Could not set up DOM0 guest OS\n");
>> +
>> +    return dom0;
>> +}
>> +
> 
> I would move the function after...
> 
>>  int __init construct_dom0(struct domain *d)
> 
> ... this function so we can mark construct_dom0() static as create_dom0() is the only caller.

Yes, I’ll modify it in the v3.

Cheers,
Luca

> 
>>  {
>>      struct kernel_info kinfo = {};
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 2532ec9739..b405f58996 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -51,7 +51,6 @@
>>  #include <asm/platform.h>
>>  #include <asm/procinfo.h>
>>  #include <asm/setup.h>
>> -#include <asm/tee/tee.h>
>>  #include <xsm/xsm.h>
>>  #include <asm/acpi.h>
>>  @@ -805,12 +804,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      const char *cmdline;
>>      struct bootmodule *xen_bootmodule;
>>      struct domain *dom0;
>> -    struct xen_domctl_createdomain dom0_cfg = {
>> -        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>> -        .max_evtchn_port = -1,
>> -        .max_grant_frames = gnttab_dom0_frames(),
>> -        .max_maptrack_frames = -1,
>> -    };
>>      int rc;
>>        dcache_line_bytes = read_dcache_line_bytes();
>> @@ -965,27 +958,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      enable_errata_workarounds();
>>        /* Create initial domain 0. */
>> -    /* The vGIC for DOM0 is exactly emulating the hardware GIC */
>> -    dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>> -    /*
>> -     * Xen vGIC supports a maximum of 992 interrupt lines.
>> -     * 32 are substracted to cover local IRQs.
>> -     */
>> -    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
>> -    if ( gic_number_lines() > 992 )
>> -        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>> -    dom0_cfg.arch.tee_type = tee_get_type();
>> -    dom0_cfg.max_vcpus = dom0_max_vcpus();
>> -
>> -    if ( iommu_enabled )
>> -        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>> -
>> -    dom0 = domain_create(0, &dom0_cfg, true);
>> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>> -        panic("Error creating domain 0\n");
>> -
>> -    if ( construct_dom0(dom0) != 0)
>> -        panic("Could not set up DOM0 guest OS\n");
>> +    dom0 = create_dom0();
>>        heap_init_late();
>>  diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
>> index 28bf622aa1..e5f5c7ebc6 100644
>> --- a/xen/include/asm-arm/setup.h
>> +++ b/xen/include/asm-arm/setup.h
>> @@ -95,6 +95,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
>>    int construct_dom0(struct domain *d);
>>  void create_domUs(void);
>> +struct domain* create_dom0(void);
>>    void discard_initial_modules(void);
>>  void fw_unreserved_regions(paddr_t s, paddr_t e,
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v2 4/4] xen/arm: Prevent Dom0 to be loaded when using dom0less

[Luca Fancellu]

> On 9 Apr 2021, at 10:12, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 08/04/2021 10:48, Luca Fancellu wrote:
>> This patch prevents the dom0 to be loaded skipping its
>> building and going forward to build domUs when the dom0
>> kernel is not found and at least one domU is present.
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>>  docs/features/dom0less.pandoc |  7 +++---
>>  xen/arch/arm/setup.c          | 42 ++++++++++++++++++++++++++++++++---
>>  2 files changed, 43 insertions(+), 6 deletions(-)
>> diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
>> index d798596cdf..a5eb5bcda0 100644
>> --- a/docs/features/dom0less.pandoc
>> +++ b/docs/features/dom0less.pandoc
>> @@ -16,9 +16,10 @@ Multiboot specification has been extended to allow for multiple domains
>>  to be passed to Xen. See docs/misc/arm/device-tree/booting.txt for more
>>  information about the Multiboot specification and how to use it.
>>  -Currently, a control domain ("dom0") is still required, but in the
>> -future it will become unnecessary when all domains are created
>> -directly from Xen. Instead of waiting for the control domain to be fully
>> +Currently, a control domain ("dom0") is still required to manage the DomU
>> +domains, but the system can start also without dom0 if the hypervisor
> 
> "hypervisor Device Tree" sounds a bit strange to me. I would either drop "hypervisor" or say "host Devicet Tree".
> 
>> +Device Tree doesn't specify it and it declares one or more domUs.
> 
> AFAICT, the first "it" refer to dom0 but it is not clear what exact property will used to do the decision.
> 
> Also you have two 'it' in a row that refers to two different entities. I would name it to avoid confusion.

Yes I will rephrase it, what about:

Currently, a control domain ("dom0") is still required to manage the DomU
domains, but the system can start also without dom0 if the Device Tree
doesn't specify the dom0 kernel and it declares one or more domUs.

> 
>> +Instead of waiting for the control domain (when declared) to be fully
>>  booted and the Xen tools to become available, domains created by Xen
>>  this way are started right away in parallel. Hence, their boot time is
>>  typically much shorter.
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index b405f58996..ecc4f0ae98 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -793,6 +793,38 @@ static void __init setup_mm(void)
>>  }
>>  #endif
>>  +static bool __init is_dom0less_mode(void)
>> +{
>> +    struct bootmodules *mods = &bootinfo.modules;
>> +    struct bootmodule *mod;
>> +    unsigned int i;
>> +    bool dom0found = false;
>> +    bool domUfound = false;
>> +
>> +    /* Look into the bootmodules */
>> +    for ( i = 0 ; i < mods->nr_mods ; i++ )
>> +    {
>> +        mod = &mods->module[i];
>> +        /* Find if dom0 and domU kernels are present */
>> +        if ( mod->kind == BOOTMOD_KERNEL )
>> +        {
>> +            if ( mod->domU == false )
>> +            {
>> +                dom0found = true;
>> +                break;
>> +            }
> 
> NIT: You can directly return false here because if you have dom0 the it can't be dom0less.

When I can I try to have just one exit point from a function, do you think here it can cause
issues?

> 
>> +            else
>> +                domUfound = true;
>> +        }
>> +    }
>> +
>> +    /*
>> +     * If there is no dom0 kernel but at least one domU, then we are in
>> +     * dom0less mode
>> +     */
>> +    return ( !dom0found && domUfound );
>> +}
>> +
>>  size_t __read_mostly dcache_line_bytes;
>>    /* C entry point for boot CPU */
>> @@ -803,7 +835,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      int cpus, i;
>>      const char *cmdline;
>>      struct bootmodule *xen_bootmodule;
>> -    struct domain *dom0;
>> +    struct domain *dom0 = NULL;
>>      int rc;
>>        dcache_line_bytes = read_dcache_line_bytes();
>> @@ -958,7 +990,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      enable_errata_workarounds();
>>        /* Create initial domain 0. */
>> -    dom0 = create_dom0();
>> +    if ( !is_dom0less_mode() )
>> +        dom0 = create_dom0();
>> +    else
>> +        printk(XENLOG_INFO "Xen dom0less mode detected\n");
>>        heap_init_late();
>>  @@ -976,7 +1011,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      if ( acpi_disabled )
>>          create_domUs();
>>  -    domain_unpause_by_systemcontroller(dom0);
>> +    if ( dom0 )
>> +        domain_unpause_by_systemcontroller(dom0);
>>        /* Switch on to the dynamically allocated stack for the idle vcpu
>>       * since the static one we're running on is about to be freed. */
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v2 4/4] xen/arm: Prevent Dom0 to be loaded when using dom0less

[Julien Grall]

On 09/04/2021 10:56, Luca Fancellu wrote:
> 
> 
>> On 9 Apr 2021, at 10:12, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Luca,
>>
>> On 08/04/2021 10:48, Luca Fancellu wrote:
>>> This patch prevents the dom0 to be loaded skipping its
>>> building and going forward to build domUs when the dom0
>>> kernel is not found and at least one domU is present.
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>>   docs/features/dom0less.pandoc |  7 +++---
>>>   xen/arch/arm/setup.c          | 42 ++++++++++++++++++++++++++++++++---
>>>   2 files changed, 43 insertions(+), 6 deletions(-)
>>> diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
>>> index d798596cdf..a5eb5bcda0 100644
>>> --- a/docs/features/dom0less.pandoc
>>> +++ b/docs/features/dom0less.pandoc
>>> @@ -16,9 +16,10 @@ Multiboot specification has been extended to allow for multiple domains
>>>   to be passed to Xen. See docs/misc/arm/device-tree/booting.txt for more
>>>   information about the Multiboot specification and how to use it.
>>>   -Currently, a control domain ("dom0") is still required, but in the
>>> -future it will become unnecessary when all domains are created
>>> -directly from Xen. Instead of waiting for the control domain to be fully
>>> +Currently, a control domain ("dom0") is still required to manage the DomU
>>> +domains, but the system can start also without dom0 if the hypervisor
>>
>> "hypervisor Device Tree" sounds a bit strange to me. I would either drop "hypervisor" or say "host Devicet Tree".
>>
>>> +Device Tree doesn't specify it and it declares one or more domUs.
>>
>> AFAICT, the first "it" refer to dom0 but it is not clear what exact property will used to do the decision.
>>
>> Also you have two 'it' in a row that refers to two different entities. I would name it to avoid confusion.
> 
> Yes I will rephrase it, what about:
> 
> Currently, a control domain ("dom0") is still required to manage the DomU
> domains, but the system can start also without dom0 if the Device Tree
> doesn't specify the dom0 kernel and it declares one or more domUs.

Sounds good to me.

> 
>>
>>> +Instead of waiting for the control domain (when declared) to be fully
>>>   booted and the Xen tools to become available, domains created by Xen
>>>   this way are started right away in parallel. Hence, their boot time is
>>>   typically much shorter.
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index b405f58996..ecc4f0ae98 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -793,6 +793,38 @@ static void __init setup_mm(void)
>>>   }
>>>   #endif
>>>   +static bool __init is_dom0less_mode(void)
>>> +{
>>> +    struct bootmodules *mods = &bootinfo.modules;
>>> +    struct bootmodule *mod;
>>> +    unsigned int i;
>>> +    bool dom0found = false;
>>> +    bool domUfound = false;
>>> +
>>> +    /* Look into the bootmodules */
>>> +    for ( i = 0 ; i < mods->nr_mods ; i++ )
>>> +    {
>>> +        mod = &mods->module[i];
>>> +        /* Find if dom0 and domU kernels are present */
>>> +        if ( mod->kind == BOOTMOD_KERNEL )
>>> +        {
>>> +            if ( mod->domU == false )
>>> +            {
>>> +                dom0found = true;
>>> +                break;
>>> +            }
>>
>> NIT: You can directly return false here because if you have dom0 the it can't be dom0less.
> 
> When I can I try to have just one exit point from a function, do you think here it can cause
> issues?

I don't think so. I was only asking that because:
   - It is clearer to me that when you find dom0 then it must not a 
dom0less configuration.
   - It removes dom0found and reduce the code

But this is a non-important things to me (hence the NIT). If you prefer 
your version, then I am happy with it :).

Cheers,

-- 
Julien Grall