[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 16/08/2021 07:43, Penny Zheng wrote:
Hi Julien,

Hi,

+{
+    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. */

This comment doesn't seem to match the check below.

+    if ( !mfn_valid(smfn) || !nr_mfns )

This check only guarantees that there will be a page for the first MFN.
Shouldn't we also check for the other MFNs?


Hmm, Do you think that it should be all checked, the whole range, [smfn, smfn + 
nr_mfns).
Since it is in linear growth, maybe adding another check of "!mfn_valid(smfn + 
nr_mfns - 1)"
is enough?

The only requirement for Xen is to provide a valid struct page for each RAM page. So checking the first and end page may not be sufficient if there is a hole in the middle.

My point here is either:
  - we trust the callers so we remove the mfn_valid() check
  - we don't trust the callers so we check the MFN is valid for every page

My preference is the latter, the more if we plan to us the helper after boot in the future. An possible compromise is to add a comment that this function needs to be reworked if used outside of boot.

Cheers,


--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.