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

Commit Message

Penny Zheng April 27, 2022, 9:27 a.m. UTC

When static domain populates memory through populate_physmap on runtime,
other than allocating from heap, it shall retrieve reserved pages from
resv_page_list to make sure that guest RAM is still restricted in statically
configured memory regions. And this commit introduces a new helper
acquire_reserved_page to make it work.

Signed-off-by: Penny Zheng <[e-mail]>
---
v3 changes
- move #ifndef is_domain_using_staticmem to the common header file
- remove #ifdef CONFIG_STATIC_MEMORY-ary
- remove meaningless page_to_mfn(page) in error log
---
v2 changes:
- introduce acquire_reserved_page to retrieve reserved pages from
resv_page_list
- forbid non-zero-order requests in populate_physmap
- let is_domain_static return ((void)(d), false) on x86
---
 xen/common/memory.c      | 23 +++++++++++++++++++++++
 xen/common/page_alloc.c  | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/domain.h |  4 ++++
 xen/include/xen/mm.h     |  3 +--
 4 files changed, 66 insertions(+), 2 deletions(-)

Comments

Jan Beulich May 4, 2022, 1:44 p.m. UTC | #1

On 27.04.2022 11:27, 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 retreived pages are contiguous,

Nit: retrieved

> +                 * so forbid non-zero-order requests here.
> +                 */
> +                if ( a->extent_order != 0 )
> +                {
> +                    gdprintk(XENLOG_INFO,
> +                             "Could not allocate non-zero-order pages for static %pd.\n.",

Nit: "Could not" reads as if an attempt was made, so maybe better "Cannot"?
I'd also pull "static" ahead of "non-zero-order" and, to help observers of
the message associate it with a call site, actually log the order (i.e.
"order-%u" instead of "non-zero-order").

Also please omit full stops in log messages. They serve no purpose but
consume space.

Finally, here as well as below: Is "info" log level really appropriate?
You're logging error conditions after all, so imo these want to be at
least "warn" level. An alternative would be to omit logging of messages
here altogether.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2769,12 +2769,50 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>  
>      return 0;
>  }
> +
> +/*
> + * Acquire a page from reserved page list(resv_page_list), when populating
> + * memory for static domain on runtime.
> + */
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    struct page_info *page;
> +    mfn_t smfn;
> +
> +    /* Acquire a page from reserved page list(resv_page_list). */
> +    page = page_list_remove_head(&d->resv_page_list);
> +    if ( unlikely(!page) )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: failed to acquire a reserved page from resv_page_list.\n",
> +               d);

A gdprintk() in the caller is acceptable. Two log messages isn't imo,
and a XENLOG_ERR message which a guest can trigger is a security concern
(log spam) anyway.

> +        return INVALID_MFN;
> +    }
> +
> +    smfn = page_to_mfn(page);
> +
> +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
> +        return INVALID_MFN;

Don't you want to add the page back to the reserved list in case of error?

> +    return smfn;
> +}
>  #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?

Jan

Penny Zheng May 5, 2022, 6:24 a.m. UTC | #2

Hi jan

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Wednesday, May 4, 2022 9:45 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
> Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano
> Stabellini <[e-mail]>; Wei Liu <[e-mail]>; xen-
> [e-mail]
> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on
> populate_physmap
> 
> On 27.04.2022 11:27, 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 retreived pages are
> > + contiguous,
> 
> Nit: retrieved
> 
> > +                 * so forbid non-zero-order requests here.
> > +                 */
> > +                if ( a->extent_order != 0 )
> > +                {
> > +                    gdprintk(XENLOG_INFO,
> > +                             "Could not allocate non-zero-order pages
> > + for static %pd.\n.",
> 
> Nit: "Could not" reads as if an attempt was made, so maybe better "Cannot"?
> I'd also pull "static" ahead of "non-zero-order" and, to help observers of the
> message associate it with a call site, actually log the order (i.e.
> "order-%u" instead of "non-zero-order").
> 
> Also please omit full stops in log messages. They serve no purpose but
> consume space.
> 
> Finally, here as well as below: Is "info" log level really appropriate?
> You're logging error conditions after all, so imo these want to be at least
> "warn" level. An alternative would be to omit logging of messages here
> altogether.
> 
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2769,12 +2769,50 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >      return 0;
> >  }
> > +
> > +/*
> > + * Acquire a page from reserved page list(resv_page_list), when
> > +populating
> > + * memory for static domain on runtime.
> > + */
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> > +{
> > +    struct page_info *page;
> > +    mfn_t smfn;
> > +
> > +    /* Acquire a page from reserved page list(resv_page_list). */
> > +    page = page_list_remove_head(&d->resv_page_list);
> > +    if ( unlikely(!page) )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "%pd: failed to acquire a reserved page from resv_page_list.\n",
> > +               d);
> 
> A gdprintk() in the caller is acceptable. Two log messages isn't imo, and a
> XENLOG_ERR message which a guest can trigger is a security concern (log
> spam) anyway.
> 
> > +        return INVALID_MFN;
> > +    }
> > +
> > +    smfn = page_to_mfn(page);
> > +
> > +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
> > +        return INVALID_MFN;
> 
> Don't you want to add the page back to the reserved list in case of error?
> 

