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

[Julien Grall <julien.grall@xxxxxxx>]

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(....);

> > > +    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 :).
Cheers,

--
Julien Grall

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