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
