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

[PATCH v3 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

Changes in v3: minor fixes explained in each patches

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              | 43 ++++++++++++++-
 xen/arch/arm/irq.c                       |  2 +-
 xen/arch/arm/setup.c                     | 69 +++++++++++++-----------
 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              |  2 +-
 xen/include/xen/sched.h                  |  3 ++
 10 files changed, 94 insertions(+), 40 deletions(-)

-- 
2.17.1

[PATCH v3 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>
---
v3 changes:
- move create_dom0 function after construct_dom0 and
  make construct_dom0 static
---
 xen/arch/arm/domain_build.c | 38 ++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/setup.c        | 29 +---------------------------
 xen/include/asm-arm/setup.h |  2 +-
 3 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 374bf655ee..359957dc1b 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,7 +2521,7 @@ void __init create_domUs(void)
     }
 }
 
-int __init construct_dom0(struct domain *d)
+static int __init construct_dom0(struct domain *d)
 {
     struct kernel_info kinfo = {};
     int rc;
@@ -2578,6 +2579,41 @@ int __init construct_dom0(struct domain *d)
     return construct_domain(d, &kinfo);
 }
 
+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;
+}
+
 /*
  * Local variables:
  * mode: C
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..5283244015 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -93,8 +93,8 @@ void acpi_create_efi_mmap_table(struct domain *d,
 
 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 v3 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>
---
v3 changes:
- removed unneeded parenthesis for macro is_domain_direct_mapped
- is_hardware_domain() checks for the passed domain and if it is
  NULL, it returns false.
- reverted back checks in the function late_hwdom_init
---
 xen/arch/arm/irq.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/xen/sched.h                  | 3 +++
 6 files changed, 8 insertions(+), 5 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/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..0a74df9931 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..5ba90f1214 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1022,6 +1022,9 @@ 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);
 }
 
-- 
2.17.1

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

[Luca Fancellu]

This patch adds a comment in create_domUs() right before
domain_create() to explain the importance of the pre-increment
operator on the variable max_init_domid, to ensure that the
domid 0 is allocated only during start_xen() function by the
create_dom0() and not on any other possible code path to the
domain_create() function.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v3:
- removed check introduced in v2.
---
 xen/arch/arm/domain_build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 359957dc1b..b1d7b9849f 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));
-- 
2.17.1

[PATCH v3 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>
---
v3 changes:
- Rephrase documentation
---
 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..c9edb529e1 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 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;
+            }
+            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 v3 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Jan Beulich]

On 12.04.2021 12:52, Luca Fancellu wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1022,6 +1022,9 @@ 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);
>  }

On v2 I did say on the respective code that was here (and my
suggestion of this alternative adjustment): "Can you point out
code paths where d may actually be NULL, and where [...] would
not behave as intended (i.e. where bad speculation would
result)?"

Since you've taken the suggestion as-is, and since the commit
message says nothing in either direction here, did you actually
verify that there's no abuse of speculation possible with this
extra return path? And did you find any caller at all which may
pass NULL into here?

Jan

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

[Luca Fancellu]

> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 12.04.2021 12:52, Luca Fancellu wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1022,6 +1022,9 @@ 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);
>> }
> 
> On v2 I did say on the respective code that was here (and my
> suggestion of this alternative adjustment): "Can you point out
> code paths where d may actually be NULL, and where [...] would
> not behave as intended (i.e. where bad speculation would
> result)?"
> 
> Since you've taken the suggestion as-is, and since the commit
> message says nothing in either direction here, did you actually
> verify that there's no abuse of speculation possible with this
> extra return path? And did you find any caller at all which may
> pass NULL into here?

Hi Jan,

I’ve analysed the code and seems that there are no path that calls 
Is_hardware_domain() with a NULL domain, however I found this
function in xen/arch/arm/irq.c:

bool irq_type_set_by_domain(const struct domain *d)
{
    return is_hardware_domain(d);
}

That is calling directly is_hardware_domain and it is global.
It can be the case that a future code can call irq_type_set_by_domain
potentially with a null domain...
I introduced a check for the domain for that reason, do you think that
maybe it’s better to put that check on the irq_type_set_by_domain instead?

Thank you for your feedback.

Cheers,
Luca

> 
> Jan

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

[Jan Beulich]

On 12.04.2021 15:58, Luca Fancellu wrote:
> 
> 
>> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 12.04.2021 12:52, Luca Fancellu wrote:
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -1022,6 +1022,9 @@ 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);
>>> }
>>
>> On v2 I did say on the respective code that was here (and my
>> suggestion of this alternative adjustment): "Can you point out
>> code paths where d may actually be NULL, and where [...] would
>> not behave as intended (i.e. where bad speculation would
>> result)?"
>>
>> Since you've taken the suggestion as-is, and since the commit
>> message says nothing in either direction here, did you actually
>> verify that there's no abuse of speculation possible with this
>> extra return path? And did you find any caller at all which may
>> pass NULL into here?
> 
> Hi Jan,
> 
> I’ve analysed the code and seems that there are no path that calls 
> Is_hardware_domain() with a NULL domain, however I found this
> function in xen/arch/arm/irq.c:
> 
> bool irq_type_set_by_domain(const struct domain *d)
> {
>     return is_hardware_domain(d);
> }
> 
> That is calling directly is_hardware_domain and it is global.
> It can be the case that a future code can call irq_type_set_by_domain
> potentially with a null domain...

I don't think that a function being global (or not) matters here. This
might be different in an environment like Linux, where modules may
also call functions, and where guarding against NULL might be desirable
in certain cases.

> I introduced a check for the domain for that reason, do you think that
> maybe it’s better to put that check on the irq_type_set_by_domain instead?

If there's a specific reason to have a guard here, then it should be
here, yes. As per above I would think though that if there's no
present reason to check for NULL, such a check would best be omitted.
We don't typically check internal function arguments like this, after
all.

Jan

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

[Luca Fancellu]

> On 12 Apr 2021, at 15:22, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 12.04.2021 15:58, Luca Fancellu wrote:
>> 
>> 
>>> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 12.04.2021 12:52, Luca Fancellu wrote:
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -1022,6 +1022,9 @@ 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);
>>>> }
>>> 
>>> On v2 I did say on the respective code that was here (and my
>>> suggestion of this alternative adjustment): "Can you point out
>>> code paths where d may actually be NULL, and where [...] would
>>> not behave as intended (i.e. where bad speculation would
>>> result)?"
>>> 
>>> Since you've taken the suggestion as-is, and since the commit
>>> message says nothing in either direction here, did you actually
>>> verify that there's no abuse of speculation possible with this
>>> extra return path? And did you find any caller at all which may
>>> pass NULL into here?
>> 
>> Hi Jan,
>> 
>> I’ve analysed the code and seems that there are no path that calls 
>> Is_hardware_domain() with a NULL domain, however I found this
>> function in xen/arch/arm/irq.c:
>> 
>> bool irq_type_set_by_domain(const struct domain *d)
>> {
>>    return is_hardware_domain(d);
>> }
>> 
>> That is calling directly is_hardware_domain and it is global.
>> It can be the case that a future code can call irq_type_set_by_domain
>> potentially with a null domain...
> 
> I don't think that a function being global (or not) matters here. This
> might be different in an environment like Linux, where modules may
> also call functions, and where guarding against NULL might be desirable
> in certain cases.
> 
>> I introduced a check for the domain for that reason, do you think that
>> maybe it’s better to put that check on the irq_type_set_by_domain instead?
> 
> If there's a specific reason to have a guard here, then it should be
> here, yes. As per above I would think though that if there's no
> present reason to check for NULL, such a check would best be omitted.
> We don't typically check internal function arguments like this, after
> all.

Thank you Jan, so as you pointed out, since there is no actual path to call
Is_hardware_domain() with a NULL pointer, then I will remove the check
from is_hardware_domain() in a v4 patch.

Cheers,
Luca

> 
> Jan

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

[Julien Grall]

Hi Luca,

On 12/04/2021 11:52, Luca Fancellu wrote:
> Move dom0 creation and start from setup.c to domain_build.c
> on a dedicate function.

s/dedicate/dedicated/

I would also suggest to add "No functional changes intended" to make 
clear this is only code movement.

> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v3 changes:
> - move create_dom0 function after construct_dom0 and
>    make construct_dom0 static
> ---
>   xen/arch/arm/domain_build.c | 38 ++++++++++++++++++++++++++++++++++++-
>   xen/arch/arm/setup.c        | 29 +---------------------------
>   xen/include/asm-arm/setup.h |  2 +-
>   3 files changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 374bf655ee..359957dc1b 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,7 +2521,7 @@ void __init create_domUs(void)
>       }
>   }
>   
> -int __init construct_dom0(struct domain *d)
> +static int __init construct_dom0(struct domain *d)

This wants a sentence in the commit message. How about the following 
commit message:

"
Move dom0 create and start from setup.c to a dedicated function in 
domain_build.c.

With this change, the function construct_dom0() is not used outside of 
domain_build.c anymore. So it is now a static function.

No functional changes intended.
"

If you agree with the new commit message. I can modify while commiting it:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Luca,

The title probably wants to be updated as you don't really reserve domid 
0. How about:

xen/arm: Clarify how the domid is decided in create_domUs()

On 12/04/2021 11:52, Luca Fancellu wrote:
> This patch adds a comment in create_domUs() right before
> domain_create() to explain the importance of the pre-increment
> operator on the variable max_init_domid, to ensure that the
> domid 0 is allocated only during start_xen() function by the
> create_dom0() and not on any other possible code path to the
> domain_create() function.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> Changes in v3:
> - removed check introduced in v2.
> ---
>   xen/arch/arm/domain_build.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 359957dc1b..b1d7b9849f 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));
> 

-- 
Julien Grall

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

[Julien Grall]

On 12/04/2021 11:52, 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>
> ---
> v3 changes:
> - Rephrase documentation
> ---
>   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..c9edb529e1 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 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;
> +            }
> +            else
> +                domUfound = true;
> +        }
> +    }
> +
> +    /*
> +     * If there is no dom0 kernel but at least one domU, then we are in
> +     * dom0less mode

NIT: missing full stop.

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Luca Fancellu]

> On 13 Apr 2021, at 17:40, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 12/04/2021 11:52, Luca Fancellu wrote:
>> Move dom0 creation and start from setup.c to domain_build.c
>> on a dedicate function.
> 
> s/dedicate/dedicated/
> 
> I would also suggest to add "No functional changes intended" to make clear this is only code movement.
> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v3 changes:
>> - move create_dom0 function after construct_dom0 and
>>   make construct_dom0 static
>> ---
>>  xen/arch/arm/domain_build.c | 38 ++++++++++++++++++++++++++++++++++++-
>>  xen/arch/arm/setup.c        | 29 +---------------------------
>>  xen/include/asm-arm/setup.h |  2 +-
>>  3 files changed, 39 insertions(+), 30 deletions(-)
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 374bf655ee..359957dc1b 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,7 +2521,7 @@ void __init create_domUs(void)
>>      }
>>  }
>>  -int __init construct_dom0(struct domain *d)
>> +static int __init construct_dom0(struct domain *d)
> 
> This wants a sentence in the commit message. How about the following commit message:
> 
> "
> Move dom0 create and start from setup.c to a dedicated function in domain_build.c.
> 
> With this change, the function construct_dom0() is not used outside of domain_build.c anymore. So it is now a static function.
> 
> No functional changes intended.
> "
> 
> If you agree with the new commit message. I can modify while commiting it:

Hi Julien, yes I agree, since I have to push a v4, I can add the modifications above in that

Cheers,
Luca

> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 
> -- 
> Julien Grall

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

[Luca Fancellu]

> On 13 Apr 2021, at 18:00, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> The title probably wants to be updated as you don't really reserve domid 0. How about:
> 
> xen/arm: Clarify how the domid is decided in create_domUs()

Sure I’ll update in the v4 patch I will send soon

Cheers,
Luca

> 
> On 12/04/2021 11:52, Luca Fancellu wrote:
>> This patch adds a comment in create_domUs() right before
>> domain_create() to explain the importance of the pre-increment
>> operator on the variable max_init_domid, to ensure that the
>> domid 0 is allocated only during start_xen() function by the
>> create_dom0() and not on any other possible code path to the
>> domain_create() function.
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> Changes in v3:
>> - removed check introduced in v2.
>> ---
>>  xen/arch/arm/domain_build.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 359957dc1b..b1d7b9849f 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));
> 
> -- 
> Julien Grall