On 05.05.2022 10:44, Penny Zheng wrote:
> Hi jan
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Thursday, May 5, 2022 3:47 PM
>> To: Penny Zheng <penny.zh...@arm.com>
>> Cc: Wei Chen <wei.c...@arm.com>; Henry Wang <henry.w...@arm.com>;
>> Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap
>> <george.dun...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano
>> Stabellini <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; xen-
>> de...@lists.xenproject.org
>> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on
>> populate_physmap
>>
>> On 05.05.2022 08:24, Penny Zheng wrote:
>>>> From: Jan Beulich <jbeul...@suse.com>
>>>> Sent: Wednesday, May 4, 2022 9:45 PM
>>>>
>>>> On 27.04.2022 11:27, Penny Zheng wrote:
>>>>> #else
>>>>> void free_staticmem_pages(struct page_info *pg, unsigned long
>> nr_mfns,
>>>>> bool need_scrub) {
>>>>> ASSERT_UNREACHABLE();
>>>>> }
>>>>> +
>>>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>>>>> + unsigned int nr_mfns, unsigned
>>>>> +int
>>>>> +memflags) {
>>>>> + ASSERT_UNREACHABLE();
>>>>> +}
>>>>
>>>> I can't spot a caller of this one outside of suitable #ifdef. Also
>>>> the __init here looks wrong and you look to have missed dropping it from
>> the real function.
>>>>
>>>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
>>>>> +memflags) {
>>>>> + ASSERT_UNREACHABLE();
>>>>> +}
>>>>> #endif
>>>>
>>>> For this one I'd again expect CSE to leave no callers, just like in
>>>> the earlier patch. Or am I overlooking anything?
>>>>
>>>
>>> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only
>>> variables, like
>>> d->resv_page_list, so I'd expect to let acquire_reserved_page guarded
>>> d->by CONFIG_STATIC_MEMORY
>>> too and provide the stub function here to avoid compilation error
>> when !CONFIG_STATIC_MEMORY.
>>
>> A compilation error should only result if there's no declaration of the
>> function. I didn't suggest to remove that. A missing definition would only be
>> noticed when linking, but CSE should result in no reference to the function
>> in
>> the first place. Just like was suggested for the earlier patch. And as
>> opposed
>> to the call site optimization the compiler can do, with -ffunction-sections
>> there's no way for the linker to eliminate the dead stub function.
>>
>
> Sure, plz correct me if I understand wrongly:
> Maybe here I should use #define xxx to do the declaration, and it will also
> avoid bringing dead stub function. Something like:
> #define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg),
> (void)(nr_mfns), (void)(need_scrub))
> And
> #define acquire_reserved_page(d, memflags) (INVALID_MFN)
No, I don't see why you would need #define-s. You want to have normal
declarations, but no definition unless STATIC_MEMORY. If that doesn't
work, please point out why (i.e. what I am overlooking).
Jan
