Re: [PATCH v5 6/7] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 11:50, Penny Zheng wrote:
> +/*
> + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
> + * static memory.
> + * This function needs to be reworked if used outside of boot.
> + */
> +static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
> +                                                         unsigned long 
> nr_mfns,
> +                                                         unsigned int 
> memflags)
> +{
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
> +    unsigned long i;
> +    struct page_info *pg;
> +
> +    ASSERT(nr_mfns);
> +    for ( unsigned long i = 0; i < nr_mfns; i++ )

This form of variable declaration gets warned about by some compiler
versions and may only be used once we settle on C99 as the base line
language level. There's one more such instance below, and the one
here is even worse in that it shadows a function scope variable.

> +        if ( !mfn_valid(mfn_add(smfn, i)) )
> +            return NULL;
> +
> +    pg = mfn_to_page(smfn);
> +
> +    spin_lock(&heap_lock);
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +    {
> +        /* The page should be reserved and not yet allocated. */
> +        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
> +        {
> +            printk(XENLOG_ERR
> +                   "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
> +                   i, mfn_x(smfn) + i,
> +                   pg[i].count_info, pg[i].tlbflush_timestamp);
> +            goto out_err;
> +        }
> +
> +        if ( !(memflags & MEMF_no_tlbflush) )
> +            accumulate_tlbflush(&need_tlbflush, &pg[i],
> +                                &tlbflush_timestamp);
> +
> +        /*
> +         * Preserve flag PGC_reserved and change page state
> +         * to PGC_state_inuse.
> +         */
> +        pg[i].count_info = PGC_reserved | PGC_state_inuse;
> +        /* Initialise fields which have other uses for free pages. */
> +        pg[i].u.inuse.type_info = 0;
> +        page_set_owner(&pg[i], NULL);
>      }
> +
> +    spin_unlock(&heap_lock);
> +
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);
> +
> +    /*
> +     * Ensure cache and RAM are consistent for platforms where the guest
> +     * can control its own visibility of/through the cache.
> +     */
> +    for ( i = 0; i < nr_mfns; i++ )
> +        flush_page_to_ram(mfn_x(smfn) + i, !(memflags & 
> MEMF_no_icache_flush));
> +
> +    return pg;
> +
> +out_err:

Please indent labels by at least one space.

> +    for ( unsigned long j = 0; j < i; j++ )

You don't need the extra variable here at all - simply use
"while ( i-- )".

Jan