Re: [PATCH v5 9/9] xen: retrieve reserved pages on populate_physmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.05.2022 11:35, Julien Grall wrote:
> On 31/05/2022 09:54, Jan Beulich wrote:
>> On 31.05.2022 05:12, Penny Zheng wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -245,6 +245,29 @@ static void populate_physmap(struct memop_args *a)
>>>   
>>>                   mfn = _mfn(gpfn);
>>>               }
>>> +            else if ( is_domain_using_staticmem(d) )
>>> +            {
>>> +                /*
>>> +                 * No easy way to guarantee the retrieved pages are 
>>> contiguous,
>>> +                 * so forbid non-zero-order requests here.
>>> +                 */
>>> +                if ( a->extent_order != 0 )
>>> +                {
>>> +                    gdprintk(XENLOG_WARNING,
>>> +                             "Cannot allocate static order-%u pages for 
>>> static %pd\n",
>>> +                             a->extent_order, d);
>>> +                    goto out;
>>> +                }
>>> +
>>> +                mfn = acquire_reserved_page(d, a->memflags);
>>> +                if ( mfn_eq(mfn, INVALID_MFN) )
>>> +                {
>>> +                    gdprintk(XENLOG_WARNING,
>>> +                             "%pd: failed to retrieve a reserved page\n",
>>> +                             d);
>>> +                    goto out;
>>> +                }
>>> +            }
>>
>> I'm not convinced of having these gdprintk()s here. 
> 
> There are a number of time where I wished some error paths would contain 
> debug printk(). Instead, I often end up to add them myself when I 
> struggle to find the reason of a failure.

But this model doesn't scale - we don't want to have log messages on
each and every error path. I agree having such for very unlikely
errors, but order != 0 is clearly a call site mistake and memory
allocation requests failing also ought to not be entirely unexpected.

Jan