Re: [PATCH V3 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages

From: Jan Beulich <jbeulich@suse.com>
To: Penny Zheng <penny.zheng@arm.com>
Cc: Bertrand.Marquis@arm.com, Wei.Chen@arm.com, nd@arm.com,
	xen-devel@lists.xenproject.org, sstabellini@kernel.org,
	julien@xen.org
Subject: Re: [PATCH V3 08/10] xen/arm: introduce acquire_staticmem_pages and acquire_domstatic_pages
Date: Mon, 19 Jul 2021 11:26:04 +0200	[thread overview]
Message-ID: <48c59068-296e-7060-1352-f07338833378@suse.com> (raw)
In-Reply-To: <20210715051819.3073628-9-penny.zheng@arm.com>

On 15.07.2021 07:18, Penny Zheng wrote:
> @@ -1065,6 +1069,73 @@ static struct page_info *alloc_heap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_MEMORY
> +/*
> + * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
> + * static memory.
> + */
> +static struct page_info *acquire_staticmem_pages(unsigned long nr_mfns,
> +                                                 mfn_t smfn,
> +                                                 unsigned int memflags)

This and the other function not being __init, and there neither being
any caller nor any freeing, a question is whether __init wants adding;
patch 10 suggests so. The lack of freeing in particular means that
domains created using static memory won't be restartable, requiring a
host reboot instead.

> +{
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
> +    unsigned long i;
> +    struct page_info *pg;
> +
> +    /* For now, it only supports pre-configured static memory. */
> +    if ( !mfn_valid(smfn) || !nr_mfns )
> +        return NULL;
> +
> +    spin_lock(&heap_lock);
> +
> +    pg = mfn_to_page(smfn);
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +    {
> +        /*
> +         * Reference count must continuously be zero for free pages
> +         * of static memory(PGC_reserved).
> +         */
> +        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(page_to_mfn(pg + i)),
> +                   pg[i].count_info, pg[i].tlbflush_timestamp);
> +            BUG();
> +        }
> +
> +        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);
> +
> +        /*
> +         * Ensure cache and RAM are consistent for platforms where the
> +         * guest can control its own visibility of/through the cache.
> +         */
> +        flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])),
> +                            !(memflags & MEMF_no_icache_flush));

Indentation.

> +    }
> +
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);
> +
> +    spin_unlock(&heap_lock);

I'm pretty sure I did comment before on the flush_page_to_ram() being
inside the locked region here. While XSA-364 doesn't apply here because
you don't defer scrubbing (yet), the flushing may still take too long
to happen inside the locked region. Of course there's a dependency here
on when exactly the call(s) to this code will happen. In particular if
other DomU-s can already be running at that point, this may not be
tolerable by some of them. Yet if this is intentional and deemed not a
problem, then I'd have kind of expected the description to mention this
difference from alloc_heap_pages(), which you say this is an equivalent
of.

> @@ -2411,6 +2483,42 @@ struct page_info *alloc_domheap_pages(
>      return pg;
>  }
>  
> +#ifdef CONFIG_STATIC_MEMORY
> +/*
> + * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
> + * then assign them to one specific domain #d.
> + */
> +struct page_info *acquire_domstatic_pages(
> +        struct domain *d, unsigned long nr_mfns, mfn_t smfn,
> +        unsigned int memflags)
> +{
> +    struct page_info *pg = NULL;
> +
> +    ASSERT(!in_irq());
> +
> +    pg = acquire_staticmem_pages(nr_mfns, smfn, memflags);
> +    if ( !pg )
> +        return NULL;
> +
> +    /* Right now, MEMF_no_owner case is meaningless here. */
> +    ASSERT(d);
> +    if ( memflags & MEMF_no_refcount )
> +    {
> +        unsigned long i;
> +
> +        for ( i = 0; i < nr_mfns; i++ )
> +            pg[i].count_info |= PGC_extra;
> +    }

With MEMF_no_owner now called out as meaningless here, what about
MEMF_no_refcount?

Jan