RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan 

> -----Original Message-----
> From: Penny Zheng
> Sent: Monday, July 5, 2021 1:22 PM
> To: Julien Grall <julien@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Subject: RE: [PATCH 4/9] xen/arm: static memory initialization
> 
> Hi Julien and Jan
> 
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Thursday, July 1, 2021 1:46 AM
> > To: Jan Beulich <jbeulich@xxxxxxxx>; Penny Zheng
> <Penny.Zheng@xxxxxxx>
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> > <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx
> > 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