From: Julien Grall <julien@xen.org>
To: Penny Zheng <penny.zheng@arm.com>,
xen-devel@lists.xenproject.org, sstabellini@kernel.org,
jbeulich@suse.com
Cc: Bertrand.Marquis@arm.com, Wei.Chen@arm.com
Subject: Re: [PATCH 9/9] xen/arm: introduce allocate_static_memory
Date: Sat, 3 Jul 2021 15:18:24 +0100 [thread overview]
Message-ID: <2dc9387c-1c6c-70af-5bf5-8b606b62cc79@xen.org> (raw)
In-Reply-To: <20210607024318.3988467-10-penny.zheng@arm.com>
Hi Penny,
On 07/06/2021 03:43, 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-configured static memory banks
> for this domain, and uses guest_physmap_add_page to set up P2M table.
> These pre-defiend static memory ranges shall be firstly mapped to the
> fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
> `GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`.
> `GUEST_RAM0` may take up several pre-defined physical RAM regions.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - rename the values, like prefix it g/p
> - fix the scalability issue
> - allocate when parse
> ---
> xen/arch/arm/domain_build.c | 155 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4166d7993c..63b6a97b2c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct domain *d,
> return true;
> }
>
> +/*
> + * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of #gsize,
> + * 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 gbank,
Please use unsigned int for index.
> + gfn_t* sgfn,
> + mfn_t smfn,
> + paddr_t gsize)
This function doesn't allocate memory and neither a "bank" per-se. So
would suggest to rename to (or something similar):
append_static_memory_to_bank()
Also, I don't think you need all those parameters. You can infer the
next GFN to use from the bank information. So how about something like:
static bool __init append_static_memory_to_bank(struct domain *d,
struct membank *bank,
mfn_t smfn,
paddr_t size)
{
gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
[....]
}
> +{
> + int res;
> + paddr_t tot_size = gsize;
> + const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
I don't like the idea of introducing rambase here. Can't the bank be
initialized in the caller?
> +
> + 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));
> + }
> +
> + /* Guest RAM bank in kinfo hasn't been initialized. */
> + if ( gbank == kinfo->mem.nr_banks )
> + {
> + kinfo->mem.bank[gbank].start = rambase[gbank];
> + kinfo->mem.nr_banks++;
> + }
> + kinfo->mem.bank[gbank].size += gsize;
> +
> + return true;
> +}
> +
> static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
It feels a bit odd that the two functions you introduced are not
together. Can they be moved together?
> {
> unsigned int i;
> @@ -480,6 +522,116 @@ fail:
> (unsigned long)kinfo->unassigned_mem >> 10);
> }
>
> +/* Allocate memory from static memory as RAM for one specific domain d. */
> +static u64 __init allocate_static_memory(struct domain *d,
> + struct kernel_info *kinfo,
> + const struct dt_device_node *node)
> +{
> + int nr_banks, bank = 0, gbank = 0;
> + const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> + const __be32 *cell;
> + const struct dt_property *prop;
> + struct dt_device_node *static_mem_node;
> + const struct dt_device_node *parent = dt_find_node_by_path("/reserved-memory");
> + u32 addr_cells = 2, size_cells = 2, reg_cells;
> + u64 tot_size;
> +
> + paddr_t pbase, psize, gsize;
> + gfn_t sgfn;
> + mfn_t smfn;
> +
> + kinfo->mem.nr_banks = 0;
> + /* Start with GUEST_RAM0. */
> + gsize = ramsize[gbank];
> + sgfn = gaddr_to_gfn(rambase[gbank]);
> +
> + /* Parse phandle in `xen,static-mem`. */
> + static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
> + if ( !static_mem_node )
> + goto fail;
> +
> + /*
> + * #address-cells and #size-cells must be consistent with the parent node,
> + * "reserved-memory".
> + */
> + dt_property_read_u32(parent, "#address-cells", &addr_cells);
> + dt_property_read_u32(parent, "#size-cells", &size_cells);
The return for dt_property_read_u32() should be checked.
> + BUG_ON(size_cells > 2 || addr_cells > 2);
> + reg_cells = addr_cells + size_cells;
> +
> + prop = dt_find_property(static_mem_node, "reg", NULL);
> + if ( !prop )
> + goto fail;
> + cell = (const __be32 *)prop->value;
> + nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> + BUG_ON(nr_banks > NR_MEM_BANKS);
> +
> + while ( bank < nr_banks )
> + {
> + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
We seem to have quite a few functions now that will iterate over "regs".
It would be worth considering to introduce a helper to iterate it.
> + tot_size += (u64)psize;
This cast seems unnecessary.
> + smfn = maddr_to_mfn(pbase);
> +
> + if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
> + {
> + printk(XENLOG_ERR
> + "%pd: cannot allocate static memory"
> + "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
> + d, pbase, pbase + psize);
> + goto fail;
> + }
> +
> + printk(XENLOG_INFO "%pd STATIC BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"\n",
> + d, bank, pbase, pbase + psize);
> +
> + /*
> + * It shall be mapped to the fixed guest RAM address rambase[i],
> + * And until it exhausts the ramsize[i], it will seek to the next
> + * rambase[i+1].
> + */
> + while ( 1 )
> + {
> + if ( gsize >= psize )
> + {
> + if ( !allocate_static_bank_memory(d, kinfo, gbank,
> + &sgfn, smfn, psize) )
> + goto fail;
> +
> + gsize = gsize - psize;
> + bank++;
> + break;
> + }
> + else
> + {
> + if ( !allocate_static_bank_memory(d, kinfo, gbank,
> + &sgfn, smfn, gsize) )
> + goto fail;
> +
> + /*
> + * Physical bank hasn't been totally mapped,
> + * seeking to the next guest RAM i+1, if exist.
> + */
> + if ( ++gbank < GUEST_RAM_BANKS )
> + {
> + psize = psize - gsize;
> + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> + gsize = ramsize[gbank];
> + sgfn = gaddr_to_gfn(rambase[gbank]);
> + }
> + else
> + goto fail;
> + }
The double loop is not nice but I can't think of a better way. However,
I think the code in the loop can be simplified. You could write
something like:
/* Map as much as possible the static range to the guest bank */
if ( !allocate_static_bank(..., min(psize, gsize) )
goto fail;
/* The physical bank is fully mapped. Handle the next bank. */
if ( gsize >= psize )
{
gsize = gsize - psize;
bank++;
break;
}
/* We still have memory to map. Check if we have another guest bank
available */
else if ( ++gbank > GUEST_RAM_BANKS ) {
printk("Exhausted the number of guest bank\n");
goto fail;
}
[Update to the next guest bank]
> + }
> + }
> + return tot_size;
> +
> +fail:
> + panic("Failed to allocate requested static memory for domain %pd."
> + "Fix the VMs configurations.\n",
> + d);
> +}
> +
> static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
> if ( prop )
> {
> static_mem = true;
> - /* static_mem_size = allocate_static_memory(...); */
> - BUG();
> + static_mem_size = allocate_static_memory(d, &kinfo, node);
> }
>
> rc = dt_property_read_u64(node, "memory", &mem);
>
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-07-03 14:18 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-07 2:43 [PATCH V2 0/9] Domain on Static Allocation Penny Zheng
2021-06-07 2:43 ` [PATCH 1/9] xen/arm: introduce domain " Penny Zheng
2021-06-07 2:43 ` [PATCH 2/9] xen/arm: introduce PGC_reserved Penny Zheng
2021-06-30 17:44 ` Julien Grall
2021-07-05 3:09 ` Penny Zheng
2021-06-07 2:43 ` [PATCH 3/9] xen/arm: introduce CONFIG_STATIC_ALLOCATION Penny Zheng
2021-06-07 6:17 ` Jan Beulich
2021-06-30 17:45 ` Julien Grall
2021-07-05 3:16 ` Penny Zheng
2021-06-07 2:43 ` [PATCH 4/9] xen/arm: static memory initialization Penny Zheng
2021-06-10 9:35 ` Jan Beulich
2021-06-30 17:46 ` Julien Grall
2021-07-05 5:22 ` Penny Zheng
2021-07-05 7:14 ` Penny Zheng
2021-07-05 7:50 ` Jan Beulich
2021-07-05 9:19 ` Penny Zheng
2021-07-05 7:48 ` Jan Beulich
2021-06-30 18:09 ` Julien Grall
2021-07-05 7:28 ` Penny Zheng
2021-07-06 9:09 ` Julien Grall
2021-07-06 9:20 ` Penny Zheng
2021-07-06 9:26 ` Julien Grall
2021-06-07 2:43 ` [PATCH 5/9] xen: introduce assign_pages_nr Penny Zheng
2021-06-10 9:49 ` Jan Beulich
2021-06-30 18:29 ` Julien Grall
2021-07-01 8:26 ` Jan Beulich
2021-07-01 9:24 ` Julien Grall
2021-07-01 10:13 ` Jan Beulich
2021-06-07 2:43 ` [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and alloc_domstatic_pages Penny Zheng
2021-06-10 10:23 ` Jan Beulich
2021-07-06 5:58 ` Penny Zheng
2021-07-06 6:53 ` Jan Beulich
2021-07-06 9:39 ` Julien Grall
2021-07-06 9:59 ` Jan Beulich
2021-07-06 10:31 ` Julien Grall
2021-07-08 9:09 ` Penny Zheng
2021-07-08 10:06 ` Jan Beulich
2021-07-08 11:07 ` Penny Zheng
2021-06-07 2:43 ` [PATCH 7/9] xen/arm: take care of concurrency on static memory allocation Penny Zheng
2021-06-10 10:53 ` Jan Beulich
2021-06-07 2:43 ` [PATCH 8/9] xen/arm: check `xen,static-mem` property during domain construction Penny Zheng
2021-07-03 13:26 ` Julien Grall
2021-07-06 6:31 ` Penny Zheng
2021-07-06 6:57 ` Jan Beulich
2021-07-06 7:35 ` Penny Zheng
2021-07-06 9:22 ` Julien Grall
2021-06-07 2:43 ` [PATCH 9/9] xen/arm: introduce allocate_static_memory Penny Zheng
2021-07-03 14:18 ` Julien Grall [this message]
2021-07-06 7:30 ` Penny Zheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2dc9387c-1c6c-70af-5bf5-8b606b62cc79@xen.org \
--to=julien@xen.org \
--cc=Bertrand.Marquis@arm.com \
--cc=Wei.Chen@arm.com \
--cc=jbeulich@suse.com \
--cc=penny.zheng@arm.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.