Re: [Xen-devel] [PATCH RFC 14/15] xen/arm: call construct_domU from start_xen and start DomU VMs

[Julien Grall <julien.grall@xxxxxxx>]

On 06/13/2018 11:15 PM, Stefano Stabellini wrote:
> Introduce support for the "xen,domU" compatible node on device tree.
> Create new DomU VMs based on the information found on device tree under
> "xen,domU".
While I like the idea of having multiple domain created by Xen, I think 
there are still few open questions here:
	1) The domains will be listed via "xl list". So are they still 
manageable via DOMCTL?
        2) Is it possible to restart those domains?
	3) If a domain crash, what will happen? Are they just going to 	sit 
there using resources until the platform rebooted?
	4) How do you handle scheduling? Is it still possible to do it via 
Dom0? How about the dom0less situation?
> Introduce a simple global variable named max_init_domid to keep track of
> the initial allocated domids.
What is the exact goal of this new variable?

> Move the discard_initial_modules after DomUs have been built
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c |  2 --
>   xen/arch/arm/setup.c        | 35 ++++++++++++++++++++++++++++++++++-
>   xen/include/asm-arm/setup.h |  2 ++
>   xen/include/asm-x86/setup.h |  2 ++
You need to CC x86 maintainers for this change.

>   4 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 97f14ca..e2d370f 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2545,8 +2545,6 @@ int __init construct_dom0(struct domain *d)
>       if ( rc < 0 )
>           return rc;
>   
> -    discard_initial_modules();
> -
Please mention this move in the commit message.

>       return __construct_domain(d, &kinfo);
>   }
>   
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 98bdb24..3723704 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -63,6 +63,8 @@ static unsigned long opt_xenheap_megabytes __initdata;
>   integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>   #endif
>   
> +domid_t __read_mostly max_init_domid = 0;
> +
>   static __used void init_done(void)
>   {
>       free_init_memory();
> @@ -711,6 +713,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>       struct bootmodule *xen_bootmodule;
>       struct domain *dom0;
>       struct xen_domctl_createdomain dom0_cfg = {};
> +    struct dt_device_node *chosen;
> +    struct dt_device_node *node;
>   
>       dcache_line_bytes = read_dcache_line_bytes();
>   
> @@ -860,7 +864,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>       dom0_cfg.arch.nr_spis = gic_number_lines() - 32;
>   
> -    dom0 = domain_create(0, &dom0_cfg);
> +    dom0 = domain_create(max_init_domid++, &dom0_cfg);
>       if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>               panic("Error creating domain 0");
>   
> @@ -886,6 +890,35 @@ void __init start_xen(unsigned long boot_phys_offset,
>   
>       domain_unpause_by_systemcontroller(dom0);
>   
> +    chosen = dt_find_node_by_name(dt_host, "chosen");
> +    if ( chosen != NULL )
> +    {
> +        dt_for_each_child_node(chosen, node)
> +        {
> +            struct domain *d;
> +            struct xen_domctl_createdomain d_cfg = {};
There are quite a few field in xen_domctl_createdomain that we may want 
to allow the user setting them. I am thinking of ssidref for XSM. How is 
this going to be done?
> +
> +            if ( !dt_device_is_compatible(node, "xen,domU") )
> +                continue;
> +
> +            d_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
Any reason to impose using the native GIC here?

> +            d_cfg.arch.nr_spis = gic_number_lines() - 32;
That's a bit unfortunate. So you are imposing to use 1020 IRQs and the 
waste of memory associated when only 32 SPIs is enough at the moment.
> +
> +            d = domain_create(max_init_domid++, &d_cfg);
> +            if ( IS_ERR(d))
Coding style ( ... )

> +                panic("Error creating domU");
> +
> +            d->is_privileged = 0;
> +            d->target = NULL;
Why do you set them? They are zeroed by default.

> +
> +            if ( construct_domU(d, node) != 0)
Coding style ( ... )

> +                printk("Could not set up DOMU guest OS");
> +
> +            domain_unpause_by_systemcontroller(d);
> +        }
> +    }
Please introduce a new function, this would avoid to increate 
start_xen() too much.

> +    discard_initial_modules();
> +
>       /* Switch on to the dynamically allocated stack for the idle vcpu
>        * since the static one we're running on is about to be freed. */
>       memcpy(idle_vcpu[0]->arch.cpu_info, get_cpu_info(),
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index e9f9905..578f3b9 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -56,6 +56,8 @@ struct bootinfo {
>   
>   extern struct bootinfo bootinfo;
>   
> +extern domid_t max_init_domid;
> +
>   void arch_init_memory(void);
>   
>   void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
> index 19232af..2fb9529 100644
> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -73,4 +73,6 @@ extern bool opt_dom0_shadow;
>   #endif
>   extern bool dom0_pvh;
>   
> +#define max_init_domid (1)
> +
>   #endif
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel