Re: [PATCH v3 14/25] xen/arm: introduce construct_domU

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi,
> 
> Title: You don't really introduce "construct_domU". You implement it. So a
> better title would be "Implement construct_domU".

OK


> On 01/08/18 00:27, Stefano Stabellini wrote:
> > Similar to construct_dom0, construct_domU creates a barebone DomU guest.
> > 
> > The device tree node passed as argument is compatible "xen,domain", see
> > docs/misc/arm/device-tree/booting.txt.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > Changes in v3:
> > - move setting type before allocate_memory
> > - add ifdef around it and a comment
> > 
> > Changes in v2:
> > - rename mem to memory
> > - make cpus and memory mandatory
> > - remove wront comment from commit message
> > - cpus and memory are read as integers
> > - read the vpl011 option
> > ---
> >   xen/arch/arm/domain_build.c | 36 +++++++++++++++++++++++++++++++++++-
> >   1 file changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 8f7ac54..101cca2 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2208,7 +2208,41 @@ static int __init __construct_domain(struct domain
> > *d, struct kernel_info *kinfo
> >     static int __init construct_domU(struct domain *d, struct dt_device_node
> > *node)
> >   {
> > -    return 0;
> > +    struct kernel_info kinfo = {};
> > +    int rc;
> > +    u32 mem;
> > +
> > +    printk("*** LOADING DOMU ***\n");
> > +
> > +    rc = dt_property_read_u32(node, "cpus", &d->max_vcpus);
> > +    if ( !rc )
> > +        return -EINVAL;
> 
> For a user PoV this will be difficult to differentiate between the -EINVAL
> here and the only below.
> 
> It is probably useful to log some error message. Similarly you might want to
> add some print telling the information about the domain created.

OK


> > +
> > +    rc = dt_property_read_u32(node, "memory", &mem);
> 
> I don't think 32-bit integer is going to be enough to cover all the case.
> Nowadays Arm supports up to 52-bits Address-Space.
> 
> So it would be best to cover all the case and use 64-bit here. That would also
> mean updating the documentation.

We need to support the whole address space, but unfortunately it means
have to add one more cell to the memory attribute, becoming less
intuitive to write. Even if we switched from KB to MB for the unit, we
would still need two cells from my calculations.

Are you sure I cannot convince you to switch to a string format for the
memory attribute where users can specify the unit (KB, MB, GB, etc.),
like they do today with the mem= command line?


> > +    if ( !rc ) > +        return -EINVAL;
> > +    kinfo.unassigned_mem = (paddr_t)mem << 10;
> 
> kinfo.unassigned_mem = KB(mem);

KB() only works for numbers, it is defined as: (_AC(_kb, ULL) << 10)


> > +
> > +    d->vcpu = xzalloc_array(struct vcpu *, d->max_vcpus);
> > +    if ( !d->vcpu )
> > +        return -ENOMEM;;
> > +    if ( alloc_vcpu(d, 0, 0) == NULL )
> > +        return -ENOMEM;
> > +    d->max_pages = ~0U;
> > +
> > +    kinfo.d = d;
> > +
> > +    rc = kernel_probe(&kinfo, node);
> > +    if ( rc < 0 )
> > +        return rc;
> > +
> > +#ifdef CONFIG_ARM_64
> > +    /* type must be set before allocate memory */
> > +    d->arch.type = kinfo.type;
> > +#endif
> > +    allocate_memory(d, &kinfo);
> > +
> > +    return __construct_domain(d, &kinfo);
> >   }
> >     void __init create_domUs(void)

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