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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.07.2021 07:22, Penny Zheng wrote:
>> 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 can see that what you say may be true for Arm, but we're in generic
code here with an arch-independent CONFIG_STATIC_ALLOCATION conditional
around. Hence you want to avoid deliberately not handling a case that
can occur on e.g. x86 (see mark_page_offline() and further related
handling elsewhere). I'd perhaps view this differently if you were
introducing completely new code, but you've specifically said you're
cloning existing code (where the case is being handled). Plus, as said,
you'll likely be able to actually share code by not excluding the case
here.

Jan