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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 20/12/2022 15:08, Jan Beulich wrote:
> On 16.12.2022 12:48, Julien Grall wrote:
> > From: Wei Liu <wei.liu2@xxxxxxxxxx>
> >
> > After the direct map removal, pages from the boot allocator are not
> > mapped at all in the direct map. Although we have map_domain_page, they
> Nit: "will not be mapped" or "are not going to be mapped", or else this
> sounds like there's a bug somewhere.
I will update.

> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -870,6 +870,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >       unsigned long eb_start, eb_end;
> >       bool acpi_boot_table_init_done = false, relocated = false;
> >       int ret;
> > +    bool vm_init_done = false;
> Can this please be grouped with the other bool-s (even visible in context)?
> > --- 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.
This seems unlikely, yet I think that relying on clear_page() to always 
fail when map_pages_to_xen() return an error is bogus.
Cheers,

--
Julien Grall