Re: [PATCH 04/22] x86/mm: ensure L4 idle_pg_table is not modified past boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2024 17:21, Roger Pau Monne wrote:
> The idle_pg_table L4 is cloned to create all the other L4 Xen uses, and hence
> it shouldn't be modified once further L4 are created.

Yes, but the window between moving into SYS_STATE_smp_boot and Dom0 having
its initial page tables created is quite large. If the justification was
relative to AP bringup, that may be all fine. But when related to cloning,
I think that would then truly want keying to there being any non-system
domain(s).

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5023,6 +5023,12 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>          mfn_t l3mfn;
>          l3_pgentry_t *l3t = alloc_mapped_pagetable(&l3mfn);
>  
> +        /*
> +         * dom0 is build at smp_boot, at which point we already create new 
> L4s
> +         * based on idle_pg_table.
> +         */
> +        BUG_ON(system_state >= SYS_STATE_smp_boot);

Which effectively means most of this function could become __init (e.g. by
moving into a helper). We'd then hit the BUG_ON() prior to init_done()
destroying the .init.* mappings, and we'd simply #PF afterwards. That's
not so much for the space savings in .text, but to document the limited
lifetime of the (helper) function directly in its function head.

I further wonder whether in such a case the enclosing if() wouldn't want
to gain unlikely() at the same time.

Jan