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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

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)

> Jan