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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2025 08:29, Mykola Kvach wrote:
> On Mon, Sep 1, 2025 at 8:02 PM Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
>>
>> Hi Jan,
>>
>> 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.
>>
>>>
>>>> --- a/xen/include/xen/domain.h
>>>> +++ b/xen/include/xen/domain.h
>>>> @@ -109,6 +109,8 @@ int arch_domain_soft_reset(struct domain *d);
>>>>
>>>>  void arch_domain_creation_finished(struct domain *d);
>>>>
>>>> +int arch_domain_resume(struct domain *d);
>>>
>>> I think this wants to move to a per-arch header, presence of which is 
>>> checked by
>>> has_include(), with an inline fallback define once centrally here.
>>
>> Would it be acceptable to use a weak function as the default
>> implementation instead? This way, architectures needing a real
>> implementation could override it, while others would use the weak
>> default.

Besides this not addressing my out-of-line concern, we avoided the use
of weak symbols so far. While I don't recall specific details, iirc
this was somewhat related to Linux at some point deciding to reduce
(eventually eliminate?) the use of weak symbols.

> AFAIU, both your proposal and mine would violate MISRA C Dir 1.1 and
> Rule 1.1 (also Rule 1.2 but it is acceptable). According to these
> requirements, any use of compiler extensions should be documented and
> understood. In the context of the Xen hypervisor, such extensions must
> be listed in "docs/misra/C-language-toolchain.rst" as required by our
> project guidelines.

Just to mention that we use has_include() already, and that there are
two uses of __weak in livepatch code (which I would prefer not to use as
justification that further use of weak symbols is okay, as they're in
macros used in livepatches only, not in core Xen).

Jan