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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 9 Jul 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/07/18 00:11, Stefano Stabellini wrote:
> > 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.
> 
> What will happen with libxl today? Is the toolstack going to be confused?

The toolstack can list the running domains without problems with `xl
list' (although they have no name). Also, xl vcpu-pin works fine.
However, some operations might not work correctly though. For instance,
`xl destroy' cannot completely get rid of the domain. I'll add info
about these limitations to a separate dom0less document (as you also
suggested below), because it is not part of the multiboot spec.


> > 
> > 
> > >   2) Is it possible to restart those domains?
> > 
> > No they can't be restarted. For example, their original kernel is gone
> > from memory.
> 
> So what will happen if you use "xl restart" on them?

Do you mean `xl reboot'?  The command executes but nothing happens.


> > 
> > 
> > >   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.
> 
> That should be clarified somewhere in the documentation.

Yes, you are right. I'll add this to the new dom0less doc. 


> > 
> > 
> > >   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).
> 
> Can you explain how the vCPUs are going to get pinned via the command line?

Today, only dom0 vCPUs can get automatically pinned with a Xen command
line option. However, `xl vcpu-pin' in dom0 works with other domains
started by Xen at boot, and the NULL scheduler doesn't require pinning.
FYI for my own usage, I plan to use the NULL scheduler.


> > 
> > 
> > > > 
> > > > 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.
> 
> I don't think this is necessary. I will reply on the second version.

OK


> > 
> > 
> > > > 
> > > > 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"
> 
> I missed that sorry.
> 
> > 
> > 
> > > >        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?
> 
> I didn't ask the implementation but how this is going to fit together.
> 
> > > > +
> > > > +            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.
> 
> Please document it in somewhere.

Yes, I'll also add this to the separate dom0less document.


> > > > +
> > > > +            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.
> 
> I would rather remove it because it give the feeling the other fields may not
> be zeroed.

OK

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