[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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.