[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.