Re: [Xen-devel] [PATCH v4 12/23] xen/arm: introduce create_domUs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 15 Oct 2018, Julien Grall wrote:
> Hi,
> 
> On 05/10/2018 19:47, 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.
> 
> s/xen,domU/xen,domain/

OK

> > Create new DomU VMs based on the information found on device tree under
> > "xen,domU". Calls construct_domU for each domain.
> 
> Ditto
> 
> s/Calls/Call/

OK


> > 
> > 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>
> > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: andrew.cooper3@xxxxxxxxxx
> > CC: jbeulich@xxxxxxxx
> > ---
> > Changes in v4:
> > - constify parameters
> > - nr_spis to 0 or  GUEST_VPL011_SPI - 32 + 1 depending on vpl011
> > - remove pointless initializer
> > - remove change to domain_create for dom0 (useless)
> > - make construct_domU return error
> > 
> > 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 | 51
> > +++++++++++++++++++++++++++++++++++++++++----
> >   xen/arch/arm/setup.c        |  5 +++++
> >   xen/include/asm-arm/setup.h |  3 +++
> >   xen/include/asm-x86/setup.h |  2 ++
> >   4 files changed, 57 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index ba3dad1..547b624 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>
> > @@ -2290,6 +2291,51 @@ static int __init __construct_domain(struct domain
> > *d, struct kernel_info *kinfo
> >       return 0;
> >   }
> >   +static int __init construct_domU(struct domain *d,
> > +                                 const struct dt_device_node *node)
> > +{
> > +    return -ENOSYS;
> > +}
> > +
> > +void __init create_domUs(void)
> > +{
> > +    struct dt_device_node *node;
> > +    const struct dt_device_node *chosen = dt_find_node_by_name(dt_host,
> > +                                                               "chosen");
> 
> Newline

OK


> It would also be better to use dt_find_node_by_path("/chosen") because
> dt_find_node_by_name may return a different node (e.g /foo/chosen).

Good idea


> > +    BUG_ON(chosen == NULL);
> > +    dt_for_each_child_node(chosen, node)
> > +    {
> > +        u32 len;
> > +        struct domain *d;
> > +        struct xen_domctl_createdomain d_cfg = {
> > +            .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> > +            .arch.nr_spis = 0,
> > +            .max_vcpus = 1,
> > +            .max_evtchn_port = -1,
> > +            .max_grant_frames = 64,
> > +            .max_maptrack_frames = 1024,
> > +        };
> > +
> > +        if ( !dt_device_is_compatible(node, "xen,domain") )
> > +            continue;
> > +
> > +        if ( dt_get_property(node, "vpl011", &len) != NULL )
> 
> dt_property_read_bool will do the right thing for you.

OK


> > +            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
> > +        dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus);
> > +
> > +        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;
> 
> is_console is a bool, so this should be true.

OK


> > +
> > +        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);
> > @@ -2342,10 +2388,7 @@ int __init construct_dom0(struct domain *d)
> >       if ( rc < 0 )
> >           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 d6d1546..8d8f180 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -64,8 +64,11 @@ static unsigned long opt_xenheap_megabytes __initdata;
> >   integer_param("xenheap_megabytes", opt_xenheap_megabytes);
> >   #endif
> >   +domid_t __read_mostly max_init_domid;
> > +
> >   static __used void init_done(void)
> >   {
> > +    discard_initial_modules();
> >       free_init_memory();
> >       startup_cpu_idle_loop();
> >   }
> > @@ -926,6 +929,8 @@ void __init start_xen(unsigned long boot_phys_offset,
> >       /* Must be done past setting system_state. */
> >       unregister_init_virtual_region();
> >   +    create_domUs();
> 
> I missed that bits in the previous version. I am not very sure this should be
> called after unregister_init_virtual_region(). If you call it after and get an
> hypervisor crash in init code, then the stack trace may miss some information
> (e.g bug frame).
> 
> Looking at x86, unregister_init_virtual_region() is called from init_done. So
> we probably want to move that call there.

OK



> > +
> >       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 177e8db..5620fe7 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -68,6 +68,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);
> > @@ -84,6 +86,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 42fddeb..1c80783 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