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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 24 Jul 2025, Jan Beulich wrote:
> 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.

If making SYSCTL depend on DOMCTL and/or a single kconfig for both
SYSCTL and DOMCTL are both way forward, then we can take this patch as
is?