[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?



        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?



        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.



        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?




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.




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.

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

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.