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.
