Re: [PATCH v7 12/12] xen/arm: Add support for system suspend triggered by control domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2026 11:19, Mykola Kvach wrote:
> On Mon, Dec 15, 2025 at 1:49 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 11.12.2025 19:43, Mykola Kvach wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -137,6 +137,11 @@ config HAS_EX_TABLE
>>>  config HAS_FAST_MULTIPLY
>>>       bool
>>>
>>> +config HAS_HWDOM_SHUTDOWN_ON_SUSPEND
>>> +     bool
>>> +     default y
>>> +     depends on !ARM_64 || !SYSTEM_SUSPEND
>>
>> As written, this would want to be "def_bool y". However, I think I indicated
> 
> OK, I’ll switch this to def_bool.
> 
>> previously that imo it would be nice if we could stop adding more "depends 
>> on"
>> referencing particular architectures. Instead "select" or "imply" from
>> xen/arch/<arch>/Kconfig appears more desirable to use in such cases. That way
>> each arch can control what it wants without needing to touch common code.
>>
>> As an aside, in the context of PV_SHIM_EXCLUSIVE it was also said several
>> times that negative dependencies aren't very nice to have. Here we have no
>> prompt, so the "allyesconfig" concern doesn't apply, but I still thought I'd
>> mention this.
> 
> I used the common-level dependency only to avoid adding selects in every
> other arch Kconfig, as the only exception I need is
>     ARM_64 && SYSTEM_SUSPEND.
> 
> If you still prefer keeping all arch-specific handling under
> xen/arch/<arch>/Kconfig, I can rework it accordingly.

Imo there are two options: Do as you suggest, but with an option not starting
HAS_*. Or use HAS_ with per-arch selects (which I think I'd prefer).

To limit the number of selects needed, perhaps the sense of the option may
want inverting? Otoh I don't think we know yet what RISC-V and PPC are going
to want?

>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1324,6 +1324,11 @@ void __domain_crash(struct domain *d)
>>>      domain_shutdown(d, SHUTDOWN_crash);
>>>  }
>>>
>>> +static inline bool need_hwdom_shutdown(uint8_t reason)
>>
>> Personally I think "want" would better express things, but I don't really
>> mind "need".
> 
> I'll change it to "want".
> 
>>
>>> +{
>>> +    return IS_ENABLED(CONFIG_HAS_HWDOM_SHUTDOWN_ON_SUSPEND) ||
>>> +           reason != SHUTDOWN_suspend;
>>> +}
>>
>> Seeing this in use, I wonder if HAS_ is really suitable here.
> 
> What name would you consider more suitable here?

As per above, HAS_ dropped would be an option. Yet that goes against my
preference above. Maybe HAS_ really is okay-ish here, despite what I said
earlier.

Jan