[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
On 19/05/2021 08:27, Penny Zheng wrote:
Hi Julien
-----Original Message-----
From: Julien Grall <julien@xxxxxxx>
Sent: Tuesday, May 18, 2021 8:06 PM
To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
sstabellini@xxxxxxxxxx
Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
<Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
Subject: Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
Hi Penny,
On 18/05/2021 06:21, Penny Zheng wrote:
This commit introduces allocate_static_memory to allocate static
memory as guest RAM for domain on Static Allocation.
It uses alloc_domstatic_pages to allocate pre-defined static memory
banks for this domain, and uses guest_physmap_add_page to set up P2M
table, guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE.
Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
---
xen/arch/arm/domain_build.c | 157
+++++++++++++++++++++++++++++++++++-
1 file changed, 155 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 30b55588b7..9f662313ad 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct
domain *d,
return true;
}
+/*
+ * #ram_index and #ram_index refer to the index and starting address
+of guest
+ * memory kank stored in kinfo->mem.
+ * Static memory at #smfn of #tot_size shall be mapped #sgfn, and
+ * #sgfn will be next guest address to map when returning.
+ */
+static bool __init allocate_static_bank_memory(struct domain *d,
+ struct kernel_info *kinfo,
+ int ram_index,
Please use unsigned.
+ paddr_t ram_addr,
+ gfn_t* sgfn,
I am confused, what is the difference between ram_addr and sgfn?
We need to constructing kinfo->mem(guest RAM banks) here, and
we are indexing in static_mem(physical ram banks). Multiple physical ram
banks consist of one guest ram bank(like, GUEST_RAM0).
ram_addr here will either be GUEST_RAM0_BASE, or GUEST_RAM1_BASE,
for now. I kinds struggled in how to name it. And maybe it shall not be a
parameter here.
Maybe I should switch.. case.. on the ram_index, if its 0, its GUEST_RAM0_BASE,
And if its 1, its GUEST_RAM1_BASE.
You only need to set kinfo->mem.bank[ram_index].start once. This is when
you know the bank is first used.
AFAICT, this function will map the memory for a range start at ``sgfn``.
It doesn't feel this belongs to the function.
The same remark is valid for kinfo->mem.nr_banks.
+ mfn_t smfn,
+ paddr_t tot_size) {
+ int res;
+ struct membank *bank;
+ paddr_t _size = tot_size;
+
+ bank = &kinfo->mem.bank[ram_index];
+ bank->start = ram_addr;
+ bank->size = bank->size + tot_size;
+
+ while ( tot_size > 0 )
+ {
+ unsigned int order = get_allocation_size(tot_size);
+
+ res = guest_physmap_add_page(d, *sgfn, smfn, order);
+ if ( res )
+ {
+ dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+ return false;
+ }
+
+ *sgfn = gfn_add(*sgfn, 1UL << order);
+ smfn = mfn_add(smfn, 1UL << order);
+ tot_size -= (1ULL << (PAGE_SHIFT + order));
+ }
+
+ kinfo->mem.nr_banks = ram_index + 1;
+ kinfo->unassigned_mem -= _size;
+
+ return true;
+}
+
static void __init allocate_memory(struct domain *d, struct kernel_info
*kinfo)
{
unsigned int i;
@@ -480,6 +524,116 @@ fail:
(unsigned long)kinfo->unassigned_mem >> 10);
}
+/* Allocate memory from static memory as RAM for one specific domain
+d. */ static void __init allocate_static_memory(struct domain *d,
+ struct kernel_info
+*kinfo) {
+ int nr_banks, _banks = 0;
AFAICT, _banks is the index in the array. I think it would be clearer if it is
caller 'bank' or 'idx'.
Sure, I’ll use the 'bank' here.
+ size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE;
+ paddr_t bank_start, bank_size;
+ gfn_t sgfn;
+ mfn_t smfn;
+
+ kinfo->mem.nr_banks = 0;
+ sgfn = gaddr_to_gfn(GUEST_RAM0_BASE);
+ nr_banks = d->arch.static_mem.nr_banks;
+ ASSERT(nr_banks >= 0);
+
+ if ( kinfo->unassigned_mem <= 0 )
+ goto fail;
+
+ while ( _banks < nr_banks )
+ {
+ bank_start = d->arch.static_mem.bank[_banks].start;
+ smfn = maddr_to_mfn(bank_start);
+ bank_size = d->arch.static_mem.bank[_banks].size;
The variable name are slightly confusing because it doesn't tell whether this
is physical are guest RAM. You might want to consider to prefix them with p
(resp. g) for physical (resp. guest) RAM.
Sure, I'll rename to make it more clearly.
+
+ if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start,
0) )
+ {
+ printk(XENLOG_ERR
+ "%pd: cannot allocate static memory"
+ "(0x%"PRIx64" - 0x%"PRIx64")",
bank_start and bank_size are both paddr_t. So this should be PRIpaddr.
Sure, I'll change
+ d, bank_start, bank_start + bank_size);
+ goto fail;
+ }
+
+ /*
+ * By default, it shall be mapped to the fixed guest RAM address
+ * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
+ * Starting from RAM0(GUEST_RAM0_BASE).
+ */
Ok. So you are first trying to exhaust the guest bank 0 and then moved to
bank 1. This wasn't entirely clear from the design document.
I am fine with that, but in this case, the developper should not need to know
that (in fact this is not part of the ABI).
Regarding this code, I am a bit concerned about the scalability if we introduce
a second bank.
Can we have an array of the possible guest banks and increment the index
when exhausting the current bank?
Correct me if I understand wrongly,
What you suggest here is that we make an array of guest banks, right now,
including
GUEST_RAM0 and GUEST_RAM1. And if later, adding more guest banks, it will not
bring scalability problem here, right?
Yes. This should also reduce the current complexity of the code.
Cheers,
--
Julien Grall
|
