[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 17/25] xen/arm: introduce allocate_memory
Hi,
On 01/08/18 00:28, 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.
Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
---
Changes in v3:
- new patch
---
xen/arch/arm/domain_build.c | 125 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 124 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ab72c36..dfa74e4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -369,6 +369,129 @@ static void __init allocate_memory_11(struct domain *d,
}
}
+static bool __init insert_bank(struct domain *d,
+ struct kernel_info *kinfo,
+ struct page_info *pg,
+ unsigned int order)
+{
+ int res, i;
+ mfn_t smfn;
+ paddr_t gaddr, size;
+ struct membank *bank;
+
+ smfn = page_to_mfn(pg);
This could combine with the declaration above.
+ size = pfn_to_paddr(1UL << order);
Ditto.
+
+ /*
+ * 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 gaddr address for DomUs accordingly.
+ */
+ gaddr = GUEST_RAM0_BASE;
+ if ( kinfo->mem.nr_banks > 0 )
+ {
+ for( i = 0; i < kinfo->mem.nr_banks; i++ )
+ {
+ bank = &kinfo->mem.bank[i];
+ gaddr = bank->start + bank->size;
+ }
+ if ( bank->start == GUEST_RAM0_BASE &&
+ gaddr + size > (GUEST_RAM0_BASE + GUEST_RAM0_SIZE) )
+ gaddr = GUEST_RAM1_BASE;
+ if ( bank->start == GUEST_RAM1_BASE &&
+ gaddr + size > (GUEST_RAM1_BASE + GUEST_RAM1_SIZE) )
+ goto fail;
+ }
I still really dislike this code. This is difficult to understand and
not scalable. As I said in the previous version, it would be possible to
have more than 2 banks in the future. This will either come with PCI PT
or dynamic memory layout.
What should really be done is a function allocate_memory that take in
parameter the range to allocate. E.g
allocate_bank_memory(struct domain *d, gfn_t sgfn, unsigned long order);
Then the function allocate_memory will compute the size of each bank
based on mem_ and call allocate_bank_memory for each bank.
+
+ dprintk(XENLOG_INFO,
+ "Allocated %#"PRIpaddr"-%#"PRIpaddr":%#"PRIpaddr"-%#"PRIpaddr"
(%ldMB/%ldMB, order %d)\n",
It would be possible to request a guest with 16KB of memory. This would
be printed as 0.
+ mfn_to_maddr(smfn), mfn_to_maddr(smfn) + size,
+ gaddr, gaddr + size,
+ 1UL << (order + PAGE_SHIFT - 20),
+ /* Don't want format this as PRIpaddr (16 digit hex) */
+ (unsigned long)(kinfo->unassigned_mem >> 20),
+ order);
+
+ res = guest_physmap_add_page(d, gaddr_to_gfn(gaddr), smfn, order);
+ if ( res )
+ {
+ dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+ goto fail;
+ }
+
+ kinfo->unassigned_mem -= size;
+ bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
+
+ bank->start = gaddr;
+ bank->size = size;
+ kinfo->mem.nr_banks++;
+ return true;
+
+fail:
+ free_domheap_pages(pg, order);
+ return false;
+}
+
+static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
+{
+ const unsigned int min_order = get_order_from_bytes(MB(4));
Why do you have this limitation for non-direct mapped domain? There are
nothing wrong to allocate 2MB/4K pages for them.
+ struct page_info *pg;
+ unsigned int order = get_allocation_size(kinfo->unassigned_mem);
+ int i;
+
+ dprintk(XENLOG_INFO, "Allocating mappings totalling %ldMB for dom%d:\n",
Ditto.
+ /* Don't want format this as PRIpaddr (16 digit hex) */
+ (unsigned long)(kinfo->unassigned_mem >> 20), d->domain_id);
+
+ kinfo->mem.nr_banks = 0;
+
+ order = get_allocation_size(kinfo->unassigned_mem);
+ if ( order > GUEST_RAM0_SIZE )
+ order = GUEST_RAM0_SIZE;
I don't understand this check. You are comparing a power of 2 with KB.
+ while ( kinfo->unassigned_mem )
+ {
+ pg = alloc_domheap_pages(d, order, 0);
+ if ( !pg )
+ {
+ order --;
+
+ if ( order >= min_order )
+ continue;
+
+ /* No more we can do */
+ break;
+ }
+
+ if ( !insert_bank(d, kinfo, pg, order) )
+ break;
+
+ /*
+ * Success, next time around try again to get the largest order
+ * allocation possible.
+ */
+ order = get_allocation_size(kinfo->unassigned_mem);
+ }
+
+ if ( kinfo->unassigned_mem )
+ dprintk(XENLOG_WARNING, "Failed to allocate requested domain memory."
+ /* Don't want format this as PRIpaddr (16 digit hex) */
+ " %ldMB unallocated\n",
+ (unsigned long)kinfo->unassigned_mem >> 20);
I understand this is the current behavior for direct mapped domain. It
makes sense for them because we don't want to create very small bank (we
request 4MB min). But for non direct mapped domain, there are no need
for such limitation. So if you don't allocate the memory, then it means
Xen has no more RAM and it makes little sense to boot them.
+
+ for( i = 0; i < kinfo->mem.nr_banks; i++ )
+ {
+ dprintk(XENLOG_INFO, "Dom%d BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"
(%ldMB)\n",
+ d->domain_id,
+ i,
+ kinfo->mem.bank[i].start,
+ kinfo->mem.bank[i].start + kinfo->mem.bank[i].size,
+ /* Don't want format this as PRIpaddr (16 digit hex) */
+ (unsigned long)(kinfo->mem.bank[i].size >> 20));
+ }
+}
+
static int __init write_properties(struct domain *d, struct kernel_info
*kinfo,
const struct dt_device_node *node)
{
@@ -2241,7 +2364,7 @@ static int __init construct_domU(struct domain *d, struct
dt_device_node *node)
/* type must be set before allocate memory */
d->arch.type = kinfo.type;
#endif
- allocate_memory_11(d, &kinfo);
+ allocate_memory(d, &kinfo);
The call to allocate_memory_11() should have never been added. Please
refactor/re-order the patch to avoid introducing wrong code.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
