[PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Penny Zheng]

In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
a few functions, like domctl_lock_acquire/release() undefined, causing linking
to fail.
To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
/hypercall-defs section, with this adjustment, we also need to release
redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
to not break compilation
Above change will leave dead code in the shim binary temporarily and will be
fixed with the introduction of domctl-op wrapping.

Fixes: 568f806cba4c ("xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"")
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- remove paging_domctl hypercall-defs
---
v2 -> v3:
- remove paging_domctl change
---
 xen/arch/x86/Makefile        | 2 +-
 xen/common/Makefile          | 5 +----
 xen/include/hypercall-defs.c | 4 +---
 xen/include/xen/domain.h     | 4 ----
 4 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5aab30a0c4..7676d7cdd8 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -28,6 +28,7 @@ obj-y += delay.o
 obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
 obj-y += domain.o
+obj-y += domctl.o
 obj-bin-y += dom0_build.init.o
 obj-y += domain_page.o
 obj-y += e820.o
@@ -79,7 +80,6 @@ obj-y += vm_event.o
 obj-y += xstate.o
 
 ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
-obj-y += domctl.o
 obj-y += platform_hypercall.o
 obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
 endif
diff --git a/xen/common/Makefile b/xen/common/Makefile
index c316957fcb..756ddf52c3 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
 obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
 obj-y += domain.o
+obj-y += domctl.o
 obj-y += event_2l.o
 obj-y += event_channel.o
 obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
@@ -69,10 +70,6 @@ obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo un
 
 obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
 
-ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
-obj-y += domctl.o
-endif
-
 extra-y := symbols-dummy.o
 
 obj-$(CONFIG_COVERAGE) += coverage/
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 8370b4b289..221dc25f6f 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -200,8 +200,8 @@ sysctl(xen_sysctl_t *u_sysctl)
 #if defined(CONFIG_X86) && defined(CONFIG_PAGING)
 paging_domctl_cont(xen_domctl_t *u_domctl)
 #endif
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
 domctl(xen_domctl_t *u_domctl)
+#ifndef CONFIG_PV_SHIM_EXCLUSIVE
 platform_op(xen_platform_op_t *u_xenpf_op)
 #endif
 #ifdef CONFIG_HVM
@@ -280,9 +280,7 @@ hvm_op                             do       do       do       do       do
 #ifdef CONFIG_SYSCTL
 sysctl                             do       do       do       do       do
 #endif
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
 domctl                             do       do       do       do       do
-#endif
 #ifdef CONFIG_KEXEC
 kexec_op                           compat   do       -        -        -
 #endif
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index e10baf2615..33dd90357c 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -182,11 +182,7 @@ struct vnuma_info {
     struct xen_vmemrange *vmemrange;
 };
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
 void vnuma_destroy(struct vnuma_info *vnuma);
-#else
-static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }
-#endif
 
 extern bool vmtrace_available;
 
-- 
2.34.1

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Fri, 15 Aug 2025, Penny Zheng wrote:
> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
> a few functions, like domctl_lock_acquire/release() undefined, causing linking
> to fail.
> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
> /hypercall-defs section, with this adjustment, we also need to release
> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
> to not break compilation
> Above change will leave dead code in the shim binary temporarily and will be
> fixed with the introduction of domctl-op wrapping.
> 
> Fixes: 568f806cba4c ("xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"")
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 15.08.2025 12:27, Penny Zheng wrote:
> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
> a few functions, like domctl_lock_acquire/release() undefined, causing linking
> to fail.
> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
> /hypercall-defs section, with this adjustment, we also need to release
> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
> to not break compilation
> Above change will leave dead code in the shim binary temporarily and will be
> fixed with the introduction of domctl-op wrapping.

Well, "temporarily" is now getting interesting. While v1 of "Introduce
CONFIG_DOMCTL" was submitted in time to still be eligible for taking into
4.21, that - as indicated elsewhere - is moving us further in an unwanted
direction. Hence I'm not sure this can even be counted as an in-time
submission. Plus it looks to be pretty extensive re-work in some areas.
Hence I'm somewhat weary as to 4.21 here. IOW question, mainly to Oleksii,
is whether to
1) strive to complete that work in time (and hence take the patch here),
2) take the patch here, accepting the size regression for the shim, or
3) revert what has caused the randconfig issues, and retry the effort in
   4.22.

> Fixes: 568f806cba4c ("xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"")
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

My earlier question (when the patch still was part of a series) sadly has
remained unanswered: You've run this through a full round of testing this
time?

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]


On 8/18/25 10:31 AM, Jan Beulich wrote:
> On 15.08.2025 12:27, Penny Zheng wrote:
>> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
>> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
>> a few functions, like domctl_lock_acquire/release() undefined, causing linking
>> to fail.
>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
>> /hypercall-defs section, with this adjustment, we also need to release
>> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
>> to not break compilation
>> Above change will leave dead code in the shim binary temporarily and will be
>> fixed with the introduction of domctl-op wrapping.
> Well, "temporarily" is now getting interesting. While v1 of "Introduce
> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into
> 4.21, that - as indicated elsewhere - is moving us further in an unwanted
> direction.

Do you mean that specifically this patch or the whole patch series is moving us
in unwanted direction? (1)


>   Hence I'm not sure this can even be counted as an in-time
> submission. Plus it looks to be pretty extensive re-work in some areas.

It doesn't clear based on change log why this patch is sent outside "Introduce
CONFIG_DOMCTL" (2) as it looks the same as in (2) and it was reverted once with
the reason "for breaking the x86 build". (I haven't checked what was changed so
it won't lead to build issue again.)

> Hence I'm somewhat weary as to 4.21 here. IOW question, mainly to Oleksii,
> is whether to
> 1) strive to complete that work in time (and hence take the patch here),
> 2) take the patch here, accepting the size regression for the shim, or
> 3) revert what has caused the randconfig issues, and retry the effort in
>     4.22.
>
In the current context, I think I prefer option 3.
However, if option 1 is possible, it could also be considered—except in the
case where the answer to (1) is that the whole patch series is moving us in
an unwanted direction. In that situation, IMO, only option 3 remains viable.

