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
