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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

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~

> >> +    /* 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