Oh, thanks for pointing that out.

> > +    return smfn;
> > +}
> >  #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 by CONFIG_STATIC_MEMORY
too and provide the stub function here to avoid compilation error when !CONFIG_STATIC_MEMORY.


> Jan

Jan Beulich May 5, 2022, 7:46 a.m. UTC | #3

On 05.05.2022 08:24, Penny Zheng wrote:
>> From: Jan Beulich <[e-mail]>
>> 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 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.

Jan

Penny Zheng May 5, 2022, 8:44 a.m. UTC | #4

Hi jan

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Thursday, May 5, 2022 3:47 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
> Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano
> Stabellini <[e-mail]>; Wei Liu <[e-mail]>; xen-
> [e-mail]
> 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 <[e-mail]>
> >> 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

Jan Beulich May 5, 2022, 8:50 a.m. UTC | #5

On 05.05.2022 10:44, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: Thursday, May 5, 2022 3:47 PM
>> To: Penny Zheng <[e-mail]>
>> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
>> Andrew Cooper <[e-mail]>; George Dunlap
>> <[e-mail]>; Julien Grall <[e-mail]>; Stefano
>> Stabellini <[e-mail]>; Wei Liu <[e-mail]>; xen-
>> [e-mail]
>> 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 <[e-mail]>
>>>> 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

Penny Zheng May 5, 2022, 9:29 a.m. UTC | #6

Hi jan

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Thursday, May 5, 2022 4:51 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
> Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano
> Stabellini <[e-mail]>; Wei Liu <[e-mail]>; xen-
> [e-mail]
> 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 <[e-mail]>
> >> Sent: Thursday, May 5, 2022 3:47 PM
> >> To: Penny Zheng <[e-mail]>
> >> Cc: Wei Chen <[e-mail]>; Henry Wang
> <[e-mail]>;
> >> Andrew Cooper <[e-mail]>; George Dunlap
> >> <[e-mail]>; Julien Grall <[e-mail]>; Stefano
> >> Stabellini <[e-mail]>; Wei Liu <[e-mail]>; xen-
> >> [e-mail]
> >> 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 <[e-mail]>
> >>>> 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

Jan Beulich May 5, 2022, 12:06 p.m. UTC | #7

On 05.05.2022 11:29, Penny Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: Thursday, May 5, 2022 4:51 PM
>>
>> On 05.05.2022 10:44, Penny Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <[e-mail]>
>>>> Sent: Thursday, May 5, 2022 3:47 PM
>>>>
>>>> On 05.05.2022 08:24, Penny Zheng wrote:
>>>>>> From: Jan Beulich <[e-mail]>
>>>>>> 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.

