|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 02/21] xen/arm: make allocate_memory work for non 1:1 mapped guests
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@xxxxxxxxxx>
> >
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |