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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Jun 2018, Julien Grall wrote:
> 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?

Yes, they are. There is an argument for making them "hidden" from
DOMCTLs, but that is not done as part of this series. Future work.


>       2) Is it possible to restart those domains?

No they can't be restarted. For example, their original kernel is gone
from memory.


>       3) If a domain crash, what will happen? Are they just going to  sit
> there using resources until the platform rebooted?

The entire platform needs to be rebooted.


>       4) How do you handle scheduling? Is it still possible to do it via
> Dom0? How about the dom0less situation?

The scheduler can be chosen via the Xen command line option. It is also
possible to do that from dom0 (if there is a dom0).


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

The goal of this variable is to know the number of domUs allocated at
boot time by Xen. Specifically, it is used by console.c to switch the
serial input to the right domU.


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

I'll add them


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

It is mentioned: "Move the discard_initial_modules after DomUs have been
built"


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

We'll introduce a separate function to initialize the
xen_domctl_createdomain fields as necessary. I don't think it is
required at the moment?


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

It is a good default. I haven't introduced an option to change the gic
version for a domU yet. It could be possible to add it in the future.


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

Well spotted, I'll fix


> > +
> > +            d = domain_create(max_init_domid++, &d_cfg);
> > +            if ( IS_ERR(d))
> 
> Coding style ( ... )

I'll fix


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

Mostly for my own clarity and understading. I can remove them if you
prefer.


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

OK


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

Yes, I'll do


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