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

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

On Tue, Jan 14, 2025 at 04:02:01PM +0100, Jan Beulich wrote:
> On 09.01.2025 18:33, Roger Pau Monné wrote:
> > On Thu, Jan 09, 2025 at 09:59:58AM +0100, Jan Beulich wrote:
> >> On 08.01.2025 15:26, Roger Pau Monne wrote:
> >>>      }
> >>>      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.
> 
> Yet it's the purpose of __context_switch() to bring curr_vcpu in sync
> with current. IOW both matching up is supposed to be an exit condition
> of the function, not an entry one.
> 
> Plus, as indicated when we were talking this through yesterday, the
> set_current() here make "current" no longer point at what - from the
> scheduler's perspective - is (supposed to be) the current vCPU.

I understand this, and I will look into alternative ways to workaround
the issues I'm facing that prompted the changes proposed on this
patch.

I've been thinking about what we spoke of disabling lazy idle context
switch when ASI was enabled, and I'm afraid that won't be enough.  The
{populate,destroy}_perdomain_mapping() functions added later in the
series will be used in the context switch path regardless of whether
ASI is enabled, and those functions require map_domain_page() to be
usable.  Hence map_domain_page() needs to be usable in the context
switch path.

I will see whether I can allow the usage of map_domain_page() at
context switch in a different way.

I understand the main concern is the window where current and the
scheduler notion of current diverge right?

Arguably this is already happening in context_switch(), as
set_current() gets called almost at the beggining of the function,
while the call to sched_context_switched() only happens at the tail of
the function.  So for the whole call to  __context_switch() current is
not in-sync with the scheduler currently running vCPU.  And I'm not
saying this is a model to follow, but the context switch code is
already fairly special, hence I don't see the change here as that much
different from the current logic.

That said, I will still try to figure an alternative way to deal with
the usage of map_domain_page() in the context switch path.

> Aiui this adjustment is the reason for ...
> 
> >>> --- 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.
> 
> ... this behavior. Which, as said, feels wrong the latest when "curr" was
> renamed to no longer suggest it actually is cached "current". At that point
> it'll be dubious whose ->arch.dr[] are actually written into the CPU
> registers.
> 
> Also let's not forget that there's a 2nd call here, where I very much hope
> it continues to be "current" that's being passed in.

Indeed, for the other call the assert would still be valid, that
context is not changed.

Thanks, Roger.