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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Jun 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/15/2018 12:35 AM, Stefano Stabellini wrote:
> > On Thu, 14 Jun 2018, Julien Grall wrote:
> > > On 13/06/18 23:15, Stefano Stabellini wrote:
> > > > -
> > > > -    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.
> 
> Oh because allocate_domain(d) rely on is_domain_32bit, right? I
> don't much like the duplication here just because of
> prepare_dtb_domU. I am wondering if we could do:
> 
> if ( !is_hardware_domain(d) )
>   prepare_dtb_domU(...);
> else if ( acpi_disabled )
>   prepare_acpi_hwdom(...);
> else
>   prepare_dt_hwdom(....);

The few remaining things in construct_dom0 and construct_domU are
different enough that I don't think there is much gain in trying to
merge them.


> > > > +    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.
> 
> Yeah, I noticed you moved the discard_initial_modules() later
> on. But I would like to have the series bisectable if possible
> :).

Yep, it will be done.

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