>> Fixes: 568f806cba4c ("xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"")
>> Reported-by: Jan Beulich<jbeulich@suse.com>
>> Signed-off-by: Penny Zheng<Penny.Zheng@amd.com>
> My earlier question (when the patch still was part of a series) sadly has
> remained unanswered: You've run this through a full round of testing this
> time?
>
> Jan

[-- Attachment #2: Type: text/html, Size: 3666 bytes --]

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 18.08.2025 15:28, Oleksii Kurochko wrote:
> On 8/18/25 10:31 AM, Jan Beulich wrote:
>> On 15.08.2025 12:27, Penny Zheng wrote:
>>> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
>>> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
>>> a few functions, like domctl_lock_acquire/release() undefined, causing linking
>>> to fail.
>>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
>>> /hypercall-defs section, with this adjustment, we also need to release
>>> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
>>> to not break compilation
>>> Above change will leave dead code in the shim binary temporarily and will be
>>> fixed with the introduction of domctl-op wrapping.
>> Well, "temporarily" is now getting interesting. While v1 of "Introduce
>> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into
>> 4.21, that - as indicated elsewhere - is moving us further in an unwanted
>> direction.
> 
> Do you mean that specifically this patch or the whole patch series is moving us
> in unwanted direction? (1)

That series. We said we don't want individual CONFIG_SYSCTL, CONFIG_DOMCTL, etc.
Instead a single umbrella option wants introducing. Which means there series
doesn't need re-doing from scratch, but it may end up being a significant re-
work, especially considering that CONFIG_SYSCTL is already in the codebase and
hence now also needs replacing.

>>   Hence I'm not sure this can even be counted as an in-time
>> submission. Plus it looks to be pretty extensive re-work in some areas.
> 
> It doesn't clear based on change log why this patch is sent outside "Introduce
> CONFIG_DOMCTL" (2) as it looks the same as in (2) and it was reverted once with
> the reason "for breaking the x86 build". (I haven't checked what was changed so
> it won't lead to build issue again.)

Before we can even consider further work in the intended direction, the present
randconfig build issue wants sorting. Which supposedly this patch alone does.

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Mon, 18 Aug 2025, Jan Beulich wrote:
> On 18.08.2025 15:28, Oleksii Kurochko wrote:
> > On 8/18/25 10:31 AM, Jan Beulich wrote:
> >> On 15.08.2025 12:27, Penny Zheng wrote:
> >>> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
> >>> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
> >>> a few functions, like domctl_lock_acquire/release() undefined, causing linking
> >>> to fail.
> >>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
> >>> /hypercall-defs section, with this adjustment, we also need to release
> >>> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
> >>> to not break compilation
> >>> Above change will leave dead code in the shim binary temporarily and will be
> >>> fixed with the introduction of domctl-op wrapping.
> >> Well, "temporarily" is now getting interesting. While v1 of "Introduce
> >> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into
> >> 4.21, that - as indicated elsewhere - is moving us further in an unwanted
> >> direction.
> > 
> > Do you mean that specifically this patch or the whole patch series is moving us
> > in unwanted direction? (1)
> 
> That series. We said we don't want individual CONFIG_SYSCTL, CONFIG_DOMCTL, etc.
> Instead a single umbrella option wants introducing. Which means there series
> doesn't need re-doing from scratch, but it may end up being a significant re-
> work, especially considering that CONFIG_SYSCTL is already in the codebase and
> hence now also needs replacing.

I would not characterize this series as "moving us in an unwanted
direction". Yes, it introduces a separate CONFIG_DOMCTL, which we
agreed we do not want. However, simplifying it to reuse a single
CONFIG is a minor improvement that can be addressed in v2. The main
challenge in this series is adding the #ifdef in the appropriate
places, and using a single CONFIG for domctl and sysctl would
actually help.


> >>   Hence I'm not sure this can even be counted as an in-time
> >> submission. Plus it looks to be pretty extensive re-work in some areas.
> > 
> > It doesn't clear based on change log why this patch is sent outside "Introduce
> > CONFIG_DOMCTL" (2) as it looks the same as in (2) and it was reverted once with
> > the reason "for breaking the x86 build". (I haven't checked what was changed so
> > it won't lead to build issue again.)
> 
> Before we can even consider further work in the intended direction, the present
> randconfig build issue wants sorting. Which supposedly this patch alone does.

I think we should take this patch right away.

I also think we should consider "Introduce CONFIG_DOMCTL" for 4.21.

RE: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Penny, Zheng]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, August 18, 2025 4:31 PM
> To: Penny, Zheng <penny.zheng@amd.com>; Oleksii Kurochko
> <oleksii.kurochko@gmail.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE
>
> On 15.08.2025 12:27, Penny Zheng wrote:
> > In order to fix CI error of a randconfig picking both
> > PV_SHIM_EXCLUSIVE=y and HVM=y results in hvm.c being built, but
> > domctl.c not being built, which leaves a few functions, like
> > domctl_lock_acquire/release() undefined, causing linking to fail.
> > To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE
> > Makefile /hypercall-defs section, with this adjustment, we also need
> > to release redundant vnuma_destroy() stub definition from
> > PV_SHIM_EXCLUSIVE guardian, to not break compilation Above change will
> > leave dead code in the shim binary temporarily and will be fixed with
> > the introduction of domctl-op wrapping.
>
> Well, "temporarily" is now getting interesting. While v1 of "Introduce
> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into 4.21,
> that - as indicated elsewhere - is moving us further in an unwanted direction. Hence
> I'm not sure this can even be counted as an in-time submission. Plus it looks to be
> pretty extensive re-work in some areas.
> Hence I'm somewhat weary as to 4.21 here. IOW question, mainly to Oleksii, is
> whether to
> 1) strive to complete that work in time (and hence take the patch here),
> 2) take the patch here, accepting the size regression for the shim, or
> 3) revert what has caused the randconfig issues, and retry the effort in
>    4.22.
>
> > Fixes: 568f806cba4c ("xen/x86: remove "depends on
> > !PV_SHIM_EXCLUSIVE"")
> > Reported-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> My earlier question (when the patch still was part of a series) sadly has remained
> unanswered: You've run this through a full round of testing this time?

Sorry, missed that, yes, it has been tested with both default defconfig and allyesconfig.

