Re: [PATCH v1] xen: move getdomaininfo() to domain.c

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2025 22:30, Stefano Stabellini wrote:
> On Wed, 23 Jul 2025, Jan Beulich wrote:
>> On 23.07.2025 02:46, Stefano Stabellini wrote:
>>> On Tue, 22 Jul 2025, Jan Beulich wrote:
>>>> On 22.07.2025 07:04, Penny Zheng wrote:
>>>>> Function getdomaininfo() is not only invoked by domctl-op, but also 
>>>>> sysctl-op,
>>>>> so it shall better live in domain.c, rather than domctl.c. Which is also
>>>>> applied for arch_get_domain_info(). Style corrections shall be applied at
>>>>> the same time while moving these functions, such as converting u64 to
>>>>> uint64_t.
>>>>>
>>>>> The movement could also fix CI error of a randconfig picking both SYSCTL=y
>>>>> and PV_SHIM_EXCLUSIVE=y results in sysctl.c being built, but domctl.c not
>>>>> being built, which leaves getdomaininfo() undefined, causing linking to 
>>>>> fail.
>>>>>
>>>>> Fixes: 34317c508294 ("xen/sysctl: wrap around sysctl hypercall")
>>>>> Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
>>>>
>>>> I'm not convinced of this approach. In the longer run this would mean 
>>>> wrapping
>>>> everything you move in "#if defined(CONFIG_SYSCTL) || 
>>>> defined(CONFIG_DOMCTL)",
>>>> which I consider undesirable. Without DOMCTL, the usefulness of
>>>> XEN_SYSCTL_getdomaininfolist is at least questionable. Therefore adding 
>>>> more
>>>> isolated "#ifdef CONFIG_DOMCTL" just there may be an option. Similarly, as
>>>> mentioned on the other thread, having SYSCTL depend on DOMCTL is an 
>>>> approach
>>>> which imo wants at least considering. And there surely are further options.
>>>>
>>>> As indicated elsewhere, my preference goes towards reverting the final one 
>>>> or
>>>> two patches of that series. They can be re-applied once the dependencies 
>>>> were
>>>> properly sorted, which may (as per above) involve properly introducing a
>>>> DOMCTL Kconfig setting first.
>>>
>>> I don't think this is a good idea.
>>
>> And implicitly you say that what I put under question in the first paragraph
>> is a good way forward?
> 
> I think it is OK.
> 
> I also think "having SYSCTL depend on DOMCTL" is certainly worth
> thinking about. In terms of privilege and potential for interference
> with other domains sysctl and domctl don't seem different so it is
> unlikely one would want to disable one but not the other.
> 
> Another idea is to have a single kconfig for both SYSCTL and DOMCTL: we
> don't necessarily need to offer individual kconfig for every feature.
> From a safety point of view, we want to disable them both.

Then again (and going against the thought of making SYSCTL depend on DOMCTL)
there may be a desire to query / alter certain properties of the system as
a whole, without also having that need for individual domains. But yes,
covering both with a single control also is an option to consider.

Jan