Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2022 10:44, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Thursday, May 5, 2022 3:47 PM
>> To: Penny Zheng <Penny.Zheng@xxxxxxx>
>> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>;
>> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
>> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
>> Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> 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 <jbeulich@xxxxxxxx>
>>>> 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