>
> Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 20.08.2025 05:12, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, August 18, 2025 4:31 PM
>> To: Penny, Zheng <penny.zheng@amd.com>; Oleksii Kurochko
>> <oleksii.kurochko@gmail.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
>> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
>> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE
>>
>> On 15.08.2025 12:27, Penny Zheng wrote:
>>> In order to fix CI error of a randconfig picking both
>>> PV_SHIM_EXCLUSIVE=y and HVM=y results in hvm.c being built, but
>>> domctl.c not being built, which leaves a few functions, like
>>> domctl_lock_acquire/release() undefined, causing linking to fail.
>>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE
>>> Makefile /hypercall-defs section, with this adjustment, we also need
>>> to release redundant vnuma_destroy() stub definition from
>>> PV_SHIM_EXCLUSIVE guardian, to not break compilation Above change will
>>> leave dead code in the shim binary temporarily and will be fixed with
>>> the introduction of domctl-op wrapping.
>>
>> Well, "temporarily" is now getting interesting. While v1 of "Introduce
>> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into 4.21,
>> that - as indicated elsewhere - is moving us further in an unwanted direction. Hence
>> I'm not sure this can even be counted as an in-time submission. Plus it looks to be
>> pretty extensive re-work in some areas.
>> Hence I'm somewhat weary as to 4.21 here. IOW question, mainly to Oleksii, is
>> whether to
>> 1) strive to complete that work in time (and hence take the patch here),
>> 2) take the patch here, accepting the size regression for the shim, or
>> 3) revert what has caused the randconfig issues, and retry the effort in
>>    4.22.
>>
>>> Fixes: 568f806cba4c ("xen/x86: remove "depends on
>>> !PV_SHIM_EXCLUSIVE"")
>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>
>> My earlier question (when the patch still was part of a series) sadly has remained
>> unanswered: You've run this through a full round of testing this time?
> 
> Sorry, missed that, yes, it has been tested with both default defconfig and allyesconfig.

