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