No, DCE (I'm sorry for the earlier wrong uses of CSE) cannot eliminate a
function, it can only eliminate call sites. Hence it doesn't matter whether
a function is empty. And no, if a stub function needs retaining, the
ASSERT_UNREACHABLE() should also remain there if the function indeed is
supposed to never be called.

Jan

Penny Zheng May 5, 2022, 1:44 p.m. UTC | #8

Hi Jan

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Thursday, May 5, 2022 8:07 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
> Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Wei Liu <[e-mail]>; xen-
> [e-mail]
> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> On 05.05.2022 11:29, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: Thursday, May 5, 2022 4:51 PM
> >>
> >> On 05.05.2022 10:44, Penny Zheng wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <[e-mail]>
> >>>> Sent: Thursday, May 5, 2022 3:47 PM
> >>>>
> >>>> On 05.05.2022 08:24, Penny Zheng wrote:
> >>>>>> From: Jan Beulich <[e-mail]>
> >>>>>> 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.
> 
> No, DCE (I'm sorry for the earlier wrong uses of CSE) cannot eliminate a
> function, it can only eliminate call sites. Hence it doesn't matter whether a
> function is empty. And no, if a stub function needs retaining, the
> ASSERT_UNREACHABLE() should also remain there if the function indeed is
> supposed to never be called.
>

Ok. Thanks for explanation.
I misunderstand what you suggested here, I thought you were suggesting a way of stub function
which could bring some optimization.
The reason I introduced free_staticmem_pages and acquire_reserved_page here is that
we now used them in common code, and if they are not defined(using stub) on !CONFIG_STATIC_MEMORY,
we will have " hidden symbol `xxx' isn't defined " compilation error.
 
> Jan

Jan Beulich May 5, 2022, 2:23 p.m. UTC | #9

On 05.05.2022 15:44, Penny Zheng wrote:
> I misunderstand what you suggested here, I thought you were suggesting a way of stub function
> which could bring some optimization.
> The reason I introduced free_staticmem_pages and acquire_reserved_page here is that
> we now used them in common code, and if they are not defined(using stub) on !CONFIG_STATIC_MEMORY,
> we will have " hidden symbol `xxx' isn't defined " compilation error.

This is what I've asked for clarification about: If such errors surface,
I'd like to understand why the respective call sites aren't DCE-ed by
the compiler.

Jan

Penny Zheng May 6, 2022, 2:59 a.m. UTC | #10

Hi jan

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Thursday, May 5, 2022 10:23 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
> Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Wei Liu <[e-mail]>; xen-
> [e-mail]
> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> On 05.05.2022 15:44, Penny Zheng wrote:
> > I misunderstand what you suggested here, I thought you were suggesting
> > a way of stub function which could bring some optimization.
> > The reason I introduced free_staticmem_pages and acquire_reserved_page
> > here is that we now used them in common code, and if they are not
> > defined(using stub) on !CONFIG_STATIC_MEMORY, we will have " hidden
> symbol `xxx' isn't defined " compilation error.
> 
> This is what I've asked for clarification about: If such errors surface, I'd like to
> understand why the respective call sites aren't DCE-ed by the compiler.
> 

Because both definition of PGC_reserved and is_domain_using_static_memory are
not guarded by CONFIG_STATIC_MEMORY in the first place in arm-specific file.

> Jan

Jan Beulich May 6, 2022, 6:14 a.m. UTC | #11

On 06.05.2022 04:59, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: Thursday, May 5, 2022 10:23 PM
>> To: Penny Zheng <[e-mail]>
>> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
>> Andrew Cooper <[e-mail]>; George Dunlap
>> <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Wei Liu <[e-mail]>; xen-
>> [e-mail]
>> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap
>>
>> On 05.05.2022 15:44, Penny Zheng wrote:
>>> I misunderstand what you suggested here, I thought you were suggesting
>>> a way of stub function which could bring some optimization.
>>> The reason I introduced free_staticmem_pages and acquire_reserved_page
>>> here is that we now used them in common code, and if they are not
>>> defined(using stub) on !CONFIG_STATIC_MEMORY, we will have " hidden
>> symbol `xxx' isn't defined " compilation error.
>>
>> This is what I've asked for clarification about: If such errors surface, I'd like to
>> understand why the respective call sites aren't DCE-ed by the compiler.
>>
> 
> Because both definition of PGC_reserved and is_domain_using_static_memory are
> not guarded by CONFIG_STATIC_MEMORY in the first place in arm-specific file.

So perhaps that's what wants changing (at least for PGC_reserved)?

Jan

Penny Zheng May 6, 2022, 7:41 a.m. UTC | #12

Hi jan and julien

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Friday, May 6, 2022 2:14 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Henry Wang <[e-mail]>;
> Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Wei Liu <[e-mail]>; xen-
> [e-mail]
> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> On 06.05.2022 04:59, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: Thursday, May 5, 2022 10:23 PM
> >> To: Penny Zheng <[e-mail]>
> >> Cc: Wei Chen <[e-mail]>; Henry Wang
> <[e-mail]>;
> >> Andrew Cooper <[e-mail]>; George Dunlap
> >> <[e-mail]>; Julien Grall <[e-mail]>; Stefano
> >> Stabellini <[e-mail]>; Wei Liu <[e-mail]>; xen-
> >> [e-mail]
> >> Subject: Re: [PATCH v3 6/6] xen: retrieve reserved pages on
> >> populate_physmap
> >>
> >> On 05.05.2022 15:44, Penny Zheng wrote:
> >>> I misunderstand what you suggested here, I thought you were
> >>> suggesting a way of stub function which could bring some optimization.
> >>> The reason I introduced free_staticmem_pages and
> >>> acquire_reserved_page here is that we now used them in common code,
> >>> and if they are not defined(using stub) on !CONFIG_STATIC_MEMORY, we
> >>> will have " hidden
> >> symbol `xxx' isn't defined " compilation error.
> >>
> >> This is what I've asked for clarification about: If such errors
> >> surface, I'd like to understand why the respective call sites aren't DCE-ed by
> the compiler.
> >>
> >
> > Because both definition of PGC_reserved and
> > is_domain_using_static_memory are not guarded by
> CONFIG_STATIC_MEMORY in the first place in arm-specific file.
> 
> So perhaps that's what wants changing (at least for PGC_reserved)?
> 

Hmmm, I remembered that when I firstly introduced PGC_reserved through
"Domain on static allocation", Julien commented that he may like it to be
used for other purpose, not only static memory. And one example is reserved
memory when Live Updating.(https://www.mail-archive.com/[e-mail]/msg97829.html
)

> Jan

Patch

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 69b0cd1e50..6cee51f0e3 100644
--- 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 retreived pages are contiguous,
+                 * so forbid non-zero-order requests here.
+                 */
+                if ( a->extent_order != 0 )
+                {
+                    gdprintk(XENLOG_INFO,
+                             "Could not allocate non-zero-order pages for static %pd.\n.",
+                             d);
+                    goto out;
+                }
+
+                mfn = acquire_reserved_page(d, a->memflags);
+                if ( mfn_eq(mfn, INVALID_MFN) )
+                {
+                    gdprintk(XENLOG_INFO,
+                             "%pd: failed to retrieve a reserved page.\n.",
+                             d);
+                    goto out;
+                }
+            }
             else
             {
                 page = alloc_domheap_pages(d, a->extent_order, a->memflags);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 1f3ad4bd28..78cc52986c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2769,12 +2769,50 @@  int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 
     return 0;
 }
+
+/*
+ * Acquire a page from reserved page list(resv_page_list), when populating
+ * memory for static domain on runtime.
+ */
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
+{
+    struct page_info *page;
+    mfn_t smfn;
+
+    /* Acquire a page from reserved page list(resv_page_list). */
+    page = page_list_remove_head(&d->resv_page_list);
+    if ( unlikely(!page) )
+    {
+        printk(XENLOG_ERR
+               "%pd: failed to acquire a reserved page from resv_page_list.\n",
+               d);
+        return INVALID_MFN;
+    }
+
+    smfn = page_to_mfn(page);
+
+    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
+        return INVALID_MFN;
+
+    return smfn;
+}
 #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();
+}
+
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
+{
+    ASSERT_UNREACHABLE();
+}
 #endif
 
 /*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 35dc7143a4..c613afa57e 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -38,6 +38,10 @@  void arch_get_domain_info(const struct domain *d,
 #define CDF_staticmem            (1U << 2)
 #endif
 
+#ifndef is_domain_using_staticmem
+#define is_domain_using_staticmem(d) ((void)(d), false)
+#endif
+
 /*
  * Arch-specifics.
  */
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 9fd95deaec..32b0837fa0 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -88,10 +88,9 @@  bool scrub_free_pages(void);
 /* These functions are for static memory */
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
-#ifdef CONFIG_STATIC_MEMORY
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
-#endif
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags);
 
 /* Map machine page range in Xen virtual address space. */
 int map_pages_to_xen(