RE: [PATCH 4/9] xen/arm: static memory initialization

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, July 5, 2021 3:51 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization
> 
> On 05.07.2021 09:14, Penny Zheng wrote:
> >> From: Penny Zheng
> >> Sent: Monday, July 5, 2021 1:22 PM
> >>
> >>> From: Julien Grall <julien@xxxxxxx>
> >>> Sent: Thursday, July 1, 2021 1:46 AM
> >>>
> >>> On 10/06/2021 10:35, Jan Beulich wrote:
> >>>> On 07.06.2021 04:43, Penny Zheng wrote:
> >>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages(
> >>>>>       spin_unlock(&heap_lock);
> >>>>>   }
> >>>>>
> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION
> >>>>> +/* Equivalent of free_heap_pages to free nr_mfns pages of static
> >>>>> +memory. */ void __init free_staticmem_pages(struct page_info *pg,
> >>> unsigned long nr_mfns,
> >>>>> +                                 bool need_scrub) {
> >>>>> +    mfn_t mfn = page_to_mfn(pg);
> >>>>> +    unsigned long i;
> >>>>> +
> >>>>> +    for ( i = 0; i < nr_mfns; i++ )
> >>>>> +    {
> >>>>> +        switch ( pg[i].count_info & PGC_state )
> >>>>> +        {
> >>>>> +        case PGC_state_inuse:
> >>>>> +            BUG_ON(pg[i].count_info & PGC_broken);
> >>>>> +            /* Mark it free and reserved. */
> >>>>> +            pg[i].count_info = PGC_state_free | PGC_reserved;
> >>>>> +            break;
> >>>>> +
> >>>>> +        default:
> >>>>> +            printk(XENLOG_ERR
> >>>>> +                   "Page state shall be only in PGC_state_inuse. "
> >>>>
> >>>> Why? A page (static or not) can become broken while in use. IOW I
> >>>> don't think you can avoid handling PGC_state_offlining here. At
> >>>> which point this code will match free_heap_pages()'es, and hence
> >>>> likely will want folding as well.
> >>>>
> >>
> >> Yeah, I was following the logic in free_heap_pages.
> >> Hmmm, I could not think of any scenario that will lead to
> >> PGC_state_offlining, that's why I was not including it at the first place.
> >> For broken issue, tbh, I just copy the bug_on from free_heap_pages,
> >> after quite a time thinking, I also could not find any scenario when
> >> a page(static or
> >> not) can become broken while in use. ;/
> 
> I'm, afraid I don't understand. Using page_to_mfn(), expensive or not, in
> ASSERT() is quite fine. The (expensive) expression won't be evaluated in
> release builds. This is specifically different from BUG_ON().
> 

Thanks for the explanation. 😉
I'll use the ASSERT() to do the check.

> Jan

Cheers

Penny Zheng