Re: [PATCH v2 02/18] x86/domain: limit window where curr_vcpu != current on context switch

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jan 09, 2025 at 09:59:58AM +0100, Jan Beulich wrote:
> On 08.01.2025 15:26, Roger Pau Monne wrote:
> > On x86 Xen will perform lazy context switches to the idle vCPU, where the
> > previously running vCPU context is not overwritten, and only current is updated
> > to point to the idle vCPU.  The state is then disjunct between current and
> > curr_vcpu: current points to the idle vCPU, while curr_vcpu points to the vCPU
> > whose context is loaded on the pCPU.
> > 
> > While on that lazy context switched state, certain calls (like
> > map_domain_page()) will trigger a full synchronization of the pCPU state by
> > forcing a context switch.  Note however how calling any of such functions
> > inside the context switch code itself is very likely to trigger an infinite
> > recursion loop.
> > 
> > Attempt to limit the window where curr_vcpu != current in the context switch
> > code, as to prevent and infinite recursion loop around sync_local_execstate().
> > 
> > This is required for using map_domain_page() in the vCPU context switch code,
> > otherwise using map_domain_page() in that context ends up in a recursive
> > sync_local_execstate() loop:
> 
> Question is whether it's a good idea in the first place to start using
> map_domain_page() from the context switch path. Surely there are possible
> alternatives.

It seemed more natural rather the introducing yet something new to use
in the context switch path.  I'm happy to hear recommendations, but
overall introducing yet another interface to map stuff just for the
context switch path seems worse than extending an existing interface
to work in that context.

> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1982,16 +1982,16 @@ static void load_default_gdt(unsigned int cpu)
> >      per_cpu(full_gdt_loaded, cpu) = false;
> >  }
> >  
> > -static void __context_switch(void)
> > +static void __context_switch(struct vcpu *n)
> >  {
> >      struct cpu_user_regs *stack_regs = guest_cpu_user_regs();
> >      unsigned int          cpu = smp_processor_id();
> >      struct vcpu          *p = per_cpu(curr_vcpu, cpu);
> > -    struct vcpu          *n = current;
> >      struct domain        *pd = p->domain, *nd = n->domain;
> >  
> >      ASSERT(p != n);
> >      ASSERT(!vcpu_cpu_dirty(n));
> > +    ASSERT(p == current);
> >  
> >      if ( !is_idle_domain(pd) )
> >      {
> > @@ -2036,6 +2036,18 @@ static void __context_switch(void)
> >  
> >      write_ptbase(n);
> >  
> > +    /*
> > +     * It's relevant to set both current and curr_vcpu back-to-back, to avoid a
> > +     * window where calls to mapcache_current_vcpu() during the context switch
> > +     * could trigger a recursive loop.
> > +     *
> > +     * Do the current switch immediately after switching to the new guest
> > +     * page-tables, so that current is (almost) always in sync with the
> > +     * currently loaded page-tables.
> > +     */
> > +    set_current(n);
> > +    per_cpu(curr_vcpu, cpu) = n;
> 
> The latter paragraph of the comment states something that so far wasn't intended,
> and imo also shouldn't be going forward. It's curr_vcpu which wants to be in sync
> with the loaded page tables. (Whether pulling ahead its updating is okay is a
> separate question. All of these actions used to be be very carefully placed they
> way they are. Which isn't to say that I can exclude things having gone stale ...)

I've noticed this was all quite carefully placed.  I've also attempted
to take care with the changes I've done here (and tested them
extensively).

> And yes, that has always meant that mapcache_current_vcpu()'s condition for
> calling sync_local_execstate() was building upon the fact that it won't be called
> from context switching contexts.
> 
> Did you consider updating that condition (evaluating curr_cpu) instead?

We cannot safely use map_domain_page() if current != curr_vcpu,
because at any point (as a result of an interrupt) a call to
sync_local_execstate(), and thus remove the mappings created by
map_domain_page() as a result of performing a full context switch to
the idle vCPU (and the idle vCPU page tables).

> 
> > @@ -2048,8 +2060,6 @@ static void __context_switch(void)
> >      if ( pd != nd )
> >          cpumask_clear_cpu(cpu, pd->dirty_cpumask);
> >      write_atomic(&p->dirty_cpu, VCPU_CPU_CLEAN);
> > -
> > -    per_cpu(curr_vcpu, cpu) = n;
> >  }
> >  
> >  void context_switch(struct vcpu *prev, struct vcpu *next)
> > @@ -2081,16 +2091,36 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
> >  
> >      local_irq_disable();
> >  
> > -    set_current(next);
> > -
> >      if ( (per_cpu(curr_vcpu, cpu) == next) ||
> >           (is_idle_domain(nextd) && cpu_online(cpu)) )
> >      {
> > +        /*
> > +         * Lazy context switch to the idle vCPU, set current == idle.  Full
> > +         * context switch happens if/when sync_local_execstate() is called.
> > +         */
> > +        set_current(next);
> >          local_irq_enable();
> 
> The comment is misleading as far as the first half of the if() condition goes:
> No further switching is going to happen in that case, aiui.

Right, I should clarify that comment: this is either a lazy context
switch, or the return from a lazy state to the previously running
vCPU.

> >      }
> >      else
> >      {
> > -        __context_switch();
> > +        /*
> > +         * curr_vcpu will always point to the currently loaded vCPU context, as
> > +         * it's not updated when doing a lazy switch to the idle vCPU.
> > +         */
> > +        struct vcpu *prev_ctx = per_cpu(curr_vcpu, cpu);
> > +
> > +        if ( prev_ctx != current )
> > +        {
> > +            /*
> > +             * Doing a full context switch to a non-idle vCPU from a lazy
> > +             * context switched state.  Adjust current to point to the
> > +             * currently loaded vCPU context.
> > +             */
> > +            ASSERT(current == idle_vcpu[cpu]);
> > +            ASSERT(!is_idle_vcpu(next));
> > +            set_current(prev_ctx);
> 
> This feels wrong, as in "current" then not representing what it should represent,
> for a certain time window. I may be dense, but neither comment not description
> clarify to me why this might be needed. I can see that it's needed to please the
> ASSERT() you add to __context_switch(), yet then I might ask why that assertion
> is put there.

This is done so that when calling __context_switch() current ==
curr_vcpu, and map_domain_page() can be used without getting into an
infinite sync_local_execstate() recursion loop.

> 
> > +        }
> > +        __context_switch(next);
> >  
> >          /* Re-enable interrupts before restoring state which may fault. */
> >          local_irq_enable();
> > @@ -2156,15 +2186,23 @@ int __sync_local_execstate(void)
> >  {
> >      unsigned long flags;
> >      int switch_required;
> > +    unsigned int cpu = smp_processor_id();
> > +    struct vcpu *p;
> >  
> >      local_irq_save(flags);
> >  
> > -    switch_required = (this_cpu(curr_vcpu) != current);
> > +    p = per_cpu(curr_vcpu, cpu);
> > +    switch_required = (p != current);
> >  
> >      if ( switch_required )
> >      {
> > -        ASSERT(current == idle_vcpu[smp_processor_id()]);
> > -        __context_switch();
> > +        ASSERT(current == idle_vcpu[cpu]);
> > +        /*
> > +         * Restore current to the previously running vCPU, __context_switch()
> > +         * will update current together with curr_vcpu.
> > +         */
> > +        set_current(p);
> 
> Similarly here.

Same reason, so that when calling __context_switch() current ==
curr_vcpu and map_domain_page() can be used (and in general
sync_local_execstate() becomes a no-op because a switch is already in
process.)

> 
> > +        __context_switch(idle_vcpu[cpu]);
> >      }
> >  
> >      local_irq_restore(flags);
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -2232,8 +2232,6 @@ void __init trap_init(void)
> >  
> >  void activate_debugregs(const struct vcpu *curr)
> >  {
> > -    ASSERT(curr == current);
> > -
> >      write_debugreg(0, curr->arch.dr[0]);
> >      write_debugreg(1, curr->arch.dr[1]);
> >      write_debugreg(2, curr->arch.dr[2]);
> 
> Why would this assertion go away? If it suddenly triggers, the parameter name
> would now end up being wrong.

Well, at the point where activate_debugregs() gets called (in
paravirt_ctxt_switch_to()), current == previous as a result of this
change, so the assert is no longer true on purpose on that call
path.

Thanks, Roger.