Re: [Xen-devel] [PATCH v4 10/23] xen/arm: introduce allocate_memory

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 10/19/18 11:28 PM, Stefano Stabellini wrote:
> On Mon, 15 Oct 2018, Julien Grall wrote:
> > Hi,
> >
> > On 05/10/2018 19:47, Stefano Stabellini wrote:
> > > Introduce an allocate_memory function able to allocate memory for DomUs
> > > and map it at the right guest addresses, according to the guest memory
> > > map: GUEST_RAM0_BASE and GUEST_RAM1_BASE.
> > Please add something along the line: "This is under #if 0 as not used for
> > now".
> I'll add
>
>
> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > ---
> > > Changes in v4:
> > > - move earlier, add #if 0
> > > - introduce allocate_bank_memory, remove insert_bank
> > > - allocate_bank_memory allocate memory and inserts the bank, while
> > >     allocate_memory specifies where to do that
> > >
> > > Changes in v3:
> > > - new patch
> > > ---
> > >    xen/arch/arm/domain_build.c | 83
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >    1 file changed, 83 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 41f2f27..fddfd82 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -368,6 +368,89 @@ static void __init allocate_memory_11(struct domain *d,
> > >        }
> > >    }
> > >    +#if 0
> > > +static bool __init allocate_bank_memory(struct domain *d,
> > > +                                        struct kernel_info *kinfo,
> > > +                                        gfn_t sgfn,
> > > +                                        unsigned int order)
> > > +{
> > > +    int res;
> > > +    struct page_info *pg;
> > > +    struct membank *bank;
> > > +    paddr_t gaddr = gfn_to_gaddr(sgfn), size = pfn_to_paddr(1UL << order);
> > There is no need to have gaddr in a variable.
> I'll remove it.
>
>
> > > +
> > > +    pg = alloc_domheap_pages(d, order, 0);
> > So here you impose the memory to be contiguously allocated for a given bank.
> > There are quite a few case where you may not have enough memory to allocate
> > contiguously.
> >
> > So more likely you want to add loop in this code to allocate until order is
> > reached.
> This case is not handled today for dom0.
I am afraid this is slightly untrue. Dom0 memory is direct mapped, Xen 
will first try to allocate the using the biggest order. If it fails, the 
order will be decreased until the size is suitable. We will always try 
to allocate the maximum of memory.
There are three major problems with your code:
	1) There may not be enough contiguous space in the host memory for a 
4GB region.
	2) The memory specify by the user may not be in power of 2 pages. For 
instance, a user can specify 40KB. With your algo, we will end to 
allocate 32KB in the first bank and 8KB in the second bank. However what 
we want is allocate 40KB in a single bank.
	3) You don't check whether the leftover memory is bigger than the size 
of the second bank.
In the case where the memory is not direct mapped, we should be able to 
allocate the whole region with multiple non-contiguous memory. I think 
the algorithm in used in libxc (see populate_guest_memory) is working 
quite well for this purpose.
It is also possible to have a simpler solution within the hypervisor. I 
quickly wrote a patch (not compiled, nor tested) that should address the 
3 problems above. See: https://pastebin.com/FQ9k9CbL
Cheers,

--
Julien Grall

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