diff mbox series

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

Message ID 20250108142659.99490-3-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86: adventures in Address Space Isolation | expand

Commit Message

Roger Pau Monné Jan. 8, 2025, 2:26 p.m. UTC
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:

map_domain_page() -> sync_local_execstate() -> map_domain_page() -> ...

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/domain.c | 58 +++++++++++++++++++++++++++++++++++--------
 xen/arch/x86/traps.c  |  2 --
 2 files changed, 48 insertions(+), 12 deletions(-)

Comments

Alejandro Vallejo Jan. 8, 2025, 4:26 p.m. UTC | #1
This is a net gain even without ASI. Having "current" hold the previous vCPU on
__context_switch() makes it _a lot_ easier to follow the lazy switch path.

On Wed Jan 8, 2025 at 2:26 PM GMT, 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:
>
> map_domain_page() -> sync_local_execstate() -> map_domain_page() -> ...

More generally, it's worth mentioning that we want to establish an invariant
between a per-cpu variable (curr_vcpu) and the currently running page tables.
That way it can be used as discriminant to know which are the currently active
per-vCPU mappings.

That's essential for implementing FPU hiding as proposed here:

  https://lore.kernel.org/xen-devel/20241105143310.28301-1-alejandro.vallejo@cloud.com/

A shorter form of that should probably be mentioned also...

>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - New in this version.
> ---
>  xen/arch/x86/domain.c | 58 +++++++++++++++++++++++++++++++++++--------
>  xen/arch/x86/traps.c  |  2 --
>  2 files changed, 48 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 78a13e6812c9..1f680bf176ee 100644
> --- 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;

... here. So we're not tempted to move these 2 far off from write_ptbase().

> +
>  #ifdef CONFIG_PV
>      /* Prefetch the VMCB if we expect to use it later in the context switch */
>      if ( using_svm() && is_pv_64bit_domain(nd) && !is_idle_domain(nd) )
> @@ -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();
>      }
>      else
>      {
> -        __context_switch();
> +        /*
> +         * curr_vcpu will always point to the currently loaded vCPU context, as

nit: s/will always point/always points/ ? It's an inconditional invariant,
after all.

> +         * 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);
> +        }
> +        __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);
> +        __context_switch(idle_vcpu[cpu]);
>      }
>  
>      local_irq_restore(flags);
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 87b30ce4df2a..487b8c5a78c5 100644
> --- 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]);

Cheers,
Alejandro
Jan Beulich Jan. 9, 2025, 8:59 a.m. UTC | #2
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.

> --- 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 ...)
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?

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

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

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

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

Jan
Roger Pau Monné Jan. 9, 2025, 5:33 p.m. UTC | #3
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.
Roger Pau Monné Jan. 9, 2025, 5:39 p.m. UTC | #4
On Wed, Jan 08, 2025 at 04:26:46PM +0000, Alejandro Vallejo wrote:
> This is a net gain even without ASI. Having "current" hold the previous vCPU on
> __context_switch() makes it _a lot_ easier to follow the lazy switch path.
> 
> On Wed Jan 8, 2025 at 2:26 PM GMT, 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:
> >
> > map_domain_page() -> sync_local_execstate() -> map_domain_page() -> ...
> 
> More generally, it's worth mentioning that we want to establish an invariant
> between a per-cpu variable (curr_vcpu) and the currently running page tables.
> That way it can be used as discriminant to know which are the currently active
> per-vCPU mappings.

You kind of already do this by checking curr_vcpu, as with this
changes there's still a window where the vCPU is lazy context
switched, and hence current != curr_vcpu (and curr_vcpu should signal
what page-tables are loaded).

The main point apart from more accurate signaling of the loaded
page-tables is to avoid infinite recursion if sync_local_execstate()
is called inside the context switch path.

> That's essential for implementing FPU hiding as proposed here:
> 
>   https://lore.kernel.org/xen-devel/20241105143310.28301-1-alejandro.vallejo@cloud.com/
> 
> A shorter form of that should probably be mentioned also...
> 
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - New in this version.
> > ---
> >  xen/arch/x86/domain.c | 58 +++++++++++++++++++++++++++++++++++--------
> >  xen/arch/x86/traps.c  |  2 --
> >  2 files changed, 48 insertions(+), 12 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index 78a13e6812c9..1f680bf176ee 100644
> > --- 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;
> 
> ... here. So we're not tempted to move these 2 far off from write_ptbase().

I think the "Do the current switch immediately after switching to the
new guest page-tables" sentence already signals that it's important to
keep the setting of current and curr_vcpu as close to the
write_ptbase() call as possible, but I'm open to suggestions for
better wording.

> > +
> >  #ifdef CONFIG_PV
> >      /* Prefetch the VMCB if we expect to use it later in the context switch */
> >      if ( using_svm() && is_pv_64bit_domain(nd) && !is_idle_domain(nd) )
> > @@ -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();
> >      }
> >      else
> >      {
> > -        __context_switch();
> > +        /*
> > +         * curr_vcpu will always point to the currently loaded vCPU context, as
> 
> nit: s/will always point/always points/ ? It's an inconditional invariant,
> after all.

Sure.

Thanks, Roger.
diff mbox series

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 78a13e6812c9..1f680bf176ee 100644
--- 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;
+
 #ifdef CONFIG_PV
     /* Prefetch the VMCB if we expect to use it later in the context switch */
     if ( using_svm() && is_pv_64bit_domain(nd) && !is_idle_domain(nd) )
@@ -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();
     }
     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);
+        }
+        __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);
+        __context_switch(idle_vcpu[cpu]);
     }
 
     local_irq_restore(flags);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 87b30ce4df2a..487b8c5a78c5 100644
--- 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]);