Re: [Xen-devel] [PATCH v2 19/21] xen/arm: introduce create_domUs
On Mon, 30 Jul 2018, Julien Grall wrote: > Hi Stefano, > > On 28/07/18 03:42, Stefano Stabellini wrote: > > On Tue, 24 Jul 2018, Julien Grall wrote: > > > > + > > > > +domain_unpause_by_systemcontroller(d); > > > > > > If a domain is bound to CPU0, then it will not boot until CPU0 is done > > > with > > > creating domain. Is that what you want? > > > > Are you suggesting to move the domain_unpause_by_systemcontroller(d) to > > a second loop after the domU creation loop? > > I was trying to understand what are the pro/cons of unpausing a DomU here. Did > you explore it? OK, let's dig into it. domain_unpause_by_systemcontroller doesn't actually trigger a DomU boot right away. It only marks the vcpu as ready to be started. Then, it is up to Xen wanting to leave hypervisor mode and start running a guest vcpu. So there should be no issues in unpausing the vcpus here, in fact, it should make no difference whether it is done inside this loop or on a second loop. In my experiments, cpu1 starts first, running dom1v0, then cpu0 (after completely start_xen) running dom0v0. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] xen/arm: introduce create_domUs
Hi Stefano, On 28/07/18 03:42, Stefano Stabellini wrote: On Tue, 24 Jul 2018, Julien Grall wrote: + +domain_unpause_by_systemcontroller(d); If a domain is bound to CPU0, then it will not boot until CPU0 is done with creating domain. Is that what you want? Are you suggesting to move the domain_unpause_by_systemcontroller(d) to a second loop after the domU creation loop? I was trying to understand what are the pro/cons of unpausing a DomU here. Did you explore it? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] xen/arm: introduce create_domUs
On Tue, 24 Jul 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 07/07/18 00:12, Stefano Stabellini wrote:
> > Call a new function, "create_domUs", from setup_xen to start DomU VMs.
> >
> > 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". Calls construct_domU for each domain.
> >
> > Introduce a simple global variable named max_init_domid to keep track of
> > the initial allocated domids.
> >
> > Move the discard_initial_modules after DomUs have been built
>
> Nit: Missing full stop.
OK
> >
> > Signed-off-by: Stefano Stabellini
> > CC: andrew.coop...@citrix.com
> > CC: jbeul...@suse.com
> > ---
> > Changes in v2:
> > - coding style
> > - set nr_spis to 32
> > - introduce create_domUs
> > ---
> > xen/arch/arm/domain_build.c | 38 +++---
> > xen/arch/arm/setup.c| 8 +++-
> > xen/include/asm-arm/setup.h | 3 +++
> > xen/include/asm-x86/setup.h | 2 ++
> > 4 files changed, 47 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d7e9040..9f58002 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -7,6 +7,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -2542,6 +2543,39 @@ static int __init construct_domU(struct domain *d,
> > struct dt_device_node *node)
> > return rc;
> > }
> > +void __init create_domUs(void)
> > +{
> > +struct dt_device_node *node;
> > +struct dt_device_node *chosen = dt_find_node_by_name(dt_host,
> > "chosen");
>
> newline here.
OK
> > +if ( chosen != NULL )
> > +{
> > +dt_for_each_child_node(chosen, node)
> > +{
> > +struct domain *d;
> > +struct xen_domctl_createdomain d_cfg = {};
> > +
> > +if ( !dt_device_is_compatible(node, "xen,domain") )
> > +continue;
> > +
> > +d_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> > +d_cfg.arch.nr_spis = 32;
>
> You can set those fields directly when defining d_cfg above.
OK
> > +
> > +d = domain_create(max_init_domid++, _cfg);
> > +if ( IS_ERR(d) )
> > +panic("Error creating domU");
>
> It is probably worth to add the node name in the message. So the user knows
> which guest failed.
OK
> > +
> > +d->is_privileged = 0;
> > +d->is_console = 1;
> > +d->target = NULL;
> > +
> > +if ( construct_domU(d, node) != 0 )
> > +printk("Could not set up DOMU guest OS");
>
> Should not it be a panic here? Also, the message is a little odd "DOMU guest"
> is a bit redundant and the function will load the kernel but that's not the
> only thing done.
>
> Lastly, you probably want to add the node name in the message, so the user
> knows which guest failed.
OK to all suggestions
> > +
> > +domain_unpause_by_systemcontroller(d);
>
> If a domain is bound to CPU0, then it will not boot until CPU0 is done with
> creating domain. Is that what you want?
Are you suggesting to move the domain_unpause_by_systemcontroller(d) to
a second loop after the domU creation loop?
> > +}
> > +}
> > +}
> > +
> > int __init construct_dom0(struct domain *d)
> > {
> > struct kernel_info kinfo = {};
> > @@ -2592,9 +2626,7 @@ int __init construct_dom0(struct domain *d)
> > return rc;
> > -rc = __construct_domain(d, );
> > -discard_initial_modules();
> > -return rc;
> > +return __construct_domain(d, );
> > }
> > /*
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 7739a80..0b08af2 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -64,6 +64,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();
> > @@ -863,7 +865,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, _cfg);
> > +dom0 = domain_create(max_init_domid++, _cfg);
> > if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> > panic("Error creating domain 0");
> > @@ -889,6 +891,10 @@ void __init start_xen(unsigned long boot_phys_offset,
> > domain_unpause_by_systemcontroller(dom0);
>
> Why do you unpause Dom0 and then create the guests? It feels like to me you
> want to create all the guests and then unpause dom0. dom0 would likely get
> blocked anyway has CPU0 will be busy creating domains.
Right, I'll do that
> > +
Re: [Xen-devel] [PATCH v2 19/21] xen/arm: introduce create_domUs
Hi Stefano,
On 07/07/18 00:12, Stefano Stabellini wrote:
Call a new function, "create_domUs", from setup_xen to start DomU VMs.
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". Calls construct_domU for each domain.
Introduce a simple global variable named max_init_domid to keep track of
the initial allocated domids.
Move the discard_initial_modules after DomUs have been built
Nit: Missing full stop.
Signed-off-by: Stefano Stabellini
CC: andrew.coop...@citrix.com
CC: jbeul...@suse.com
---
Changes in v2:
- coding style
- set nr_spis to 32
- introduce create_domUs
---
xen/arch/arm/domain_build.c | 38 +++---
xen/arch/arm/setup.c| 8 +++-
xen/include/asm-arm/setup.h | 3 +++
xen/include/asm-x86/setup.h | 2 ++
4 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d7e9040..9f58002 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -2542,6 +2543,39 @@ static int __init construct_domU(struct domain *d,
struct dt_device_node *node)
return rc;
}
+void __init create_domUs(void)
+{
+struct dt_device_node *node;
+struct dt_device_node *chosen = dt_find_node_by_name(dt_host, "chosen");
newline here.
+if ( chosen != NULL )
+{
+dt_for_each_child_node(chosen, node)
+{
+struct domain *d;
+struct xen_domctl_createdomain d_cfg = {};
+
+if ( !dt_device_is_compatible(node, "xen,domain") )
+continue;
+
+d_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
+d_cfg.arch.nr_spis = 32;
You can set those fields directly when defining d_cfg above.
+
+d = domain_create(max_init_domid++, _cfg);
+if ( IS_ERR(d) )
+panic("Error creating domU");
It is probably worth to add the node name in the message. So the user
knows which guest failed.
+
+d->is_privileged = 0;
+d->is_console = 1;
+d->target = NULL;
+
+if ( construct_domU(d, node) != 0 )
+printk("Could not set up DOMU guest OS");
Should not it be a panic here? Also, the message is a little odd "DOMU
guest" is a bit redundant and the function will load the kernel but
that's not the only thing done.
Lastly, you probably want to add the node name in the message, so the
user knows which guest failed.
+
+domain_unpause_by_systemcontroller(d);
If a domain is bound to CPU0, then it will not boot until CPU0 is done
with creating domain. Is that what you want?
+}
+}
+}
+
int __init construct_dom0(struct domain *d)
{
struct kernel_info kinfo = {};
@@ -2592,9 +2626,7 @@ int __init construct_dom0(struct domain *d)
return rc;
-rc = __construct_domain(d, );
-discard_initial_modules();
-return rc;
+return __construct_domain(d, );
}
/*
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7739a80..0b08af2 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -64,6 +64,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();
@@ -863,7 +865,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, _cfg);
+dom0 = domain_create(max_init_domid++, _cfg);
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
panic("Error creating domain 0");
@@ -889,6 +891,10 @@ void __init start_xen(unsigned long boot_phys_offset,
domain_unpause_by_systemcontroller(dom0);
Why do you unpause Dom0 and then create the guests? It feels like to me
you want to create all the guests and then unpause dom0. dom0 would
likely get blocked anyway has CPU0 will be busy creating domains.
+create_domUs();
+
+discard_initial_modules();
I think it would be better to move that in init_done. This is where all
initial memory is freed.
+
/* 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 5ecfe27..21b9729 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);
Re: [Xen-devel] [PATCH v2 19/21] xen/arm: introduce create_domUs
On Mon, 16 Jul 2018, Jan Beulich wrote: > >>> On 07.07.18 at 01:12, wrote: > > --- 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) > > Why is this 1 rather than 0? Or is the name imprecise? Yeah, the name is imprecise. On x86 there is always only dom0 at boot, so this variable is set to 0+1 = 1. I can change the variable to actually match the name, so that on a system with only dom0 at boot, max_init_domid would be (0). It makes more sense that way. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] xen/arm: introduce create_domUs
>>> On 07.07.18 at 01:12, wrote: > --- 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) Why is this 1 rather than 0? Or is the name imprecise? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
