Hi Jan
> -----Original Message-----
> From: Penny Zheng
> Sent: Monday, July 5, 2021 1:22 PM
> To: Julien Grall <jul...@xen.org>; Jan Beulich <jbeul...@suse.com>
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen
> <wei.c...@arm.com>; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Subject: RE: [PATCH 4/9] xen/arm: static memory initialization
>
> Hi Julien and Jan
>
> > -----Original Message-----
> > From: Julien Grall <jul...@xen.org>
> > Sent: Thursday, July 1, 2021 1:46 AM
> > To: Jan Beulich <jbeul...@suse.com>; Penny Zheng
> <penny.zh...@arm.com>
> > Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen
> > <wei.c...@arm.com>; xen-devel@lists.xenproject.org;
> > sstabell...@kernel.org
> > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> >
> > Hi,
> >
> > On 10/06/2021 10:35, Jan Beulich wrote:
> > > 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.
> >
> > +1.
> >
>
> Understood. Thx.
>
> > >
> > >> + /*
> > >> + * TODO: Considering NUMA-support scenario.
> > >> + */
> > >
> > > Nit: Comment style.
> > >
>
> Sure, thx.
>
> > >> @@ -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?
> > >
>
> Sure, right.
> Like what Julien suggests, init_staticmem_pages() is already able to cope
> with nr_banks == 0.
>
> > >> --- 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.
> > >
>
> Thank for the suggestion~
>
While applying your suggestion here, if adding an ASSERT() to double check both
passed-in
arguments match up, either use like page_to_mfn to establish connection, which
is absolutely
unacceptable, or pass more parameters like base page/mfn to compare the offset.
Hmmm,
I am not in favor of this, since both extra parameters are only used in
assertion only.
> > >> + /* 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.
> > >
>
> To not bring dead codes in other archs, I'll use more generic option
> CONFIG_STATIC_ALLOCATION.
>
> > >> + 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.
> > >
>
> Sure. Will do.
>
> > >> + {
> > >> + pg->count_info |= PGC_need_scrub;
> > >> + poison_one_page(pg);
> > >> + }
> > >> +
> > >> + return;
> > >
> > > Please omit return statements at the end of functions returning void.
> > >
>
> Sure, thx
>
> > >> +}
> > >
> > > 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?
> > >
>
> Ok. Thx. Always not good at giving names. I'll take mark_page_free()
>
> > >> @@ -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.
> > >
>
> Yeah, I was following the logic in free_heap_pages.
> Hmmm, I could not think of any scenario that will lead to PGC_state_offlining,
> that's why I was not including it at the first place.
> For broken issue, tbh, I just copy the bug_on from free_heap_pages, after
> quite a time thinking, I also could not find any scenario when a page(static
> or
> not) can become broken while in use. ;/
>
> > >> --- 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
> > >
> >
> > --
> > Julien Grall
>
> Cheers
>
> --
> Penny Zheng
