Re: [PATCH v3 2/8] keyhandler: drop regs parameter from handle_keyregs()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2024 23:09, Julien Grall wrote:
> On 05/02/2024 13:28, Jan Beulich wrote:
>> In preparation for further removal of regs parameters, drop it here. In
>> the two places where it's actually needed, retrieve IRQ context if
>> available, or else guest context.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> As an alternative to the new boolean parameter, I wonder if we couldn't
>> special-case the idle vCPU case: It's only there where we would not have
>> proper context retrievable via guest_cpu_user_regs().
> 
> I am trying to understand the implication. Looking at the code, it seems 
> in the case where we pass NULL, we would expect to call 
> run_in_exception_handler().

Right, when NULL was passed so far, and when true is passed now, that's
to indicate to invoke run_in_exception_handler().

> If I am not mistaken, at least for Arm, regs would not be the same as 
> guest_cpu_user_regs(). So I think your current approach is more correct.
> 
> Did I miss anything?

Whether regs are the same isn't overly relevant here. The thing that's
relevant is whether what would be logged actually makes sense. And
invoking guest_cpu_user_regs() in idle vCPU context makes no sense.
Whereas in other contexts its result is good enough to show the present
state of the CPU; there's no real need in such a case to go through
run_in_exception_handler().

The present approach therefore isn't necessarily "more correct", but it
is closer to prior behavior.

The corner case that makes me prefer the presently chosen approach is
when a CPU is in the middle of scheduling, especially considering x86's
lazy switching (when current != this_cpu(curr_vcpu). The main reason to
mention the alternative is because iirc Andrew had suggested to move
more in that direction.

Jan