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

[Jan Beulich <jbeulich@xxxxxxxx>]

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