Re: [PATCH v2 02/21] xen/arm: make allocate_memory work for non 1:1 mapped guests

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 9 Jul 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/07/18 00:11, Stefano Stabellini wrote:
> > Extend allocate_memory to work for non 1:1 mapped domUs. Specifically,
> > memory allocated for domU will be mapped into the domU pseudo-physical
> > address space at the appropriate addresses according to the guest memory
> > map: GUEST_RAM0_BASE and GUEST_RAM1_BASE.
> > 
> > To do that, insert_11_bank has been extended to deal with non-dom0
> > mappings starting from GUEST_RAM0_BASE. insert_11_bank has been renamed
> > to insert_bank.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > Changes in v2:
> > - new patch
> > ---
> >   xen/arch/arm/domain_build.c | 57
> > ++++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 44 insertions(+), 13 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 182e3d5..2a6619a 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -97,27 +97,51 @@ static unsigned int get_allocation_size(paddr_t size)
> >    * Returns false if the memory would be below bank 0 or we have run
> >    * out of banks. In this case it will free the pages.
> >    */
> > -static bool insert_11_bank(struct domain *d,
> > -                           struct kernel_info *kinfo,
> > -                           struct page_info *pg,
> > -                           unsigned int order)
> > +static bool insert_bank(struct domain *d,
> > +                        struct kernel_info *kinfo,
> > +                        struct page_info *pg,
> > +                        unsigned int order,
> > +                        bool map_11)
> >   {
> > -    int res, i;
> > +    int res, i, nr_mem_banks = map_11 ? NR_MEM_BANKS : 2;
> 
> nr_mem_banks should be unsigned. I also would drop "mem_" to shorten a bit the
> name.

OK


> >       mfn_t smfn;
> >       paddr_t start, size;
> > +    struct membank *bank;
> >         smfn = page_to_mfn(pg);
> >       start = mfn_to_maddr(smfn);
> 
> The new code is pretty horrible to read. Can you please add some comments to
> help understanding it?
> 
> Here is already an example where you set start to MFN. But then override after
> with none comment to understand why.
> 
> Also, this code as always assumed MFN == GFN so start was making sense. Now,
> you re-purpose it to just the guest-physical address. So more likely you want
> to rework the code a bit.

I'll add more comments in the code. Next time the patch will be clearer.
This is a snippet to give you an idea, but it might be best for you to
just wait for the next version before reading this again.

    /*
     * smfn: the address of the set of pages to map
     * start: the address in guest pseudo-physical memory where to map
     *        the pages
     */
    smfn = page_to_mfn(pg);
    start = mfn_to_maddr(smfn);
    size = pfn_to_paddr(1UL << order);
    if ( !is_hardware_domain(d) )
    {
        /*
         * Dom0 is 1:1 mapped, so start is the same as (smfn <<
         * PAGE_SHIFT).
         *
         * Instead, DomU memory is provided in two banks:
         *   GUEST_RAM0_BASE - GUEST_RAM0_BASE + GUEST_RAM0_SIZE
         *   GUEST_RAM1_BASE - GUEST_RAM1_BASE + GUEST_RAM1_SIZE
         * 
         * Find the right start address for DomUs accordingly.
         */
  


> >       size = pfn_to_paddr(1UL << order);
> > +    if ( !map_11 )
> 
> I am not sure why map_11 would mean DomU? I don't see any reason to not allow
> that for Dom0. Note that I am not asking to do it, just having clearer name.

Good point. I think I should just drop the option, which is just
confusing, and keep using is_hardware_domain(d) checks like in
allocate_memory. Otherwise let me know if you have a better idea.


> > +    {
> > +        start = GUEST_RAM0_BASE;
> > +        if ( kinfo->mem.nr_banks > 0 )
> > +        {
> > +            for( i = 0; i < kinfo->mem.nr_banks; i++ )
> > +            {
> > +                bank = &kinfo->mem.bank[i];
> > +                start = bank->start + bank->size;
> > +            }
> > +            if ( bank->start == GUEST_RAM0_BASE &&
> > +                    start + size > (GUEST_RAM0_BASE + GUEST_RAM0_SIZE) )
> 
> The indentation looks wrong.

OK


> > +                start = GUEST_RAM1_BASE;
> > +            if ( bank->start == GUEST_RAM1_BASE &&
> > +                    start + size > (GUEST_RAM1_BASE + GUEST_RAM1_SIZE) )
> > +            {
> > +                D11PRINT("Allocation of domain memory exceeds max
> > amount\n");
> 
> This looks quite strange to use D11PRINT here as this related to direct-domain
> mapped.
 
You are right, but I didn't feel like replacing all D11PRINT
occurrences. Should I do that? Or should I change just this instance? I
could also just drop this D11PRINT, given that the printk is not even
enabled by default. Let me know.


> > +                goto fail;
> > +            }
> > +        }
> > +    }
> >   -    D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order
> > %d)\n",
> > +    D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr":%#"PRIpaddr"-%#"PRIpaddr"
> > (%ldMB/%ldMB, order %d)\n",
> > +             mfn_to_maddr(smfn), mfn_to_maddr(smfn) + size,
> >                start, start + size,
> >                1UL << (order + PAGE_SHIFT - 20),
> >                /* Don't want format this as PRIpaddr (16 digit hex) */
> >                (unsigned long)(kinfo->unassigned_mem >> 20),
> >                order);
> >   -    if ( kinfo->mem.nr_banks > 0 &&
> > +    if ( map_11 && kinfo->mem.nr_banks > 0 &&
> 
> Why do you drop that check? It should be harmless for non-direct mapped
> domain.

You are right. I'll remove this change.


> >            size < MB(128) && >            start + size <
> > kinfo->mem.bank[0].start )
> >       {
> > @@ -125,7 +149,7 @@ static bool insert_11_bank(struct domain *d,
> >           goto fail;
> >       }
> >   -    res = guest_physmap_add_page(d, _gfn(mfn_x(smfn)), smfn, order);
> > +    res = guest_physmap_add_page(d, gaddr_to_gfn(start), smfn, order);
> >       if ( res )
> >           panic("Failed map pages to DOM0: %d", res);
> >   @@ -141,7 +165,7 @@ static bool insert_11_bank(struct domain *d,
> >         for( i = 0; i < kinfo->mem.nr_banks; i++ )
> >       {
> > -        struct membank *bank = &kinfo->mem.bank[i];
> > +        bank = &kinfo->mem.bank[i];
> >             /* If possible merge new memory into the start of the bank */
> >           if ( bank->start == start+size )
> > @@ -164,7 +188,7 @@ static bool insert_11_bank(struct domain *d,
> >            * could have inserted the memory into/before we would already
> >            * have done so, so this must be the right place.
> >            */
> > -        if ( start + size < bank->start && kinfo->mem.nr_banks <
> > NR_MEM_BANKS )
> > +        if ( start + size < bank->start && kinfo->mem.nr_banks <
> > nr_mem_banks )
> >           {
> >               memmove(bank + 1, bank,
> >                       sizeof(*bank) * (kinfo->mem.nr_banks - i));
> > @@ -175,7 +199,7 @@ static bool insert_11_bank(struct domain *d,
> >           }
> >       }
> >   -    if ( i == kinfo->mem.nr_banks && kinfo->mem.nr_banks < NR_MEM_BANKS )
> > +    if ( i == kinfo->mem.nr_banks && kinfo->mem.nr_banks < nr_mem_banks )
> >       {
> >           struct membank *bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> >   @@ -253,6 +277,7 @@ static void allocate_memory(struct domain *d, struct
> > kernel_info *kinfo)
> >       struct page_info *pg;
> >       unsigned int order = get_allocation_size(kinfo->unassigned_mem);
> >       int i;
> > +    bool hwdom = d->domain_id == 0;
> 
> You should use is_hardware_domain(...).

Yes, I'll do that here and elsewhere in this patch


> >         bool lowmem = true;
> >       unsigned int bits;
> > @@ -263,6 +288,12 @@ static void allocate_memory(struct domain *d, struct
> > kernel_info *kinfo)
> >         kinfo->mem.nr_banks = 0;
> >   +    if ( !hwdom )
> > +    {
> > +        lowmem = false;
> > +        goto got_bank0;
> > +    }
> 
> Can you explain why you need this?

Yes, I'll add a comment. The first part of allocate_memory tries to
allocate memory as low as possible, which is fine for dom0 but it is
unnecessary for DomUs, given that they are not 1:1 mapped. So, this
check is meant to go straight to the regular allocation for DomUs,
skipping the special below-4G allocation loop.


> > +
> >       /*
> >        * First try and allocate the largest thing we can as low as
> >        * possible to be bank 0.
> > @@ -274,7 +305,7 @@ static void allocate_memory(struct domain *d, struct
> > kernel_info *kinfo)
> >               pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
> >               if ( pg != NULL )
> >               {
> > -                if ( !insert_11_bank(d, kinfo, pg, order) )
> > +                if ( !insert_bank(d, kinfo, pg, order, hwdom) )
> >                       BUG(); /* Cannot fail for first bank */
> >                     goto got_bank0;
> > @@ -319,7 +350,7 @@ static void allocate_memory(struct domain *d, struct
> > kernel_info *kinfo)
> >               break;
> >           }
> >   -        if ( !insert_11_bank(d, kinfo, pg, order) )
> > +        if ( !insert_bank(d, kinfo, pg, order, hwdom) )
> >           {
> >               if ( kinfo->mem.nr_banks == NR_MEM_BANKS )
> >                   /* Nothing more we can do. */
> > 

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