From: Penny Zheng <Penny.Zheng@arm.com>
To: Julien Grall <julien@xen.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
"sstabellini@kernel.org" <sstabellini@kernel.org>,
"jbeulich@suse.com" <jbeulich@suse.com>
Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>, Wei Chen <Wei.Chen@arm.com>
Subject: RE: [PATCH 9/9] xen/arm: introduce allocate_static_memory
Date: Tue, 6 Jul 2021 07:30:31 +0000 [thread overview]
Message-ID: <VE1PR08MB521539446863D93467AAA8BBF71B9@VE1PR08MB5215.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <2dc9387c-1c6c-70af-5bf5-8b606b62cc79@xen.org>
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, July 3, 2021 10:18 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 9/9] xen/arm: introduce allocate_static_memory
>
> 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.
>
Sure.
> > + 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()
>
Ok~~~
> 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);
>
> [....]
> }
>
Oh. Right, since we are setting info in kinfo.mem.bank[i], we could infer itself to get the
Next GFN, clever! Thks a lot!
> > +{
> > + 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?
>
Hmm, I'm kinds of confused the suggestion here. If we using rambase[] here,
then in later double loop, when updating to the next guest bank, we could
use rambase[gbank] to refer.
> > +
> > + 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?
>
Ok. Will do.
> > {
> > 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.
Sure.
>
> > + 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.
>
Do you want to expand the "device_tree_get_reg" to only cope with "regs" property,
right now, it passes into cells, "prop->value", the cells of the reg property.
Changing it to that the input parameter will be the device node holding the reg property, like
"device_tree_get_reg(const struct dt_device_node *node, u32 addr_cells, u32 size_cells, struct meminfo *info)"
Or we still retain the original device_tree_get_reg()(maybe, the name shall be changed....), and
Introduce a new function to do what I said above.
> > + 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]
>
Thanks for the detailed simplification!!!
> > + }
> > + }
> > + 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
Cheers
--
Penny Zheng
prev parent reply other threads:[~2021-07-06 7:31 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
2021-07-06 7:30 ` Penny Zheng [this message]
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=VE1PR08MB521539446863D93467AAA8BBF71B9@VE1PR08MB5215.eurprd08.prod.outlook.com \
--to=penny.zheng@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Wei.Chen@arm.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--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.