I'm sorry if my request was unclear, but with "full round of testing" I in particular
meant a full CI pipeline, plus (given the issue that's being fixed) some extra
randconfig testing.

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

On Thu, 21 Aug 2025, Jan Beulich wrote:
> On 20.08.2025 05:12, Penny, Zheng wrote:
> > [Public]
> > 
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Monday, August 18, 2025 4:31 PM
> >> To: Penny, Zheng <penny.zheng@amd.com>; Oleksii Kurochko
> >> <oleksii.kurochko@gmail.com>
> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> >> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
> >> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> >> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> >> Subject: Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE
> >>
> >> On 15.08.2025 12:27, Penny Zheng wrote:
> >>> In order to fix CI error of a randconfig picking both
> >>> PV_SHIM_EXCLUSIVE=y and HVM=y results in hvm.c being built, but
> >>> domctl.c not being built, which leaves a few functions, like
> >>> domctl_lock_acquire/release() undefined, causing linking to fail.
> >>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE
> >>> Makefile /hypercall-defs section, with this adjustment, we also need
> >>> to release redundant vnuma_destroy() stub definition from
> >>> PV_SHIM_EXCLUSIVE guardian, to not break compilation Above change will
> >>> leave dead code in the shim binary temporarily and will be fixed with
> >>> the introduction of domctl-op wrapping.
> >>
> >> Well, "temporarily" is now getting interesting. While v1 of "Introduce
> >> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into 4.21,
> >> that - as indicated elsewhere - is moving us further in an unwanted direction. Hence
> >> I'm not sure this can even be counted as an in-time submission. Plus it looks to be
> >> pretty extensive re-work in some areas.
> >> Hence I'm somewhat weary as to 4.21 here. IOW question, mainly to Oleksii, is
> >> whether to
> >> 1) strive to complete that work in time (and hence take the patch here),
> >> 2) take the patch here, accepting the size regression for the shim, or
> >> 3) revert what has caused the randconfig issues, and retry the effort in
> >>    4.22.
> >>
> >>> Fixes: 568f806cba4c ("xen/x86: remove "depends on
> >>> !PV_SHIM_EXCLUSIVE"")
> >>> Reported-by: Jan Beulich <jbeulich@suse.com>
> >>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> >>
> >> My earlier question (when the patch still was part of a series) sadly has remained
> >> unanswered: You've run this through a full round of testing this time?
> > 
> > Sorry, missed that, yes, it has been tested with both default defconfig and allyesconfig.
> 
> I'm sorry if my request was unclear, but with "full round of testing" I in particular
> meant a full CI pipeline, plus (given the issue that's being fixed) some extra
> randconfig testing.

https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/1997431361

I ran a few tests myself changing config options on purpose trying to
break it, and so far they were all successful.

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 22.08.2025 02:10, Stefano Stabellini wrote:
> On Thu, 21 Aug 2025, Jan Beulich wrote:
>> On 20.08.2025 05:12, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Monday, August 18, 2025 4:31 PM
>>>> To: Penny, Zheng <penny.zheng@amd.com>; Oleksii Kurochko
>>>> <oleksii.kurochko@gmail.com>
>>>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
>>>> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
>>>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
>>>> Subject: Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE
>>>>
>>>> On 15.08.2025 12:27, Penny Zheng wrote:
>>>>> In order to fix CI error of a randconfig picking both
>>>>> PV_SHIM_EXCLUSIVE=y and HVM=y results in hvm.c being built, but
>>>>> domctl.c not being built, which leaves a few functions, like
>>>>> domctl_lock_acquire/release() undefined, causing linking to fail.
>>>>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE
>>>>> Makefile /hypercall-defs section, with this adjustment, we also need
>>>>> to release redundant vnuma_destroy() stub definition from
>>>>> PV_SHIM_EXCLUSIVE guardian, to not break compilation Above change will
>>>>> leave dead code in the shim binary temporarily and will be fixed with
>>>>> the introduction of domctl-op wrapping.
>>>>
>>>> Well, "temporarily" is now getting interesting. While v1 of "Introduce
>>>> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into 4.21,
>>>> that - as indicated elsewhere - is moving us further in an unwanted direction. Hence
>>>> I'm not sure this can even be counted as an in-time submission. Plus it looks to be
>>>> pretty extensive re-work in some areas.
>>>> Hence I'm somewhat weary as to 4.21 here. IOW question, mainly to Oleksii, is
>>>> whether to
>>>> 1) strive to complete that work in time (and hence take the patch here),
>>>> 2) take the patch here, accepting the size regression for the shim, or
>>>> 3) revert what has caused the randconfig issues, and retry the effort in
>>>>    4.22.
>>>>
>>>>> Fixes: 568f806cba4c ("xen/x86: remove "depends on
>>>>> !PV_SHIM_EXCLUSIVE"")
>>>>> Reported-by: Jan Beulich <jbeulich@suse.com>
>>>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>>>
>>>> My earlier question (when the patch still was part of a series) sadly has remained
>>>> unanswered: You've run this through a full round of testing this time?
>>>
>>> Sorry, missed that, yes, it has been tested with both default defconfig and allyesconfig.
>>
>> I'm sorry if my request was unclear, but with "full round of testing" I in particular
>> meant a full CI pipeline, plus (given the issue that's being fixed) some extra
>> randconfig testing.
> 
> https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/1997431361
> 
> I ran a few tests myself changing config options on purpose trying to
> break it, and so far they were all successful.

Should I translate this to Tested-by: then?

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Fri, 22 Aug 2025, Jan Beulich wrote:
> >> I'm sorry if my request was unclear, but with "full round of testing" I in particular
> >> meant a full CI pipeline, plus (given the issue that's being fixed) some extra
> >> randconfig testing.
> > 
> > https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/1997431361
> > 
> > I ran a few tests myself changing config options on purpose trying to
> > break it, and so far they were all successful.
> 
> Should I translate this to Tested-by: then?

Yes

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 19.08.2025 01:51, Stefano Stabellini wrote:
> On Mon, 18 Aug 2025, Jan Beulich wrote:
>> On 18.08.2025 15:28, Oleksii Kurochko wrote:
>>> On 8/18/25 10:31 AM, Jan Beulich wrote:
>>>> On 15.08.2025 12:27, Penny Zheng wrote:
>>>>> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
>>>>> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
>>>>> a few functions, like domctl_lock_acquire/release() undefined, causing linking
>>>>> to fail.
>>>>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
>>>>> /hypercall-defs section, with this adjustment, we also need to release
>>>>> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
>>>>> to not break compilation
>>>>> Above change will leave dead code in the shim binary temporarily and will be
>>>>> fixed with the introduction of domctl-op wrapping.
>>>> Well, "temporarily" is now getting interesting. While v1 of "Introduce
>>>> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into
>>>> 4.21, that - as indicated elsewhere - is moving us further in an unwanted
>>>> direction.
>>>
>>> Do you mean that specifically this patch or the whole patch series is moving us
>>> in unwanted direction? (1)
>>
>> That series. We said we don't want individual CONFIG_SYSCTL, CONFIG_DOMCTL, etc.
>> Instead a single umbrella option wants introducing. Which means there series
>> doesn't need re-doing from scratch, but it may end up being a significant re-
>> work, especially considering that CONFIG_SYSCTL is already in the codebase and
>> hence now also needs replacing.
> 
> I would not characterize this series as "moving us in an unwanted
> direction". Yes, it introduces a separate CONFIG_DOMCTL, which we
> agreed we do not want. However, simplifying it to reuse a single
> CONFIG is a minor improvement that can be addressed in v2. The main
> challenge in this series is adding the #ifdef in the appropriate
> places, and using a single CONFIG for domctl and sysctl would
> actually help.

Well, when are we going to see a v2 then which does this? Of the three
options I mentioned in the earlier reply, Oleksii favored the revert
path, leaving open the get-everything-in one. For the latter, however,
we need to see relatively constant progress now, or else time will run
out. Whether to commit the patch here really depends on what route we
settle on for 4.21.

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Mon, 25 Aug 2025, Jan Beulich wrote:
> On 19.08.2025 01:51, Stefano Stabellini wrote:
> > On Mon, 18 Aug 2025, Jan Beulich wrote:
> >> On 18.08.2025 15:28, Oleksii Kurochko wrote:
> >>> On 8/18/25 10:31 AM, Jan Beulich wrote:
> >>>> On 15.08.2025 12:27, Penny Zheng wrote:
> >>>>> In order to fix CI error of a randconfig picking both PV_SHIM_EXCLUSIVE=y and
> >>>>> HVM=y results in hvm.c being built, but domctl.c not being built, which leaves
> >>>>> a few functions, like domctl_lock_acquire/release() undefined, causing linking
> >>>>> to fail.
> >>>>> To fix that, we intend to move domctl.o out of the PV_SHIM_EXCLUSIVE Makefile
> >>>>> /hypercall-defs section, with this adjustment, we also need to release
> >>>>> redundant vnuma_destroy() stub definition from PV_SHIM_EXCLUSIVE guardian,
> >>>>> to not break compilation
> >>>>> Above change will leave dead code in the shim binary temporarily and will be
> >>>>> fixed with the introduction of domctl-op wrapping.
> >>>> Well, "temporarily" is now getting interesting. While v1 of "Introduce
> >>>> CONFIG_DOMCTL" was submitted in time to still be eligible for taking into
> >>>> 4.21, that - as indicated elsewhere - is moving us further in an unwanted
> >>>> direction.
> >>>
> >>> Do you mean that specifically this patch or the whole patch series is moving us
> >>> in unwanted direction? (1)
> >>
> >> That series. We said we don't want individual CONFIG_SYSCTL, CONFIG_DOMCTL, etc.
> >> Instead a single umbrella option wants introducing. Which means there series
> >> doesn't need re-doing from scratch, but it may end up being a significant re-
> >> work, especially considering that CONFIG_SYSCTL is already in the codebase and
> >> hence now also needs replacing.
> > 
> > I would not characterize this series as "moving us in an unwanted
> > direction". Yes, it introduces a separate CONFIG_DOMCTL, which we
> > agreed we do not want. However, simplifying it to reuse a single
> > CONFIG is a minor improvement that can be addressed in v2. The main
> > challenge in this series is adding the #ifdef in the appropriate
> > places, and using a single CONFIG for domctl and sysctl would
> > actually help.
> 
> Well, when are we going to see a v2 then which does this? Of the three
> options I mentioned in the earlier reply, Oleksii favored the revert
> path, leaving open the get-everything-in one. For the latter, however,
> we need to see relatively constant progress now, or else time will run
> out. Whether to commit the patch here really depends on what route we
> settle on for 4.21.

While I share Jan's view that now is a good time to make progress on
"Introduce CONFIG_DOMCTL", and I also believe that this is the best
course of action, I would like to share a few thoughts on the other
alternatives.

My understanding is that the PV_SHIM_EXCLUSIVE option is no longer
widely used, and a small size increase would not compromise its
functionality and should be tolerable.

In general, we are slow at getting larger series reviewed and committed,
and choosing to revert rather than accept fixes tends to make us even
slower, which is undesirable.

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

Hi all,

So I ran a test and the appended change, which is based on [1] and
renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
build issue.

For 4.21, I suggest we go with two patches:
1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS

Jan, are you OK with this?

Cheers,

Stefano

[1] https://marc.info/?l=xen-devel&m=175421457323598

diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index e10baf2615..dedc73412f 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
 
 int arch_vcpu_reset(struct vcpu *v);
 
+#ifdef CONFIG_SYSCTL
 bool domctl_lock_acquire(void);
 void domctl_lock_release(void);
+#else
+static inline bool domctl_lock_acquire(void)
+{
+    return false;
+}
+
+static inline void domctl_lock_release(void) {}
+#endif /* CONFIG_DOMCTL */
 
 /*
  * Continue the current hypercall via func(data) on specified cpu.

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Tue, 26 Aug 2025, Stefano Stabellini wrote:
> Hi all,
> 
> So I ran a test and the appended change, which is based on [1] and
> renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
> build issue.
> 
> For 4.21, I suggest we go with two patches:
> 1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> 2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS
> 
> Jan, are you OK with this?

The other patch doesn't work in the regular case but this patch does. I
ran it through 60+ randconfigs and so far so good. The strategy is still
the same I suggested above for 4.21: global rename, plus one more patch
(this one).


diff --git a/xen/common/Makefile b/xen/common/Makefile
index c316957fcb..352f004a2c 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -50,7 +50,6 @@ obj-y += spinlock.o
 obj-$(CONFIG_STACK_PROTECTOR) += stack-protector.o
 obj-y += stop_machine.o
 obj-y += symbols.o
-obj-$(CONFIG_SYSCTL) += sysctl.o
 obj-y += tasklet.o
 obj-y += time.o
 obj-y += timer.o
@@ -70,7 +69,10 @@ obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo un
 obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o)
 
 ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
+ifeq ($(CONFIG_SYSCTL),y)
 obj-y += domctl.o
+obj-y += sysctl.o
+endif
 endif
 
 extra-y := symbols-dummy.o
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 8370b4b289..18ab294f94 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -194,14 +194,16 @@ kexec_op(unsigned long op, void *uarg)
 #ifdef CONFIG_IOREQ_SERVER
 dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
 #endif
-#ifdef CONFIG_SYSCTL
+#if !defined(CONFIG_PV_SHIM_EXCLUSIVE) && defined(CONFIG_SYSCTL)
 sysctl(xen_sysctl_t *u_sysctl)
 #endif
 #if defined(CONFIG_X86) && defined(CONFIG_PAGING)
 paging_domctl_cont(xen_domctl_t *u_domctl)
 #endif
 #ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#ifdef CONFIG_SYSCTL
 domctl(xen_domctl_t *u_domctl)
+#endif
 platform_op(xen_platform_op_t *u_xenpf_op)
 #endif
 #ifdef CONFIG_HVM
@@ -277,10 +279,8 @@ physdev_op                         compat   do       hvm      hvm      do_arm
 #ifdef CONFIG_HVM
 hvm_op                             do       do       do       do       do
 #endif
-#ifdef CONFIG_SYSCTL
+#if !defined(CONFIG_PV_SHIM_EXCLUSIVE) && defined(CONFIG_SYSCTL)
 sysctl                             do       do       do       do       do
-#endif
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
 domctl                             do       do       do       do       do
 #endif
 #ifdef CONFIG_KEXEC
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index e10baf2615..cb80e49de1 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
 
 int arch_vcpu_reset(struct vcpu *v);
 
+#if !defined(CONFIG_PV_SHIM_EXCLUSIVE) && defined(CONFIG_SYSCTL)
 bool domctl_lock_acquire(void);
 void domctl_lock_release(void);
+#else
+static inline bool domctl_lock_acquire(void)
+{
+    return false;
+}
+
+static inline void domctl_lock_release(void) {}
+#endif /* CONFIG_DOMCTL */
 
 /*
  * Continue the current hypercall via func(data) on specified cpu.
@@ -182,7 +191,7 @@ struct vnuma_info {
     struct xen_vmemrange *vmemrange;
 };
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if !defined(CONFIG_PV_SHIM_EXCLUSIVE) && defined(CONFIG_SYSCTL)
 void vnuma_destroy(struct vnuma_info *vnuma);
 #else
 static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); }

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 27.08.2025 02:33, Stefano Stabellini wrote:
> So I ran a test and the appended change, which is based on [1] and
> renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
> build issue.
> 
> For 4.21, I suggest we go with two patches:
> 1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> 2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS
> 
> Jan, are you OK with this?

Naming if the option aside, no, I fear I dislike the stubbing. What's
worse though, ...

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
>  
>  int arch_vcpu_reset(struct vcpu *v);
>  
> +#ifdef CONFIG_SYSCTL
>  bool domctl_lock_acquire(void);
>  void domctl_lock_release(void);
> +#else
> +static inline bool domctl_lock_acquire(void)
> +{
> +    return false;

... this will break x86'es HVM_PARAM_IDENT_PT handling. That is, in
principle I would agree that returning false here is appropriate. But
for the specific case there it's wrong.

As said on the call yesterday, until what you call MGMT_HYPERCALLS is
completely done, the option needs to be prompt-less, always-on. Adding
a prompt was necessary to be the last thing on the SYSCTL series, and
it'll need to be last on the follow-on one masking out further
hypercalls. IOW my take is that 34317c508294 and 568f806cba4c will
need reverting (the latter being what caused the regression, and the
former depending on the latter), to allow to cleanly continue that
work after the rename. If we don't do the reverts now (and take either
Penny's patch or what you propose), imo we'll need to do them later.
Else we're risking to introduce new randconfig breakages while the
further conversion work is ongoing.

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Wed, 27 Aug 2025, Jan Beulich wrote:
> On 27.08.2025 02:33, Stefano Stabellini wrote:
> > So I ran a test and the appended change, which is based on [1] and
> > renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
> > build issue.
> > 
> > For 4.21, I suggest we go with two patches:
> > 1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> > 2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS
> > 
> > Jan, are you OK with this?
> 
> Naming if the option aside, no, I fear I dislike the stubbing. What's
> worse though, ...
> 
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
> >  
> >  int arch_vcpu_reset(struct vcpu *v);
> >  
> > +#ifdef CONFIG_SYSCTL
> >  bool domctl_lock_acquire(void);
> >  void domctl_lock_release(void);
> > +#else
> > +static inline bool domctl_lock_acquire(void)
> > +{
> > +    return false;
> 
> ... this will break x86'es HVM_PARAM_IDENT_PT handling. That is, in
> principle I would agree that returning false here is appropriate. But
> for the specific case there it's wrong.

Uhm, that is a good point actually. And while in principle "false"
sounds appropriate, in practice there is no domctl.c to worry about
concurrency so "true" is what we want.


> As said on the call yesterday, until what you call MGMT_HYPERCALLS is
> completely done, the option needs to be prompt-less, always-on.

I do not think this is a good idea, because we would be unable to test
the configuration. Although we have been accepting code without tests,
that is not a good principle. At least with the current approach we can
run manual tests if automated tests are not available. If we make it
silent, we risk introducing broken code, or code soon-to-become broken.

In my view, we need to make gradual progress toward the goal. In this
case, we should move incrementally toward compiling out all the
"management" hypercalls. Also the alternative of waiting until all
patches are ready before committing them is not feasible. An incremental
approach reduces risk, preserves testability, and makes regressions
easier to identify.

An extreme example is that I could write:

static inline bool domctl_lock_acquire(void)
{
    obviously broken
}

and no tests would catch it.

So in short, I think we need to keep the prompt.


> Adding
> a prompt was necessary to be the last thing on the SYSCTL series, and
> it'll need to be last on the follow-on one masking out further
> hypercalls. IOW my take is that 34317c508294 and 568f806cba4c will
> need reverting (the latter being what caused the regression, and the
> former depending on the latter), to allow to cleanly continue that
> work after the rename. If we don't do the reverts now (and take either
> Penny's patch or what you propose), imo we'll need to do them later.
> Else we're risking to introduce new randconfig breakages while the
> further conversion work is ongoing.

My suggestion remains to go forward with 2 patches:
0) keep both 568f806cba4c and 34317c508294
1) rename CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
2) this patch with return true from domctl_lock_acquire

I am open to reverting 568f806cba4c but I don't think it would improve
things. I definitely don't think we should revert 34317c508294. We need
34317c508294 otherwise this patch doesn't fix the build.

This is why I think we need the prompt: otherwise we would not discover
even very basic important build breakages.

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 28.08.2025 02:52, Stefano Stabellini wrote:
> On Wed, 27 Aug 2025, Jan Beulich wrote:
>> On 27.08.2025 02:33, Stefano Stabellini wrote:
>>> So I ran a test and the appended change, which is based on [1] and
>>> renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
>>> build issue.
>>>
>>> For 4.21, I suggest we go with two patches:
>>> 1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
>>> 2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS
>>>
>>> Jan, are you OK with this?
>>
>> Naming if the option aside, no, I fear I dislike the stubbing. What's
>> worse though, ...
>>
>>> --- a/xen/include/xen/domain.h
>>> +++ b/xen/include/xen/domain.h
>>> @@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
>>>  
>>>  int arch_vcpu_reset(struct vcpu *v);
>>>  
>>> +#ifdef CONFIG_SYSCTL
>>>  bool domctl_lock_acquire(void);
>>>  void domctl_lock_release(void);
>>> +#else
>>> +static inline bool domctl_lock_acquire(void)
>>> +{
>>> +    return false;
>>
>> ... this will break x86'es HVM_PARAM_IDENT_PT handling. That is, in
>> principle I would agree that returning false here is appropriate. But
>> for the specific case there it's wrong.
> 
> Uhm, that is a good point actually. And while in principle "false"
> sounds appropriate, in practice there is no domctl.c to worry about
> concurrency so "true" is what we want.

Except that, as said, conceptually "true" is the wrong value to use in
such a stub.

>> As said on the call yesterday, until what you call MGMT_HYPERCALLS is
>> completely done, the option needs to be prompt-less, always-on.
> 
> I do not think this is a good idea, because we would be unable to test
> the configuration. Although we have been accepting code without tests,
> that is not a good principle. At least with the current approach we can
> run manual tests if automated tests are not available. If we make it
> silent, we risk introducing broken code, or code soon-to-become broken.
> 
> In my view, we need to make gradual progress toward the goal. In this
> case, we should move incrementally toward compiling out all the
> "management" hypercalls. Also the alternative of waiting until all
> patches are ready before committing them is not feasible. An incremental
> approach reduces risk, preserves testability, and makes regressions
> easier to identify.

If that's your view, then why did you not comment on the SYSCTL series,
when I asked the prompt to appear last?

> An extreme example is that I could write:
> 
> static inline bool domctl_lock_acquire(void)
> {
>     obviously broken
> }
> 
> and no tests would catch it.

Tests would catch it at the point the prompt is added. Much like it was
with the SYSCTL series (and why, with the prompt removed, the rest of
the series can stay in).

>> Adding
>> a prompt was necessary to be the last thing on the SYSCTL series, and
>> it'll need to be last on the follow-on one masking out further
>> hypercalls. IOW my take is that 34317c508294 and 568f806cba4c will
>> need reverting (the latter being what caused the regression, and the
>> former depending on the latter), to allow to cleanly continue that
>> work after the rename. If we don't do the reverts now (and take either
>> Penny's patch or what you propose), imo we'll need to do them later.
>> Else we're risking to introduce new randconfig breakages while the
>> further conversion work is ongoing.
> 
> My suggestion remains to go forward with 2 patches:
> 0) keep both 568f806cba4c and 34317c508294
> 1) rename CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> 2) this patch with return true from domctl_lock_acquire
> 
> I am open to reverting 568f806cba4c but I don't think it would improve
> things. I definitely don't think we should revert 34317c508294. We need
> 34317c508294 otherwise this patch doesn't fix the build.

If "this patch" is the one outlined here, then with the reverts we wouldn't
need it at all. The reverts alone will fix the build issue, according to my
understanding.

Jan

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

On Thu, 28 Aug 2025, Jan Beulich wrote:
> On 28.08.2025 02:52, Stefano Stabellini wrote:
> > On Wed, 27 Aug 2025, Jan Beulich wrote:
> >> On 27.08.2025 02:33, Stefano Stabellini wrote:
> >>> So I ran a test and the appended change, which is based on [1] and
> >>> renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
> >>> build issue.
> >>>
> >>> For 4.21, I suggest we go with two patches:
> >>> 1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> >>> 2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS
> >>>
> >>> Jan, are you OK with this?
> >>
> >> Naming if the option aside, no, I fear I dislike the stubbing. What's
> >> worse though, ...
> >>
> >>> --- a/xen/include/xen/domain.h
> >>> +++ b/xen/include/xen/domain.h
> >>> @@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
> >>>  
> >>>  int arch_vcpu_reset(struct vcpu *v);
> >>>  
> >>> +#ifdef CONFIG_SYSCTL
> >>>  bool domctl_lock_acquire(void);
> >>>  void domctl_lock_release(void);
> >>> +#else
> >>> +static inline bool domctl_lock_acquire(void)
> >>> +{
> >>> +    return false;
> >>
> >> ... this will break x86'es HVM_PARAM_IDENT_PT handling. That is, in
> >> principle I would agree that returning false here is appropriate. But
> >> for the specific case there it's wrong.
> > 
> > Uhm, that is a good point actually. And while in principle "false"
> > sounds appropriate, in practice there is no domctl.c to worry about
> > concurrency so "true" is what we want.
> 
> Except that, as said, conceptually "true" is the wrong value to use in
> such a stub.
> 
> >> As said on the call yesterday, until what you call MGMT_HYPERCALLS is
> >> completely done, the option needs to be prompt-less, always-on.
> > 
> > I do not think this is a good idea, because we would be unable to test
> > the configuration. Although we have been accepting code without tests,
> > that is not a good principle. At least with the current approach we can
> > run manual tests if automated tests are not available. If we make it
> > silent, we risk introducing broken code, or code soon-to-become broken.
> > 
> > In my view, we need to make gradual progress toward the goal. In this
> > case, we should move incrementally toward compiling out all the
> > "management" hypercalls. Also the alternative of waiting until all
> > patches are ready before committing them is not feasible. An incremental
> > approach reduces risk, preserves testability, and makes regressions
> > easier to identify.
> 
> If that's your view, then why did you not comment on the SYSCTL series,
> when I asked the prompt to appear last?

I am not trying to be obtuse, but I am not sure what you mean by this.
In any case, I do not recall reading a specific email on this topic. I
try my best to follow other review comments, but I may have overlooked
this one.


> > An extreme example is that I could write:
> > 
> > static inline bool domctl_lock_acquire(void)
> > {
> >     obviously broken
> > }
> > 
> > and no tests would catch it.
> 
> Tests would catch it at the point the prompt is added. Much like it was
> with the SYSCTL series (and why, with the prompt removed, the rest of
> the series can stay in).

In my opinion, this is a no-go. The code must function correctly, and
that is my top priority, certainly above conceptual issues, such as the
return value of domctl_lock_acquire. With your suggestion, there is no
guarantee the code works and there is no way to test it.


> >> Adding
> >> a prompt was necessary to be the last thing on the SYSCTL series, and
> >> it'll need to be last on the follow-on one masking out further
> >> hypercalls. IOW my take is that 34317c508294 and 568f806cba4c will
> >> need reverting (the latter being what caused the regression, and the
> >> former depending on the latter), to allow to cleanly continue that
> >> work after the rename. If we don't do the reverts now (and take either
> >> Penny's patch or what you propose), imo we'll need to do them later.
> >> Else we're risking to introduce new randconfig breakages while the
> >> further conversion work is ongoing.
> > 
> > My suggestion remains to go forward with 2 patches:
> > 0) keep both 568f806cba4c and 34317c508294
> > 1) rename CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> > 2) this patch with return true from domctl_lock_acquire
> > 
> > I am open to reverting 568f806cba4c but I don't think it would improve
> > things. I definitely don't think we should revert 34317c508294. We need
> > 34317c508294 otherwise this patch doesn't fix the build.
> 
> If "this patch" is the one outlined here, then with the reverts we wouldn't
> need it at all. The reverts alone will fix the build issue, according to my
> understanding.

The reverts you are suggesting do not fix the issue; they only hide it.
The Kconfig option can no longer be disabled, which renders the entire
patch series ineffective. In fact, the #ifdef's could be completely
incorrect, and the code would still build, as in my example:

static inline bool domctl_lock_acquire(void)
{
    obviously broken
}

It looks like we'll need a third opinion to make forward progress.

Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Jan Beulich]

On 29.08.2025 01:41, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, Jan Beulich wrote:
>> On 28.08.2025 02:52, Stefano Stabellini wrote:
>>> On Wed, 27 Aug 2025, Jan Beulich wrote:
>>>> On 27.08.2025 02:33, Stefano Stabellini wrote:
>>>>> --- a/xen/include/xen/domain.h
>>>>> +++ b/xen/include/xen/domain.h
>>>>> @@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
>>>>>  
>>>>>  int arch_vcpu_reset(struct vcpu *v);
>>>>>  
>>>>> +#ifdef CONFIG_SYSCTL
>>>>>  bool domctl_lock_acquire(void);
>>>>>  void domctl_lock_release(void);
>>>>> +#else
>>>>> +static inline bool domctl_lock_acquire(void)
>>>>> +{
>>>>> +    return false;
>>>>
>>>> ... this will break x86'es HVM_PARAM_IDENT_PT handling. That is, in
>>>> principle I would agree that returning false here is appropriate. But
>>>> for the specific case there it's wrong.
>>>
>>> Uhm, that is a good point actually. And while in principle "false"
>>> sounds appropriate, in practice there is no domctl.c to worry about
>>> concurrency so "true" is what we want.
>>
>> Except that, as said, conceptually "true" is the wrong value to use in
>> such a stub.
>>
>>>> As said on the call yesterday, until what you call MGMT_HYPERCALLS is
>>>> completely done, the option needs to be prompt-less, always-on.
>>>
>>> I do not think this is a good idea, because we would be unable to test
>>> the configuration. Although we have been accepting code without tests,
>>> that is not a good principle. At least with the current approach we can
>>> run manual tests if automated tests are not available. If we make it
>>> silent, we risk introducing broken code, or code soon-to-become broken.
>>>
>>> In my view, we need to make gradual progress toward the goal. In this
>>> case, we should move incrementally toward compiling out all the
>>> "management" hypercalls. Also the alternative of waiting until all
>>> patches are ready before committing them is not feasible. An incremental
>>> approach reduces risk, preserves testability, and makes regressions
>>> easier to identify.
>>
>> If that's your view, then why did you not comment on the SYSCTL series,
>> when I asked the prompt to appear last?
> 
> I am not trying to be obtuse, but I am not sure what you mean by this.
> In any case, I do not recall reading a specific email on this topic. I
> try my best to follow other review comments, but I may have overlooked
> this one.

Originally Penny introduced the option with prompt, very early in the
series. It became clear very quickly that this way she introduced
randconfig issues, for the case where randconfig could have chosen to
turn the option off. Hence why I asked that the option be introduced
prompt-less, then all #ifdef-ary be added, and then the option would
gain a prompt.

>>> An extreme example is that I could write:
>>>
>>> static inline bool domctl_lock_acquire(void)
>>> {
>>>     obviously broken
>>> }
>>>
>>> and no tests would catch it.
>>
>> Tests would catch it at the point the prompt is added. Much like it was
>> with the SYSCTL series (and why, with the prompt removed, the rest of
>> the series can stay in).
> 
> In my opinion, this is a no-go. The code must function correctly, and
> that is my top priority, certainly above conceptual issues, such as the
> return value of domctl_lock_acquire. With your suggestion, there is no
> guarantee the code works and there is no way to test it.

The code working correctly will be tested at the point the option gains
the prompt.

>>>> Adding
>>>> a prompt was necessary to be the last thing on the SYSCTL series, and
>>>> it'll need to be last on the follow-on one masking out further
>>>> hypercalls. IOW my take is that 34317c508294 and 568f806cba4c will
>>>> need reverting (the latter being what caused the regression, and the
>>>> former depending on the latter), to allow to cleanly continue that
>>>> work after the rename. If we don't do the reverts now (and take either
>>>> Penny's patch or what you propose), imo we'll need to do them later.
>>>> Else we're risking to introduce new randconfig breakages while the
>>>> further conversion work is ongoing.
>>>
>>> My suggestion remains to go forward with 2 patches:
>>> 0) keep both 568f806cba4c and 34317c508294
>>> 1) rename CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
>>> 2) this patch with return true from domctl_lock_acquire
>>>
>>> I am open to reverting 568f806cba4c but I don't think it would improve
>>> things. I definitely don't think we should revert 34317c508294. We need
>>> 34317c508294 otherwise this patch doesn't fix the build.
>>
>> If "this patch" is the one outlined here, then with the reverts we wouldn't
>> need it at all. The reverts alone will fix the build issue, according to my
>> understanding.
> 
> The reverts you are suggesting do not fix the issue; they only hide it.
> The Kconfig option can no longer be disabled, which renders the entire
> patch series ineffective.

Yes, hence why we wouldn't need a revert of the entire series.

Jan

Domctl series as a fix to PV_SHIM_EXCLUSIVE, was: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini]

Hi Oleksii,

We previously discussed the PV_SHIM_EXCLUSIVE build issue on Matrix
and agreed on resolving it after the feature freeze as a fix. This
conversation took place before the feature freeze was rescheduled to
September 5. I am documenting this on xen-devel both for record-keeping
and because Matrix is currently offline. I am proceeding under the
assumption that the conclusions from that discussion still apply.

I would like to request an additional clarification. While there is no
consensus among the maintainers on the preferred short-term compromise,
there is agreement that reviewing and committing the domctl series [1]
in time would be the best outcome. (Together with unifying CONFIG_SYSCTL
and CONFIG_DOMCTL into a single CONFIG_MGMT_HYPERCALLS for simplicify.)

Are you still OK with us going ahead with reviewing and committing the
domctl series as a fix to the PV_SHIM_EXCLUSIVE issue, potentially going
past the feature freeze by a week?

Many thanks!

Cheers,

Stefano

[1] https://marc.info/?l=xen-devel&m=175421442423500

Re: Domctl series as a fix to PV_SHIM_EXCLUSIVE, was: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

Hello Stefano,

On 9/3/25 12:16 AM, Stefano Stabellini wrote:
> Hi Oleksii,
>
> We previously discussed the PV_SHIM_EXCLUSIVE build issue on Matrix
> and agreed on resolving it after the feature freeze as a fix. This
> conversation took place before the feature freeze was rescheduled to
> September 5. I am documenting this on xen-devel both for record-keeping
> and because Matrix is currently offline. I am proceeding under the
> assumption that the conclusions from that discussion still apply.

Thank you for documenting this on xen-devel.


>
> I would like to request an additional clarification. While there is no
> consensus among the maintainers on the preferred short-term compromise,
> there is agreement that reviewing and committing the domctl series [1]
> in time would be the best outcome. (Together with unifying CONFIG_SYSCTL
> and CONFIG_DOMCTL into a single CONFIG_MGMT_HYPERCALLS for simplicify.)
>
> Are you still OK with us going ahead with reviewing and committing the
> domctl series as a fix to the PV_SHIM_EXCLUSIVE issue, potentially going
> past the feature freeze by a week?

Regarding your request for clarification on the domctl series [1], I agree
that reviewing and committing this series as a fix for the PV_SHIM_EXCLUSIV
issue is the preferred short-term solution.
Given that the series primarily consists of refactoring changes with minimal
functional impact, I am comfortable with making an exception to the feature
freeze to allow its inclusion, potentially extending up to one week past
September 5 (only for this patch series).

But if it will/could take more time then I think that we should consider
an option just to revert some patches, IIUC it should fix the mentioned issue
too.

~ Oleksii

> Stefano
>
> [1]https://marc.info/?l=xen-devel&m=175421442423500

[-- Attachment #2: Type: text/html, Size: 2730 bytes --]