[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 7/7] xen/arm: introduce allocate_static_memory
Hi Stefano,
On 02/09/2021 22:32, Stefano Stabellini wrote:
On Tue, 24 Aug 2021, Penny Zheng wrote:
This commit introduces allocate_static_memory to allocate static memory as
guest RAM for Domain on Static Allocation.
It uses acquire_domstatic_pages to acquire pre-configured static memory
for this domain, and uses guest_physmap_add_pages to set up P2M table.
These pre-defined static memory banks shall be mapped to the fixed guest RAM
banks. And only when they exhausts the current guest RAM bank, it will seek
to the next one.
In order to deal with the trouble of count-to-order conversion when page number
is not in a power-of-two, this commit exports p2m_insert_mapping and introduce
a new function guest_physmap_add_pages to cope with adding guest RAM p2m
mapping with nr_pages.
Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
---
v5 changes:
- don't split comment over multi-line (even they are more than 80 characters)
- simply use dt_find_property(node, "xen,static-mem", NULL) to tell
whether using allocate_static_memory, and add error comment when
"xen,static-mem" is used but CONFIG_STATIC_MEMORY is not enabled.
- exporting p2m_insert_mapping() and introduce guest_physmap_add_pages
to cope with adding guest RAM p2m mapping with nr_pages.
- check both pbase and psize are page aligned
- simplify the code in the loops by moving append_static_memory_to_bank()
outside of the if/else.
---
xen/arch/arm/domain_build.c | 156 +++++++++++++++++++++++++++++++++++-
xen/arch/arm/p2m.c | 7 +-
xen/include/asm-arm/p2m.h | 11 +++
3 files changed, 168 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6c86d52781..843b8514c7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -480,6 +480,148 @@ fail:
(unsigned long)kinfo->unassigned_mem >> 10);
}
+#ifdef CONFIG_STATIC_MEMORY
+static bool __init append_static_memory_to_bank(struct domain *d,
+ struct membank *bank,
+ mfn_t smfn,
+ paddr_t size)
+{
+ int res;
+ unsigned int nr_pages = size >> PAGE_SHIFT;
^ PFN_DOWN
+ /* Infer next GFN. */
+ gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
+
+ res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
+ if ( res )
+ {
+ dprintk(XENLOG_ERR, "Failed to map pages to DOMU: %d", res);
+ return false;
+ }
+
+ bank->size = bank->size + size;
+
+ return true;
+}
+
+/* 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,
+ const struct dt_device_node *node)
+{
+ const struct dt_property *prop;
+ u32 addr_cells, size_cells, reg_cells;
+ unsigned int nr_banks, gbank = 0, bank = 0;
+ const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
+ const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
+ const __be32 *cell;
+ u64 tot_size = 0;
+ paddr_t pbase, psize, gsize;
+ mfn_t smfn;
+ int res;
+
+ prop = dt_find_property(node, "xen,static-mem", NULL);
+ if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
+ &addr_cells) )
+ {
+ printk(XENLOG_ERR
+ "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d);
+ goto fail;
+ }
+
+ if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
+ &size_cells) )
+ {
+ printk(XENLOG_ERR
+ "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d);
+ goto fail;
+ }
+ reg_cells = addr_cells + size_cells;
+
+ /* Start with GUEST_RAM0. */
+ gsize = ramsize[gbank];
+ kinfo->mem.bank[gbank].start = rambase[gbank];
+
+ cell = (const __be32 *)prop->value;
+ nr_banks = (prop->length) / (reg_cells * sizeof (u32));
+
+ for ( ; bank < nr_banks; bank++ )
+ {
+ device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
+ ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
+
+ smfn = maddr_to_mfn(pbase);
+ res = acquire_domstatic_pages(d, smfn, psize >> PAGE_SHIFT, 0);
^ PFN_DOWN(psize)
+ if ( res )
+ {
+ printk(XENLOG_ERR
+ "%pd: failed to acquire static memory: %d.\n", d, res);
+ goto fail;
+ }
+
+ printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
+ d, bank, pbase, pbase + psize);
+
+ /*
+ * It shall be mapped to the fixed guest RAM
banks(GUEST_RAM_BANK_BASES),
+ * And only when it exhausts the current guest RAM bank, it will seek
+ * to the next.
+ */
+ while ( 1 )
+ {
+ /* Map as much as possible the static range to the guest bank */
+ if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
smfn,
+ min(psize, gsize)) )
+ goto fail;
+
+ /*
+ * The current physical bank is fully mapped.
+ * Handle the next physical bank.
+ */
+ if ( gsize >= psize )
+ {
+ gsize = gsize - psize;
+ break;
+ }
+ /*
+ * When current guest bank size is not enough to map.
+ * Before seeking to the next, check if we still have available
+ * guest bank.
+ */
+ else if ( (gbank + 1) >= GUEST_RAM_BANKS )
+ {
+ printk(XENLOG_ERR "Exhausted all fixed guest banks.\n");
+ goto fail;
+ }
+ else
+ {
+ psize = psize - gsize;
+ smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
^ PFN_DOWN
+ /* Update to the next guest bank. */
+ gbank++;
+ gsize = ramsize[gbank];
+ kinfo->mem.bank[gbank].start = rambase[gbank];
+ }
+ }
+
+ tot_size += psize;
+ }
+
+ kinfo->mem.nr_banks = ++gbank;
+ kinfo->unassigned_mem -= tot_size;
+ if ( kinfo->unassigned_mem )
+ {
+ printk(XENLOG_ERR
+ "Size of \"memory\" property doesn't match up with the sum-up of
\"xen,static-mem\".\n");
+ goto fail;
Do we need to make this a fatal failure?
I am asking because unassigned_mem comes from the "memory" property of
the domain in device tree which is kind of redundant with the
introduction of xen,static-mem. In fact, I think it would be better to
clarify the role of "memory" when "xen,static-mem" is also present.
In that case, we could even make "memory" optional.
I requested to make it mandatory. Imagine you have a domU that has 1GB
and now you want to switch to static memory. If we make the property
optional, then there is a risk for the admin to not correctly pass the
amount of memory. This may become unnoticed until late.
So I think making it mandatory is cheap for us and an easy way to
confirm you properly sized the region. It also has the benefits to
easily find out how much memory you gave to the guest (but that's just
because I am lazy :)).
In any case, even if we don't make "memory" optional, it might still be
good to turn this error into a warning and ignore the remaining
kinfo->unassigned_mem.
The behavior is matching the existing function and I think this is a
good idea. If you ask 10MB of memory and we only give you 9MB, then at
some point your guest is not going to be happy.
It is much better to know it in advance with a failure over discovering
hours later when you see an OOM from your domain.
+ }
+
+ return;
+
+fail:
+ panic("Failed to allocate requested static memory for domain %pd.", d);
+}
+#endif
+
static int __init write_properties(struct domain *d, struct kernel_info
*kinfo,
const struct dt_device_node *node)
{
@@ -2452,7 +2594,19 @@ static int __init construct_domU(struct domain *d,
/* type must be set before allocate memory */
d->arch.type = kinfo.type;
#endif
- allocate_memory(d, &kinfo);
+ if ( !dt_find_property(node, "xen,static-mem", NULL) )
+ allocate_memory(d, &kinfo);
+#ifdef CONFIG_STATIC_MEMORY
+ else
+ allocate_static_memory(d, &kinfo, node);
+#else
+ else
+ {
+ printk(XENLOG_ERR
+ "CONFIG_STATIC_MEMORY must be enabled to use
\"xen,static-mem\".\n");
+ return -EINVAL;
+ }
+#endif
To avoid the double else in the code, this part could be written like
this which is a bit simpler I think:
if ( !dt_find_property(node, "xen,static-mem", NULL) )
allocate_memory(d, &kinfo);
else
{
#ifdef CONFIG_STATIC_MEMORY
allocate_static_memory(d, &kinfo, node);
#else
printk(XENLOG_ERR
"CONFIG_STATIC_MEMORY must be enabled to use
\"xen,static-mem\".\n");
return -EINVAL;
#endif
}
This is just a suggestion to improve readability, I am also OK with what
you wrote.
(Another alternative would be to provide a stub allocate_static_memory
implementation for !CONFIG_STATIC_MEMORY.)
My preference is 1) Stub function 2) your #ifdef proposal 3) Penny's
approach.
Cheers,
--
Julien Grall
|
