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

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

On Wed, Aug 21, 2024 at 05:42:26PM +0100, Alejandro Vallejo wrote:
> 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.

Sure, moved to the else branch.

> > +            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"?

I have to admit I found this coding style weird at first, but it's
used all over Xen.  I don't mind setting rc ahead of the goto, AFAICT
the only benefit of the current style is that we can avoid the braces
around the if code block for it being a single statement.

> > +            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.

Indeed, vcpus are only destroyed when destroying domains, and system
domains are never destroyed.

Thanks, Roger.