Re: [PATCH v12 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2025 19:17, Mykola Kvach wrote:
> On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
>> On Mon, Sep 1, 2025 at 5:29 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 31.08.2025 00:10, Mykola Kvach wrote:
>>>> --- a/xen/arch/ppc/stubs.c
>>>> +++ b/xen/arch/ppc/stubs.c
>>>> @@ -224,6 +224,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      BUG_ON("unimplemented");
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>>>  {
>>>>      BUG_ON("unimplemented");
>>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
>>>> index 1a8c86cd8d..52532ae14d 100644
>>>> --- a/xen/arch/riscv/stubs.c
>>>> +++ b/xen/arch/riscv/stubs.c
>>>> @@ -198,6 +198,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      BUG_ON("unimplemented");
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  int arch_set_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>>>  {
>>>>      BUG_ON("unimplemented");
>>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>>> index 19fd86ce88..94a06bc697 100644
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1138,6 +1138,11 @@ void arch_domain_creation_finished(struct domain *d)
>>>>          hvm_domain_creation_finished(d);
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  #ifdef CONFIG_COMPAT
>>>>  #define xen_vcpu_guest_context vcpu_guest_context
>>>>  #define fpu_ctxt fpu_ctxt.x
>>>
>>> I definitely don't like this redundancy, and even less so that you 
>>> introduce out-
>>> of-line calls.
>>
>> Thank you for your feedback.
>> I followed the existing pattern used in other architecture stubs.
> 
> ... while I understand your concern about redundancy and out-of-line
> calls, I would appreciate more specific technical reasoning for why
> this approach is undesirable.

Out of line functions, even if as simple as the example above, have a
code size and performance effect; effectively empty inline functions
can typically be eliminated altogether by the compiler, including the
checking of their "return" values. While the impact may be low, any
such instance can later be used as motivation / justification to
introduce further instances (much like you did in to your earlier
reply, still in context above). And the sum of them then may not be
"low impact" anymore.

Furthermore we're already moving towards wider use of has_include().

> Code review is most effective when it is based on objective criteria
> and project guidelines, rather than personal preferences.

And what did you derive from that my comment was purely based on a
personal preference? Plus even if it were (often I would indicate so),
that's imo still okay, as in many case maintainer preferences also
matter (e.g. if only for a more consistent overall code base).

> This helps contributors understand the rationale and make improvements
> that benefit the whole project.

While content-wise I agree, considering the amount of work I put into
doing reviews, I still view this sort of "education" as pretty close
to an offense. Plus did you consider how well it would scale if in
every review all sorts of extra justification would need giving? I
don't really like to put things this way, but I would really recommend
you first start doing perhaps dozens of reviews a week before judging
on whether any particular review gave you enough background info.

Jan