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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 21/12/2022 10:22, Jan Beulich wrote:
> 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).
I have moved the change in a separate patch.

Cheers,

> Jan
--
Julien Grall