Re: [Xen-devel] [PATCH v4 13/23] xen/arm: implement construct_domU

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 15 Oct 2018, Julien Grall wrote:
> Hi,
> 
> On 05/10/2018 19:47, 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.
> > 
> > Add const to kernel_probe dt_device_node parameter.
> 
> This likely belongs to patch #7 where the parameter was added.

OK


> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > ---
> > Changes in v4:
> > - constify kernel_probe
> > - change title
> > - better error messages and printed info
> > - 64bit memory
> > 
> > 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 | 37 ++++++++++++++++++++++++++++++++++---
> >   xen/arch/arm/kernel.c       |  3 ++-
> >   xen/arch/arm/kernel.h       |  2 +-
> >   3 files changed, 37 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 547b624..efb530a 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -369,7 +369,6 @@ static void __init allocate_memory_11(struct domain *d,
> >       }
> >   }
> >   -#if 0
> 
> Please add a word about this change in the commit message.

OK


> >   static bool __init allocate_bank_memory(struct domain *d,
> >                                           struct kernel_info *kinfo,
> >                                           gfn_t sgfn,
> > @@ -450,7 +449,6 @@ fail:
> >               (unsigned long)kinfo->unassigned_mem >> 10);
> >       BUG();
> >   }
> > -#endif
> >     static int __init write_properties(struct domain *d, struct kernel_info
> > *kinfo,
> >                                      const struct dt_device_node *node)
> > @@ -2294,7 +2292,40 @@ static int __init __construct_domain(struct domain
> > *d, struct kernel_info *kinfo
> >   static int __init construct_domU(struct domain *d,
> >                                    const struct dt_device_node *node)
> >   {
> > -    return -ENOSYS;
> > +    struct kernel_info kinfo = {};
> > +    int rc;
> > +    u64 mem;
> > +
> > +    rc = dt_property_read_u64(node, "memory", &mem);
> > +    if ( !rc )
> > +    {
> > +        printk("Error building DomU: cannot read \"memory\" property\n");
> > +        return -EINVAL;
> > +    }
> > +    kinfo.unassigned_mem = (paddr_t)mem << 10;
> 
> I noticed I forgot to answer to:
> "KB() only works for numbers, it is defined as: (_AC(_kb, ULL) << 10)"
> 
> unsigned long long is always going to be bigger than paddr_t. Also, we already
> use MB(...) in similar situation. So I am not sure to understand your concern
> here.

I admit that my explanation was so bad that even I had to go back to
figure out what I meant :-)

What I wanted to say is that KB() only works for constants, not
variables. So KB(10) works, but KB(mem) does not.


> > +
> > +    printk("*** LOADING DOMU cpus=%u memory=%luKB ***\n", d->max_vcpus,
> > mem);
> > +
> > +    d->vcpu = xzalloc_array(struct vcpu *, d->max_vcpus);
> > +    if ( !d->vcpu )
> > +        return -ENOMEM;;
> 
> d->vcpu is already allocated by domain_create.

OK


> > +    if ( vcpu_create(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)
> > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index e5b8213..2239a07 100644
> > --- a/xen/arch/arm/kernel.c
> > +++ b/xen/arch/arm/kernel.c
> > @@ -421,7 +421,8 @@ static int __init kernel_zimage32_probe(struct
> > kernel_info *info,
> >       return 0;
> >   }
> >   -int __init kernel_probe(struct kernel_info *info, struct dt_device_node
> > *domain)
> > +int __init kernel_probe(struct kernel_info *info,
> > +                        const struct dt_device_node *domain)
> >   {
> >       struct bootmodule *mod = NULL;
> >       struct bootcmdline *cmd = NULL;
> > diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> > index 4a65289..4320f72 100644
> > --- a/xen/arch/arm/kernel.h
> > +++ b/xen/arch/arm/kernel.h
> > @@ -55,7 +55,7 @@ struct kernel_info {
> >    *  ->type
> >    *  ->load hook, and sets loader specific variables ->zimage
> >    */
> > -int kernel_probe(struct kernel_info *info, struct dt_device_node *domain);
> > +int kernel_probe(struct kernel_info *info, const struct dt_device_node
> > *domain);
> >     /*
> >    * Loads the kernel into guest RAM.
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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