From: Jan Beulich <jbeulich@suse.com>
To: Penny Zheng <penny.zheng@arm.com>
Cc: Bertrand.Marquis@arm.com, Wei.Chen@arm.com,
xen-devel@lists.xenproject.org, sstabellini@kernel.org,
julien@xen.org
Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
Date: Thu, 10 Jun 2021 11:35:36 +0200 [thread overview]
Message-ID: <e0a312a1-f430-3ff0-6dd6-fcfe18e58071@suse.com> (raw)
In-Reply-To: <20210607024318.3988467-5-penny.zheng@arm.com>
On 07.06.2021 04:43, Penny Zheng wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -611,6 +611,30 @@ static void __init init_pdx(void)
> }
> }
>
> +/* Static memory initialization */
> +static void __init init_staticmem_pages(void)
> +{
> + int bank;
While I'm not a maintainer of this code, I'd still like to point out
that wherever possible we prefer "unsigned int" when dealing with
only non-negative values, and even more so when using them as array
indexes.
> + /*
> + * TODO: Considering NUMA-support scenario.
> + */
Nit: Comment style.
> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset,
> cmdline_parse(cmdline);
>
> setup_mm();
> + /* If exists, Static Memory Initialization. */
> + if ( bootinfo.static_mem.nr_banks > 0 )
> + init_staticmem_pages();
I don't think the conditional is really needed here?
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void)
> return node_to_scrub(false) != NUMA_NO_NODE;
> }
>
> +static void free_page(struct page_info *pg, bool need_scrub)
> +{
> + mfn_t mfn = page_to_mfn(pg);
With pdx compression this is a non-trivial conversion. The function
being an internal helper and the caller already holding the MFN, I
think it would be preferable if the MFN was passed in here. If done
this way, you may want to consider adding an ASSERT() to double
check both passed in arguments match up.
> + /* If a page has no owner it will need no safety TLB flush. */
> + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL);
> + if ( pg->u.free.need_tlbflush )
> + page_set_tlbflush_timestamp(pg);
> +
> + /* This page is not a guest frame any more. */
> + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */
> + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
> +
> +#ifdef CONFIG_ARM
If avoidable there should be no arch-specific code added to this
file. Assuming another arch gained PGC_reserved, what's wrong with
enabling this code right away for them as well? I.e. use
PGC_reserved here instead of CONFIG_ARM? Alternatively this may
want to be CONFIG_STATIC_ALLOCATION, assuming we consider
PGC_reserved tied to it.
> + if ( pg->count_info & PGC_reserved )
> + {
> + /* TODO: asynchronous scrubbing. */
> + if ( need_scrub )
> + scrub_one_page(pg);
> + return;
> + }
> +#endif
> + if ( need_scrub )
Nit: Please have a blank line between these last two.
> + {
> + pg->count_info |= PGC_need_scrub;
> + poison_one_page(pg);
> + }
> +
> + return;
Please omit return statements at the end of functions returning void.
> +}
On the whole, bike shedding or not, I'm afraid the function's name
doesn't match what it does: There's no freeing of a page here. What
gets done is marking of a page as free. Hence maybe mark_page_free()
or mark_free_page() or some such?
> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
> spin_unlock(&heap_lock);
> }
>
> +#ifdef CONFIG_STATIC_ALLOCATION
> +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
> +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> + bool need_scrub)
> +{
> + mfn_t mfn = page_to_mfn(pg);
> + unsigned long i;
> +
> + for ( i = 0; i < nr_mfns; i++ )
> + {
> + switch ( pg[i].count_info & PGC_state )
> + {
> + case PGC_state_inuse:
> + BUG_ON(pg[i].count_info & PGC_broken);
> + /* Mark it free and reserved. */
> + pg[i].count_info = PGC_state_free | PGC_reserved;
> + break;
> +
> + default:
> + printk(XENLOG_ERR
> + "Page state shall be only in PGC_state_inuse. "
Why? A page (static or not) can become broken while in use. IOW I
don't think you can avoid handling PGC_state_offlining here. At which
point this code will match free_heap_pages()'es, and hence likely
will want folding as well.
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
> } while ( false )
> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>
> +#ifdef CONFIG_ARM
ITYM CONFIG_STATIC_ALLOCATION here?
Jan
next prev parent reply other threads:[~2021-06-10 9:36 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 [this message]
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
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=e0a312a1-f430-3ff0-6dd6-fcfe18e58071@suse.com \
--to=jbeulich@suse.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Wei.Chen@arm.com \
--cc=julien@xen.org \
--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.