On 24/05/2021 11:10, Penny Zheng wrote:
Hi Julien
Hi Penny,
+ if ( !pg )
+ return NULL;
+
+ for ( i = 0; i < nr_pfns; i++)
+ {
+ /*
+ * Reference count must continuously be zero for free pages
+ * of static memory(PGC_reserved).
+ */
+ ASSERT(pg[i].count_info & PGC_reserved);
+ if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free )
+ {
+ printk(XENLOG_ERR
+ "Reference count must continuously be zero for free pages"
+ "pg[%u] 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();
So we would crash Xen if the caller pass a wrong range. Is it what we want?
Also, who is going to prevent concurrent access?
Sure, to fix concurrency issue, I may need to add one spinlock like `static
DEFINE_SPINLOCK(staticmem_lock);`
In current alloc_heap_pages, it will do similar check, that pages in free state
MUST have zero reference count. I guess, if condition not met, there is no need
to proceed.
Another thought on concurrency problem, when constructing patch v2, do we need
to
consider concurrency here?
heap_lock is to take care concurrent allocation on the one heap, but static
memory is
always reserved for only one specific domain.
In theory yes, but you are relying on the admin to correctly write the
device-tree nodes.
You are probably not going to hit the problem today because the domains
are created one by one. But, as you may want to allocate memory at
runtime, this is quite important to get the code protected from
concurrent access.
Here, you will likely want to use the heaplock rather than a new lock.
So you are also protect against concurrent access to count_info from
other part of Xen.
Cheers,
--
Julien Grall
