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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, July 25, 2025 1:38 PM
> To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Penny, Zheng <penny.zheng@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <bertrand.marquis@xxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>; Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1] xen: move getdomaininfo() to domain.c
>
> On 25.07.2025 03:21, Stefano Stabellini wrote:
> > 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?
>
> In both of the named cases this patch simply wouldn't be needed. Once the
> conversion work was done, that is. And to be frank, I'm not happy to see the
> function move out and then back in.
>

I think the new patch "move domctl.o out of PV_SHIM_EXCLUSIVE " could also fix 
getdomaininfo() linking error

> Jan