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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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.

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.

Jan