Re: [PATCH 02/22] x86/setup: move vm_init() before acpi calls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2022 11:18, Julien Grall wrote:
> On 20/12/2022 15:08, Jan Beulich wrote:
>> On 16.12.2022 12:48, Julien Grall wrote:
>>> --- a/xen/common/vmap.c
>>> +++ b/xen/common/vmap.c
>>> @@ -34,9 +34,20 @@ void __init vm_init_type(enum vmap_region type, void 
>>> *start, void *end)
>>>   
>>>       for ( i = 0, va = (unsigned long)vm_bitmap(type); i < nr; ++i, va += 
>>> PAGE_SIZE )
>>>       {
>>> -        struct page_info *pg = alloc_domheap_page(NULL, 0);
>>> +        mfn_t mfn;
>>> +        int rc;
>>>   
>>> -        map_pages_to_xen(va, page_to_mfn(pg), 1, PAGE_HYPERVISOR);
>>> +        if ( system_state == SYS_STATE_early_boot )
>>> +            mfn = alloc_boot_pages(1, 1);
>>> +        else
>>> +        {
>>> +            struct page_info *pg = alloc_domheap_page(NULL, 0);
>>> +
>>> +            BUG_ON(!pg);
>>> +            mfn = page_to_mfn(pg);
>>> +        }
>>> +        rc = map_pages_to_xen(va, mfn, 1, PAGE_HYPERVISOR);
>>> +        BUG_ON(rc);
>>
>> The adding of a return value check is unrelated and not overly useful:
>>
>>>           clear_page((void *)va);
>>
>> This will fault anyway if the mapping attempt failed.
> 
> Not always. At least on Arm, map_pages_to_xen() could fail if the VA was 
> mapped to another physical address.

Oh, okay.

> This seems unlikely, yet I think that relying on clear_page() to always 
> fail when map_pages_to_xen() return an error is bogus.

Fair enough, but then please at least call out the change (and the reason)
in the description. Even better might be to make this a separate change,
but I wouldn't insist (quite likely I wouldn't have made this a separate
change either).

Jan