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 4:51 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 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
> >>> d->guarded 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).
> 

I was trying to avoid dead stub function, and I think #define-s is the wrong 
path...
So, I guess If I remove the ASSERT_UNREACHABLE() part and only leave the empty
function body there, the CSE could do the optimization and result in no 
reference.

> Jan