xen/arch/arm/domain.c | 9 ++++++ xen/arch/arm/domctl.c | 9 ------ xen/arch/x86/domain.c | 10 +++++++ xen/arch/x86/domctl.c | 10 ------- xen/common/domain.c | 64 +++++++++++++++++++++++++++++++++++++++++++ xen/common/domctl.c | 64 ------------------------------------------- 6 files changed, 83 insertions(+), 83 deletions(-)
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@suse.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
xen/arch/arm/domain.c | 9 ++++++
xen/arch/arm/domctl.c | 9 ------
xen/arch/x86/domain.c | 10 +++++++
xen/arch/x86/domctl.c | 10 -------
xen/common/domain.c | 64 +++++++++++++++++++++++++++++++++++++++++++
xen/common/domctl.c | 64 -------------------------------------------
6 files changed, 83 insertions(+), 83 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 79a144e61b..2c2ef639ee 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1206,6 +1206,15 @@ void vcpu_kick(struct vcpu *v)
}
}
+void arch_get_domain_info(const struct domain *d,
+ struct xen_domctl_getdomaininfo *info)
+{
+ /* All ARM domains use hardware assisted paging. */
+ info->flags |= XEN_DOMINF_hap;
+
+ info->gpaddr_bits = p2m_ipa_bits;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index ad914c915f..e0ea73005a 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -17,15 +17,6 @@
#include <xsm/xsm.h>
#include <public/domctl.h>
-void arch_get_domain_info(const struct domain *d,
- struct xen_domctl_getdomaininfo *info)
-{
- /* All ARM domains use hardware assisted paging. */
- info->flags |= XEN_DOMINF_hap;
-
- info->gpaddr_bits = p2m_ipa_bits;
-}
-
static int handle_vuart_init(struct domain *d,
struct xen_domctl_vuart_op *vuart_op)
{
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 56c3816187..4af9f41cca 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2639,6 +2639,16 @@ unsigned int domain_max_paddr_bits(const struct domain *d)
return bits;
}
+void arch_get_domain_info(const struct domain *d,
+ struct xen_domctl_getdomaininfo *info)
+{
+ if ( paging_mode_hap(d) )
+ info->flags |= XEN_DOMINF_hap;
+
+ info->arch_config.emulation_flags = d->arch.emulation_flags;
+ info->gpaddr_bits = hap_paddr_bits;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 3044f706de..35572767ba 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -138,16 +138,6 @@ static int vcpu_set_vmce(struct vcpu *v,
return vmce_restore_vcpu(v, &vmce);
}
-void arch_get_domain_info(const struct domain *d,
- struct xen_domctl_getdomaininfo *info)
-{
- if ( paging_mode_hap(d) )
- info->flags |= XEN_DOMINF_hap;
-
- info->arch_config.emulation_flags = d->arch.emulation_flags;
- info->gpaddr_bits = hap_paddr_bits;
-}
-
static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 303c338ef2..b74d4c7549 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -2440,6 +2440,70 @@ void thaw_domains(void)
#endif /* CONFIG_SYSTEM_SUSPEND */
+void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
+{
+ struct vcpu *v;
+ uint64_t cpu_time = 0;
+ int flags = XEN_DOMINF_blocked;
+ struct vcpu_runstate_info runstate;
+
+ memset(info, 0, sizeof(*info));
+
+ info->domain = d->domain_id;
+ info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
+
+ /*
+ * - domain is marked as blocked only if all its vcpus are blocked
+ * - domain is marked as running if any of its vcpus is running
+ */
+ for_each_vcpu ( d, v )
+ {
+ vcpu_runstate_get(v, &runstate);
+ cpu_time += runstate.time[RUNSTATE_running];
+ info->max_vcpu_id = v->vcpu_id;
+ if ( !(v->pause_flags & VPF_down) )
+ {
+ if ( !(v->pause_flags & VPF_blocked) )
+ flags &= ~XEN_DOMINF_blocked;
+ if ( v->is_running )
+ flags |= XEN_DOMINF_running;
+ info->nr_online_vcpus++;
+ }
+ }
+
+ info->cpu_time = cpu_time;
+
+ info->flags = (info->nr_online_vcpus ? flags : 0) |
+ ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying : 0) |
+ (d->is_shut_down ? XEN_DOMINF_shutdown : 0) |
+ (d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) |
+ (d->debugger_attached ? XEN_DOMINF_debugged : 0) |
+ (is_xenstore_domain(d) ? XEN_DOMINF_xs_domain : 0) |
+ (is_hvm_domain(d) ? XEN_DOMINF_hvm_guest : 0) |
+ d->shutdown_code << XEN_DOMINF_shutdownshift;
+
+ xsm_security_domaininfo(d, info);
+
+ info->tot_pages = domain_tot_pages(d);
+ info->max_pages = d->max_pages;
+ info->outstanding_pages = d->outstanding_pages;
+#ifdef CONFIG_MEM_SHARING
+ info->shr_pages = atomic_read(&d->shr_pages);
+#endif
+#ifdef CONFIG_MEM_PAGING
+ info->paged_pages = atomic_read(&d->paged_pages);
+#endif
+ info->shared_info_frame =
+ gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
+ BUG_ON(SHARED_M2P(info->shared_info_frame));
+
+ info->cpupool = cpupool_get_id(d);
+
+ memcpy(info->handle, d->handle, sizeof(xen_domain_handle_t));
+
+ arch_get_domain_info(d, info);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f2a7caaf85..99de77380f 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -65,70 +65,6 @@ static inline int is_free_domid(domid_t dom)
return 0;
}
-void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
-{
- struct vcpu *v;
- u64 cpu_time = 0;
- int flags = XEN_DOMINF_blocked;
- struct vcpu_runstate_info runstate;
-
- memset(info, 0, sizeof(*info));
-
- info->domain = d->domain_id;
- info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
-
- /*
- * - domain is marked as blocked only if all its vcpus are blocked
- * - domain is marked as running if any of its vcpus is running
- */
- for_each_vcpu ( d, v )
- {
- vcpu_runstate_get(v, &runstate);
- cpu_time += runstate.time[RUNSTATE_running];
- info->max_vcpu_id = v->vcpu_id;
- if ( !(v->pause_flags & VPF_down) )
- {
- if ( !(v->pause_flags & VPF_blocked) )
- flags &= ~XEN_DOMINF_blocked;
- if ( v->is_running )
- flags |= XEN_DOMINF_running;
- info->nr_online_vcpus++;
- }
- }
-
- info->cpu_time = cpu_time;
-
- info->flags = (info->nr_online_vcpus ? flags : 0) |
- ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying : 0) |
- (d->is_shut_down ? XEN_DOMINF_shutdown : 0) |
- (d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) |
- (d->debugger_attached ? XEN_DOMINF_debugged : 0) |
- (is_xenstore_domain(d) ? XEN_DOMINF_xs_domain : 0) |
- (is_hvm_domain(d) ? XEN_DOMINF_hvm_guest : 0) |
- d->shutdown_code << XEN_DOMINF_shutdownshift;
-
- xsm_security_domaininfo(d, info);
-
- info->tot_pages = domain_tot_pages(d);
- info->max_pages = d->max_pages;
- info->outstanding_pages = d->outstanding_pages;
-#ifdef CONFIG_MEM_SHARING
- info->shr_pages = atomic_read(&d->shr_pages);
-#endif
-#ifdef CONFIG_MEM_PAGING
- info->paged_pages = atomic_read(&d->paged_pages);
-#endif
- info->shared_info_frame =
- gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
- BUG_ON(SHARED_M2P(info->shared_info_frame));
-
- info->cpupool = cpupool_get_id(d);
-
- memcpy(info->handle, d->handle, sizeof(xen_domain_handle_t));
-
- arch_get_domain_info(d, info);
-}
-
bool domctl_lock_acquire(void)
{
/*
--
2.34.1On 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@suse.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
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.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, July 22, 2025 3:16 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <bertrand.marquis@arm.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Roger Pau Monné <roger.pau@citrix.com>; Daniel P. Smith
> <dpsmith@apertussolutions.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1] xen: move getdomaininfo() to domain.c
>
> 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@suse.com>
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> 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.
>
Yes, later when introducing CONFIG_DOMCTL. I intend to wrap getdomaininfo() with "#if defined(CONFIG_SYSCTL) || defined(CONFIG_DOMCTL)". imo, getdomaininfo() is to achieve domain info, domctl-op use it to get per-domain info and sysctl-op use it to get system-wide, all-domain info, That's why I suggest to move it to more common location domain.c.
Apart from it, we will have functions like, cpumask_to_xenctl_bitmap(), etc, generic type-conversion helper, needs such wrapping. That's all the scenario fwis.
> 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.
>
> Jan
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@suse.com>
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> 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. The patch that Penny wrote addresses
this specific issue effectively and allows us to move forward. It also
helps us continue identifying problems in GitLab CI and prevents
regressions with CONFIG_SYSCTL.
I am also aware that Penny has already more patches to send, including
the DOMCTL Kconfig you are asking.
I think this patch is fine as is:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
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@suse.com>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>
>> 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?
Jan
> The patch that Penny wrote addresses
> this specific issue effectively and allows us to move forward. It also
> helps us continue identifying problems in GitLab CI and prevents
> regressions with CONFIG_SYSCTL.
>
> I am also aware that Penny has already more patches to send, including
> the DOMCTL Kconfig you are asking.
>
> I think this patch is fine as is:
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
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@suse.com>
> >>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> >>
> >> 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.
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@suse.com>
>>>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>>>
>>>> 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
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@suse.com>
> >>>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> >>>>
> >>>> 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?
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@suse.com>
>>>>>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>>>>>
>>>>>> 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.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, July 25, 2025 1:38 PM
> To: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Penny, Zheng <penny.zheng@amd.com>; Huang, Ray
> <Ray.Huang@amd.com>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <bertrand.marquis@arm.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Roger Pau Monné <roger.pau@citrix.com>; Daniel P. Smith
> <dpsmith@apertussolutions.com>; xen-devel@lists.xenproject.org
> 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@suse.com>
> >>>>>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> >>>>>>
> >>>>>> 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
© 2016 - 2025 Red Hat, Inc.