Re: [Xen-devel] [PATCH V2] xen/arm: domain_build: allocate lowmem for dom0 as much as possible

[Julien Grall <julien.grall@xxxxxxx>]

On 20/09/16 10:10, Peng Fan wrote:
> Hello Julien,
Hello Peng,

> On Tue, Sep 20, 2016 at 10:36:27AM +0200, Julien Grall wrote:
> > Hello Peng,
> >
> > On 20/09/2016 07:52, van.freenix@xxxxxxxxx wrote:
> > > From: Peng Fan <peng.fan@xxxxxxx>
> > >
> > > On AArch64 SoCs, some IPs may only have the capability to access
> > > 32bits address space. The physical memory assigned for Dom0 maybe
> > s/32bits/32 bits/
> Fix in V3.
>
> > > not in 4GB address space, then the IPs will not work properly.
> > > So need to allocate memory under 4GB for Dom0.
> > >
> > > There is no restriction that how much lowmem needs to be allocated for
> > > Dom0. Dom0 now use 1:1 mapping, but DomU does not use 1:1 mapping,
> > > there is no need to reserve lowmem for DomU, so allocate lowmem as much
> > > as possible for Dom0.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Cc: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > >
> > > This patch is to resolve the issue mentioned in
> > > https://lists.xen.org/archives/html/xen-devel/2016-09/msg00235.html
> > >
> > > V2:
> > > Remove the bootargs dom0_lowmem introduced in V1.
> > > Following 
> > > "https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01459.html";
> > > to allocate as much as possible lowmem.
> > >
> > > Tested results:
> > > (XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
> > > (XEN) BANK[0] 0x00000088000000-0x000000f8000000 (1792MB)
> > > (XEN) BANK[1] 0x000009e0000000-0x000009f0000000 (256MB)
> > > 1792M allocated in 4GB address space.
> > >
> > > xen/arch/arm/domain_build.c | 20 +++++++++++++-------
> > > 1 file changed, 13 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 35ab08d..0b9b85c 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct 
> > > kernel_info *kinfo)
> > As mentioned on V1, please update the comment above this function.
> About this part:
> * 3. For 32-bit dom0 we want to place as much of the RAM as we
> *    reasonably can below 4GB, so that it can be used by non-LPAE
> *    enabled kernels.
>
> Changed to this V3:
> For dom0 we want to place as much of the RAM as we reasonably can
> below 4GB, so the devices have the limitation to access 64 bits
> address space can work properly and it can also be used by
> non-LPAE enabled kernels for 32-bit dom0.
>
> About
> "
>  * For 32-bit domain we require that the initial allocation for the
>  * first bank is under 4G.
> "
>
> Changed to this in V3:
>
> "
> For 32-bit domain we require that the initial allocation for the
> first bank is under 4G. For 64-bit domain, the first bank is preferred
> to be allocated under 4GB.
> "
>
> Is this ok?
I think so.

> > >     const unsigned int min_low_order =
> > >         get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128)));
> > >     const unsigned int min_order = get_order_from_bytes(MB(4));
> > > -    struct page_info *pg;
> > > +    struct page_info *pg = NULL;
> > >     unsigned int order = get_11_allocation_size(kinfo->unassigned_mem);
> > >     int i;
> > >
> > > -    bool_t lowmem = is_32bit_domain(d);
> > > +    bool_t lowmem = true;
> > >     unsigned int bits;
> > >
> > >     /*
> > > @@ -265,22 +265,28 @@ static void allocate_memory(struct domain *d, struct 
> > > kernel_info *kinfo)
> > >      */
> > >     while ( order >= min_low_order )
> > >     {
> > > -        for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ )
> > > +        for ( bits = order ; bits <= 32 ; bits++ )
> > >         {
> > >             pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
> > >             if ( pg != NULL )
> > > +            {
> > > +                if ( !insert_11_bank(d, kinfo, pg, order) )
> > > +                    BUG(); /* Cannot fail for first bank */
> > > +
> > >                 goto got_bank0;
> > > +           }
> > >         }
> > >         order--;
> > >     }
> > >
> > > -    panic("Unable to allocate first memory bank");
> > > +    if ( pg == NULL )
> > > +    {
> > > +        printk(XENLOG_INFO "No bank has been allocated below 4GB.\n");
> > > +        lowmem = false;
> > > +    }
> > This new behavior should be documented in the commit message, with some
> > rationale why it is fine to do it for ARM32.
> Consider the comments of the function:
>   1. The dom0 kernel should be loaded within the first 128MB of RAM. This
>      is necessary at least for Linux zImage kernels, which are all we
>      support today.
>  4. For 32-bit dom0 the kernel must be located below 4GB.
>
> I think I may need to add back the panic when first bank is failed to be 
> allocated
> under 4GB.
It occurred to me that the first bank below 4GB is a mandatory for 
32-bit domain, because the kernel is booted with paging disabled.
> Will use the following code in V3 and document this in commit log.
>     if ( pg == NULL )
>     {
>         if ( is_32bit_domain(d) )
>             panic("Unable to allocate first memory bank");
>
>         printk(XENLOG_INFO "No bank has been allocated below 4GB.\n");
>         lowmem = false;
>     }
>
>
> > >  got_bank0:
> > >
> > > -    if ( !insert_11_bank(d, kinfo, pg, order) )
> > > -        BUG(); /* Cannot fail for first bank */
> > > -
> > >     /* Now allocate more memory and fill in additional banks */
> > This comment should be updated.
> Add more comments? I did not modify code below the comment and the comment
> is correct I think.
Well, you can reach here with the bank 0 not allocated, so "now allocate 
more memory..." is inaccurate for me. When you do change, you also need 
to see whether it fits with the rest of the function.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel