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

[Julien Grall <julien@xen.org>]

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