RE: [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi 

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, July 3, 2021 9:26 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH 8/9] xen/arm: check `xen,static-mem` property during
> domain construction
> 
> Hi Penny,
> 
> On 07/06/2021 03:43, Penny Zheng wrote:
> > This commit checks `xen,static-mem` device tree property in /domUx
> > node, to determine whether domain is on Static Allocation, when
> > constructing domain during boot-up.
> >
> > Right now, the implementation of allocate_static_memory is missing,
> > and will be introduced later. It just BUG() out at the moment.
> >
> > And if the `memory` property and `xen,static-mem` are both set, it
> > shall be verified that if the memory size defined in both is consistent.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> > changes v2:
> > - remove parsing procedure here
> > - check the consistency when `xen,static-mem` and `memory` are both
> > defined
> > ---
> >   xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++---
> ---
> >   1 file changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 282416e74d..4166d7993c 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2424,23 +2424,47 @@ static int __init construct_domU(struct domain
> *d,
> >   {
> >       struct kernel_info kinfo = {};
> >       int rc;
> > -    u64 mem;
> > +    u64 mem, static_mem_size = 0;
> > +    const struct dt_property *prop;
> > +    bool static_mem = false;
> > +
> > +    d->max_pages = ~0U;
> > +    /*
> > +     * Guest RAM could be of static memory from static allocation,
> > +     * which will be specified through "xen,static-mem" phandle.
> > +     */
> > +    prop = dt_find_property(node, "xen,static-mem", NULL);
> > +    if ( prop )
> > +    {
> > +        static_mem = true;
> > +        /* static_mem_size = allocate_static_memory(...); */
> > +        BUG();
> > +    }
> 
> I would prefer if the static memory is allocated close to
> allocate_memory() below. AFAICT, the reason you allocate here is because you
> want to have the property "memory" optional.
> 
> However, I am not entirely convinced this is a good idea to make optional. It
> would be easier for a reader to figure out from the device-tree how much
> memory we give to the guest.
> 

Hmmm, now I think maybe I understand wrongly what you suggested in v1.
"
Could we allocate the memory as we parse?
"
I just simply think it means the code sequence, putting allocation immediately
after parsing. ;/

Re-investigating the docs on "memory":

"
- memory

    A 64-bit integer specifying the amount of kilobytes of RAM to
    allocate to the guest.
"
If you prefer "memory" mandate, then tbh, it will make the code
here more easily-read, no ifs.
But maybe I shall put more info on docs to clarify that even though user using
"xen, static-mem" to refer to static memory allocation, they shall still use
"memory" property to tell how much memory we give to the guest.

> >
> >       rc = dt_property_read_u64(node, "memory", &mem);
> > -    if ( !rc )
> > +    if ( !static_mem && !rc )
> >       {
> >           printk("Error building DomU: cannot read \"memory\" property\n");
> >           return -EINVAL;
> > +    } else if ( rc && static_mem )
> > +    {
> > +        if ( static_mem_size != mem * SZ_1K )
> > +        {
> > +            printk("Memory size in \"memory\" property isn't consistent 
> > with"
> > +                   "the ones defined in \"xen,static-mem\".\n");
> > +            return -EINVAL;
> > +        }
> >       } > -    kinfo.unassigned_mem = (paddr_t)mem * SZ_1K;
> > +    kinfo.unassigned_mem = static_mem ? 0 : (paddr_t)mem * SZ_1K; >
> > -    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n", d-
> >max_vcpus, mem);
> > +    printk("*** LOADING DOMU cpus=%u memory=%"PRIx64"KB ***\n",
> > +            d->max_vcpus,
> > +            static_mem ? static_mem_size : (kinfo.unassigned_mem) >>
> > + 10);
> 
> 
> If we mandate the property "memory", then kinfo.unassigned_mem doesn't
> need to be touched. Instead, you could simply check the
> kinfo.unassigned_mem is equivalent to static_mem_size.
> 

True, true. 

> >
> >       kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
> >
> >       if ( vcpu_create(d, 0) == NULL )
> >           return -ENOMEM;
> > -    d->max_pages = ~0U;
> >
> >       kinfo.d = d;
> >
> > @@ -2452,7 +2476,8 @@ static int __init construct_domU(struct domain *d,
> >       /* type must be set before allocate memory */
> >       d->arch.type = kinfo.type;
> >   #endif
> > -    allocate_memory(d, &kinfo);
> > +    if ( !static_mem )
> > +        allocate_memory(d, &kinfo);
> >
> >       rc = prepare_dtb_domU(d, &kinfo);
> >       if ( rc < 0 )
> >
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng