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

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 08/03/2021 11:56, 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.
As you are skipping dom0, the domid 0 will not be usable for another 
domain. I can see a few issues:
   1) The first domU created will now be considered as the hardware 
domain (see domain_create()).
   2)  There are still a few hardcoded use of d->domain_id == 0 in the 
codebase (I could spot at least on in the RTDS code).
   3) Not all the code seems to be able to cope with hardware_domain is 
NULL (although most of it looks to be only reachable by x86)?
   4) is_hardware_domain() will return true when passing NULL. It is 
not clear whether one may pass NULL here.
For 2), ideally this needs to be fixed. But we may also want to reserve 
domid 0 just for sanity.
For 3) and 4), you will need to go through the code and check the usage.

> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>   xen/arch/arm/setup.c | 83 +++++++++++++++++++++++++++++++-------------
>   1 file changed, 59 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2532ec9739..6d169ff6ce 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -794,6 +794,35 @@ 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;
> +            else
> +                domUfound = true;
> +        }
> +    }
> +
> +    /*
> +     * If there is no dom0 kernel but at least one domU, then we are in
> +     * dom0less mode
> +     */
> +    return ( !dom0found && domUfound );
> +}
Should the documentation be updated to reflect this change?

> +
>   size_t __read_mostly dcache_line_bytes;
>   
>   /* C entry point for boot CPU */
> @@ -804,7 +833,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;
>       struct xen_domctl_createdomain dom0_cfg = {
>           .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>           .max_evtchn_port = -1,
> @@ -964,28 +993,33 @@ void __init start_xen(unsigned long boot_phys_offset,
>       apply_alternatives_all();
>       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");
> +    if ( !is_dom0less_mode() )
> +    {
> +        /* 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");
> +    }
It always felt a bit strange the dom0 creation is partly happening in 
setup.c when for domU everythink will happen in domain_build.c.
Woule you be able to create a patch that will first move the code in a 
new function (maybe create_dom0())? The function would return NULL in 
case of an error or the domain.
> +    else
> +        printk(XENLOG_INFO "Xen dom0less mode detected\n");
>   
>       heap_init_late();
>   
> @@ -1003,7 +1037,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