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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

Hi all,

> On 8 Mar 2021, at 14:12, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

I’m investigating these points, but I agree with you all that domid 0 should be 
reserved.

> 
>> 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?

Sure I will update the documentation in the v2 patch

> 
>> +
>>  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.

Yes I will create a new patch with this change and I will put on top the v2 
dom0less patch

> 
>> +    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

Thank you for your feedbacks.

Cheers,
Luca