Re: [Xen-devel] [PATCH v3 13/25] xen/arm: introduce create_domUs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/08/18 00:27, 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. It holds the max domid among the initial
> > domains.
> > 
> > Move the discard_initial_modules after DomUs have been built.
> > 
> > First create domUs, then start dom0 -- no point in trying to start dom0
> > when the cpu is busy.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > CC: andrew.cooper3@xxxxxxxxxx
> > CC: jbeulich@xxxxxxxx
> > ---
> > Changes in v3:
> > - move patch earlier and introduce empty construct_domU to fix bisection
> >    builds
> > - fix max_init_domid to actually hold the max domid among initial
> >    domains (instead of max_domid + 1)
> > - move domain_unpause_by_systemcontroller(dom0) after creating domUs
> > 
> > Changes in v2:
> > - coding style
> > - set nr_spis to 32
> > - introduce create_domUs
> > ---
> >   xen/arch/arm/domain_build.c | 42
> > +++++++++++++++++++++++++++++++++++++++---
> >   xen/arch/arm/setup.c        |  7 ++++++-
> >   xen/include/asm-arm/setup.h |  3 +++
> >   xen/include/asm-x86/setup.h |  2 ++
> >   4 files changed, 50 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 0835340..8f7ac54 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -7,6 +7,7 @@
> >   #include <asm/irq.h>
> >   #include <asm/regs.h>
> >   #include <xen/errno.h>
> > +#include <xen/err.h>
> >   #include <xen/device_tree.h>
> >   #include <xen/libfdt/libfdt.h>
> >   #include <xen/guest_access.h>
> > @@ -2205,6 +2206,43 @@ static int __init __construct_domain(struct domain
> > *d, struct kernel_info *kinfo
> >       return 0;
> >   }
> >   +static int __init construct_domU(struct domain *d, struct dt_device_node
> > *node)
> 
> AFAICT, the node is not going to be modified. So it should be const here.

OK


> > +{
> > +    return 0;
> 
> As Jan said, this should return an error until it is been implemented.

OK


> > +}
> > +
> > +void __init create_domUs(void)
> > +{
> > +    struct dt_device_node *node;
> > +    struct dt_device_node *chosen = dt_find_node_by_name(dt_host,
> > "chosen");
> 
> Same here, you want them to be const.

OK


> > +
> > +    if ( chosen != NULL )
> 
> I would prefer if we don't introduce another indentation level by inverting
> the condition.
> 
> Also, it is very unlikely to not have the chosen node. So it might be better
> to turn this into a panic/BUG_ON.

OK


> > +    {
> > +        dt_for_each_child_node(chosen, node)
> > +        {
> > +            struct domain *d;
> > +            struct xen_domctl_createdomain d_cfg = {
> > +                .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> > +                .arch.nr_spis = 32,
> > +            };
> > +
> > +            if ( !dt_device_is_compatible(node, "xen,domain") )
> > +                continue;
> > +
> > +            d = domain_create(++max_init_domid, &d_cfg, false);
> > +            if ( IS_ERR(d) )
> > +                panic("Error creating domain %s", dt_node_name(node));
> > +
> > +            d->is_console = 1;
> > +
> > +            if ( construct_domU(d, node) != 0 )
> > +                panic("Could not set up domain %s", dt_node_name(node));
> > +
> > +            domain_unpause_by_systemcontroller(d);
> > +        }
> > +    }
> > +}
> > +
> >   int __init construct_dom0(struct domain *d)
> >   {
> >       const struct bootcmdline *kernel =
> > boot_cmdline_find_by_kind(BOOTMOD_KERNEL);
> > @@ -2258,9 +2296,7 @@ int __init construct_dom0(struct domain *d)
> >           return rc;
> >     -    rc = __construct_domain(d, &kinfo);
> > -    discard_initial_modules();
> > -    return rc;
> > +    return __construct_domain(d, &kinfo);
> >   }
> >     /*
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index c320df9..096484f 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -63,8 +63,11 @@ 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)
> >   {
> > +    discard_initial_modules();
> >       free_init_memory();
> >       startup_cpu_idle_loop();
> >   }
> > @@ -894,7 +897,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, true);
> > +    dom0 = domain_create(max_init_domid, &dom0_cfg, true);
> >       if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> >               panic("Error creating domain 0");
> >   @@ -915,6 +918,8 @@ void __init start_xen(unsigned long boot_phys_offset,
> >       /* Must be done past setting system_state. */
> >       unregister_init_virtual_region();
> >   +    create_domUs();
> > +
> >       domain_unpause_by_systemcontroller(dom0);
> >         /* Switch on to the dynamically allocated stack for the idle vcpu
> > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> > index 4551f72..d9cd8e1 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -67,6 +67,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);
> > @@ -83,6 +85,7 @@ void acpi_create_efi_mmap_table(struct domain *d,
> >   int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
> >     int construct_dom0(struct domain *d);
> > +void __init create_domUs(void);
> >     void discard_initial_modules(void);
> >   void dt_unreserved_regions(paddr_t s, paddr_t e,
> > diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
> > index 8d4b9ee..c0bd86f 100644
> > --- a/xen/include/asm-x86/setup.h
> > +++ b/xen/include/asm-x86/setup.h
> > @@ -66,4 +66,6 @@ extern bool opt_dom0_shadow;
> >   #endif
> >   extern bool dom0_pvh;
> >   +#define max_init_domid (0)
> > +
> >   #endif
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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