Re: [PATCH 15/22] x86/idle: allow using a per-pCPU L4

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

On Fri Jul 26, 2024 at 4:21 PM BST, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9cfcf0dc63f3..b62c4311da6c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -555,6 +555,7 @@ void arch_vcpu_regs_init(struct vcpu *v)
>  int arch_vcpu_create(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> +    root_pgentry_t *pgt = NULL;
>      int rc;
>  
>      v->arch.flags = TF_kernel_mode;
> @@ -589,7 +590,23 @@ int arch_vcpu_create(struct vcpu *v)
>      else
>      {
>          /* Idle domain */
> -        v->arch.cr3 = __pa(idle_pg_table);
> +        if ( (opt_asi_pv || opt_asi_hvm) && v->vcpu_id )
> +        {
> +            pgt = alloc_xenheap_page();
> +
> +            /*
> +             * For the idle vCPU 0 (the BSP idle vCPU) use idle_pg_table
> +             * directly, there's no need to create yet another copy.
> +             */

Shouldn't this comment be in the else branch instead? Or reworded to refer to
non-0 vCPUs.

> +            rc = -ENOMEM;

While it's true rc is overriden later, I feel uneasy leaving it with -ENOMEM
after the check. Could we have it immediately before "goto fail"?

> +            if ( !pgt )
> +                goto fail;
> +
> +            copy_page(pgt, idle_pg_table);
> +            v->arch.cr3 = __pa(pgt);
> +        }
> +        else
> +            v->arch.cr3 = __pa(idle_pg_table);
>          rc = 0;
>          v->arch.msrs = ZERO_BLOCK_PTR; /* Catch stray misuses */
>      }
> @@ -611,6 +628,7 @@ int arch_vcpu_create(struct vcpu *v)
>      vcpu_destroy_fpu(v);
>      xfree(v->arch.msrs);
>      v->arch.msrs = NULL;
> +    free_xenheap_page(pgt);
>  
>      return rc;
>  }

I guess the idle domain has a forever lifetime and its vCPUs are kept around
forever too, right?; otherwise we'd need extra logic in the the vcpu_destroy()
to free the page table copies should they exist too.

Cheers,
Alejandro