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

Re: [Xen-devel] [PATCH RFC 09/15] xen/arm: refactor construct_dom0



On Thu, 14 Jun 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 13/06/18 23:15, Stefano Stabellini wrote:
> > Move generic initializations out of construct_dom0 so that they can be
> > reused.
> > 
> > No functional changes in this patch.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> >   xen/arch/arm/domain_build.c | 124
> > ++++++++++++++++++++++++--------------------
> >   1 file changed, 67 insertions(+), 57 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4e4cd19..b31c563 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2092,73 +2092,27 @@ static void __init find_gnttab_region(struct domain
> > *d,
> >              kinfo->gnttab_start, kinfo->gnttab_start + kinfo->gnttab_size);
> >   }
> >   -int __init construct_dom0(struct domain *d)
> > +int __init __construct_domain(struct domain *d, struct kernel_info *kinfo)
> >   {
> > -    struct kernel_info kinfo = {};
> >       struct vcpu *saved_current;
> > -    int rc, i, cpu;
> > +    int i, cpu;
> >         struct vcpu *v = d->vcpu[0];
> >       struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
> >   -    /* Sanity! */
> > -    BUG_ON(d->domain_id != 0);
> > -    BUG_ON(d->vcpu[0] == NULL);
> > -    BUG_ON(v->is_initialised);
> 
> I think we want to keep the last two BUG_ON here. "d->vcpu[0] == NULL" is
> making sure that the value used when setting current is sane.
> 
> "v->is_initalised" is making sure this function is not called twice by
> mistake. Indeed the variable will be set at the end of __construct_domain.

OK, I'll make the change.


> > -
> > -    printk("*** LOADING DOMAIN 0 ***\n");
> > -    if ( dom0_mem <= 0 )
> > -    {
> > -        warning_add("PLEASE SPECIFY dom0_mem PARAMETER - USING 512M FOR
> > NOW\n");
> > -        dom0_mem = MB(512);
> > -    }
> > -
> > -
> > -    iommu_hwdom_init(d);
> > -
> > -    d->max_pages = ~0U;
> > -
> > -    kinfo.unassigned_mem = dom0_mem;
> > -    kinfo.d = d;
> > -
> > -    rc = kernel_probe(&kinfo);
> > -    if ( rc < 0 )
> > -        return rc;
> > -
> >   #ifdef CONFIG_ARM_64
> >       /* if aarch32 mode is not supported at EL1 do not allow 32-bit domain
> > */
> > -    if ( !(cpu_has_el1_32) && kinfo.type == DOMAIN_32BIT )
> > +    if ( !(cpu_has_el1_32) && kinfo->type == DOMAIN_32BIT )
> >       {
> >           printk("Platform does not support 32-bit domain\n");
> >           return -EINVAL;
> >       }
> > -    d->arch.type = kinfo.type;
> 
> Any reason to move this out?

Yeah, initially I left it there but it didn't work. It needs to be set
before calling allocate_memory() for domUs otherwise memory allocations
fail.


> >         if ( is_64bit_domain(d) )
> >           vcpu_switch_to_aarch64_mode(v);
> >     #endif
> >   -    allocate_memory(d, &kinfo);
> 
> I think this could stay here.

Same as before, if I leave it, it doesn't work. allocate_memory() needs
be called before prepare_dtb_domU.


> > -    find_gnttab_region(d, &kinfo);
> > -
> > -    /* Map extra GIC MMIO, irqs and other hw stuffs to dom0. */
> > -    rc = gic_map_hwdom_extra_mappings(d);
> > -    if ( rc < 0 )
> > -        return rc;
> > -
> > -    rc = platform_specific_mapping(d);
> > -    if ( rc < 0 )
> > -        return rc;
> > -
> > -    if ( acpi_disabled )
> > -        rc = prepare_dtb(d, &kinfo);
> > -    else
> > -        rc = prepare_acpi(d, &kinfo);
> 
> I think this is probably a call to add "hwdom" in the name of the 2 functions.

I'll do the rename in a later patch (and I'll use hwdom instead of dom0).


> > -
> > -    if ( rc < 0 )
> > -        return rc;
> > -
> >       /*
> >        * The following loads use the domain's p2m and require current to
> >        * be a vcpu of the domain, temporarily switch
> > @@ -2171,20 +2125,18 @@ int __init construct_dom0(struct domain *d)
> >        * kernel_load will determine the placement of the kernel as well
> >        * as the initrd & fdt in RAM, so call it first.
> >        */
> > -    kernel_load(&kinfo);
> > +    kernel_load(kinfo);
> >       /* initrd_load will fix up the fdt, so call it before dtb_load */
> > -    initrd_load(&kinfo);
> > -    dtb_load(&kinfo);
> > +    initrd_load(kinfo);
> > +    dtb_load(kinfo);
> >         /* Now that we are done restore the original p2m and current. */
> >       set_current(saved_current);
> >       p2m_restore_state(saved_current);
> >   -    discard_initial_modules();
> > -
> >       memset(regs, 0, sizeof(*regs));
> >   -    regs->pc = (register_t)kinfo.entry;
> > +    regs->pc = (register_t)kinfo->entry;
> >         if ( is_32bit_domain(d) )
> >       {
> > @@ -2202,14 +2154,14 @@ int __init construct_dom0(struct domain *d)
> >            */
> >           regs->r0 = 0; /* SBZ */
> >           regs->r1 = 0xffffffff; /* We use DTB therefore no machine id */
> > -        regs->r2 = kinfo.dtb_paddr;
> > +        regs->r2 = kinfo->dtb_paddr;
> >       }
> >   #ifdef CONFIG_ARM_64
> >       else
> >       {
> >           regs->cpsr = PSR_GUEST64_INIT;
> >           /* From linux/Documentation/arm64/booting.txt */
> > -        regs->x0 = kinfo.dtb_paddr;
> > +        regs->x0 = kinfo->dtb_paddr;
> >           regs->x1 = 0; /* Reserved for future use */
> >           regs->x2 = 0; /* Reserved for future use */
> >           regs->x3 = 0; /* Reserved for future use */
> > @@ -2235,6 +2187,64 @@ int __init construct_dom0(struct domain *d)
> >       return 0;
> >   }
> >   +int __init construct_dom0(struct domain *d)
> > +{
> > +    struct kernel_info kinfo = {};
> > +    int rc;
> > +
> > +    struct vcpu *v = d->vcpu[0];
> > +
> > +    /* Sanity! */
> > +    BUG_ON(d->domain_id != 0);
> > +    BUG_ON(d->vcpu[0] == NULL);
> > +    BUG_ON(v->is_initialised);
> > +
> > +    printk("*** LOADING DOMAIN 0 ***\n");
> > +    if ( dom0_mem <= 0 )
> > +    {
> > +        warning_add("PLEASE SPECIFY dom0_mem PARAMETER - USING 512M FOR
> > NOW\n");
> > +        dom0_mem = MB(512);
> > +    }
> > +
> > +
> 
> No need to copy the second newline :).

:-)


> > +    iommu_hwdom_init(d);
> > +
> > +    d->max_pages = ~0U;
> > +
> > +    kinfo.unassigned_mem = dom0_mem;
> > +    kinfo.d = d;
> > +
> > +    rc = kernel_probe(&kinfo);
> > +    if ( rc < 0 )
> > +        return rc;
> > +
> > +    allocate_memory(d, &kinfo);
> > +    find_gnttab_region(d, &kinfo);
> > +
> > +    /* Map extra GIC MMIO, irqs and other hw stuffs to dom0. */
> > +    rc = gic_map_hwdom_extra_mappings(d);
> > +    if ( rc < 0 )
> > +        return rc;
> > +
> > +    rc = platform_specific_mapping(d);
> > +    if ( rc < 0 )
> > +        return rc;
> > +
> > +    d->arch.type = kinfo.type;
> > +
> > +    if ( acpi_disabled )
> > +        rc = prepare_dtb(d, &kinfo);
> > +    else
> > +        rc = prepare_acpi(d, &kinfo);
> > +
> > +    if ( rc < 0 )
> > +        return rc;
> > +
> > +    discard_initial_modules();
> 
> You say "no functional change" in this patch. But this is one. The module are
> now discard much earlier. This imply that memory baking the Image/Initrd will
> be free to be re-used at any time.
>
> I don't think this is what we want. Unless you can promise no memory is
> allocated in __construct_domain().

discard_initial_modules() will be moved later by patch #14, but I think
it makes sense to call discard_initial_modules() after
__construct_domain() here.


> > +
> > +    return __construct_domain(d, &kinfo);
> > +}
> > +
> >   /*
> >    * Local variables:
> >    * mode: C
> > 
> 
> 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®.