Re: [PATCH V3 (resend) 05/19] x86/mapcache: Initialise the mapcache for the idle domain

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 13, 2024 at 01:40:32PM +0000, Elias El Yandouzi 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().

Oh, so this is the reason for the remark on the previous commit
message: idle domain init gets short-circuited earlier in
arch_domain_create() and never gets to the mapcache_domain_init()
call.

> 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.
> 
> Signed-off-by: Wei Wang <wawei@xxxxxxxxx>
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Elias El Yandouzi <eliasely@xxxxxxxxxx>
> 
> ----
> 
>       Changes in V2:
>           * Free resources if mapcache initialisation fails
>           * Remove `is_idle_domain()` check from `create_perdomain_mappings()`

I'm not seeing any is_idle_domain() in create_perdomain_mapping(), and
neither anything removed by this patch.

> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 507d704f16..3303bdb53e 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -758,9 +758,16 @@ int arch_domain_create(struct domain *d,
>  
>      spin_lock_init(&d->arch.e820_lock);
>  
> +    if ( (rc = mapcache_domain_init(d)) != 0)
> +    {
> +        free_perdomain_mappings(d);
> +        return rc;


What about all the error paths below here that don't use the fail
label, don't you need to also call free_perdomain_mappings() on them?

Or alternatively arrange the fail label to be suitable to be used this
early if it's not already the case.

> +    }
> +
>      /* Minimal initialisation for the idle domain. */
>      if ( unlikely(is_idle_domain(d)) )
>      {
> +        struct page_info *pg = d->arch.perdomain_l3_pg;

const?

>          static const struct arch_csw idle_csw = {
>              .from = paravirt_ctxt_switch_from,
>              .to   = paravirt_ctxt_switch_to,
> @@ -771,6 +778,9 @@ int arch_domain_create(struct domain *d,
>  
>          d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>  
> +        idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
> +            l4e_from_page(pg, __PAGE_HYPERVISOR_RW);

Albeit I think you could just use d->arch.perdomain_l3_pg directly
here and avoid the local pg variable?

Would you mind adding:

/* Slot 260: Per-domain mappings. */

I wonder if it won't be better to just use init_xen_l4_slots() and
special case the idle domain in there, instead of open-coding the L4
population for the idle domain like this.

Thanks, Roger.