Re: [PATCH 10/22] x86/mapcache: initialise the mapcache for the idle domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia <hongyxia@xxxxxxxxxx>
> 
> In order to use the mapcache in the idle domain, we also have to
> populate its page tables in the PERDOMAIN region, and we need to move
> mapcache_domain_init() earlier in arch_domain_create().
> 
> Note, commit 'x86: lift mapcache variable to the arch level' has
> initialised the mapcache for HVM domains. With this patch, PV, HVM,
> idle domains now all initialise the mapcache.

But they can't use it yet, can they? This needs saying explicitly, or
else one is going to make wrong implications.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -732,6 +732,8 @@ int arch_domain_create(struct domain *d,
>  
>      spin_lock_init(&d->arch.e820_lock);
>  
> +    mapcache_domain_init(d);
> +
>      /* Minimal initialisation for the idle domain. */
>      if ( unlikely(is_idle_domain(d)) )
>      {
> @@ -829,8 +831,6 @@ int arch_domain_create(struct domain *d,
>  
>      psr_domain_init(d);
>  
> -    mapcache_domain_init(d);

You move this ahead of error paths taking the "goto out" route, so
adjustments to affected error paths are going to be needed to avoid
memory leaks.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5963,6 +5963,9 @@ int create_perdomain_mapping(struct domain *d, unsigned 
> long va,
>          l3tab = __map_domain_page(pg);
>          clear_page(l3tab);
>          d->arch.perdomain_l3_pg = pg;
> +        if ( is_idle_domain(d) )
> +            idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
> +                l4e_from_page(pg, __PAGE_HYPERVISOR_RW);

Hmm, having an idle domain check here isn't very nice. I agree putting
it in arch_domain_create()'s respective conditional isn't very neat
either, but personally I'd consider this at least a little less bad.
And the layering violation aspect isn't much worse than that of setting
d->arch.ctxt_switch there as well.

Jan