* [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-28 23:56 ` Stefano Stabellini
2025-03-26 5:50 ` [PATCH v2 02/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (17 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
We intend to remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally
equivalent "if !...") in Kconfig file, since negative dependancy will badly
affect allyesconfig.
This commit is based on "x86: provide an inverted Kconfig control for
shim-exclusive mode"[1]
[1] https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
xen/arch/x86/Kconfig | 6 ++----
xen/arch/x86/hvm/Kconfig | 1 -
xen/drivers/video/Kconfig | 4 ++--
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 18efdb2e31..1e5df84b25 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -137,7 +137,6 @@ config XEN_IBT
config SHADOW_PAGING
bool "Shadow Paging"
- default !PV_SHIM_EXCLUSIVE
depends on PV || HVM
help
@@ -169,7 +168,6 @@ config BIGMEM
config TBOOT
bool "Xen tboot support (UNSUPPORTED)"
depends on INTEL && UNSUPPORTED
- default !PV_SHIM_EXCLUSIVE
select CRYPTO
help
Allows support for Trusted Boot using the Intel(R) Trusted Execution
@@ -279,10 +277,11 @@ config PV_SHIM_EXCLUSIVE
Build Xen in a way which unconditionally assumes PV_SHIM mode. This
option is only intended for use when building a dedicated PV Shim
firmware, and will not function correctly in other scenarios.
+ Features, like tboot, shadow page, VGA, HVM, Hyper-V Guest, etc,
+ are unavailable in shim-exclusive mode.
If unsure, say N.
-if !PV_SHIM_EXCLUSIVE
config HYPERV_GUEST
bool "Hyper-V Guest"
@@ -292,7 +291,6 @@ config HYPERV_GUEST
If unsure, say N.
-endif
config REQUIRE_NX
bool "Require NX (No eXecute) support"
diff --git a/xen/arch/x86/hvm/Kconfig b/xen/arch/x86/hvm/Kconfig
index 2def0f98e2..b903764bda 100644
--- a/xen/arch/x86/hvm/Kconfig
+++ b/xen/arch/x86/hvm/Kconfig
@@ -1,6 +1,5 @@
menuconfig HVM
bool "HVM support"
- depends on !PV_SHIM_EXCLUSIVE
default !PV_SHIM
select COMPAT
select IOREQ_SERVER
diff --git a/xen/drivers/video/Kconfig b/xen/drivers/video/Kconfig
index 245030beea..66ee1e7c9c 100644
--- a/xen/drivers/video/Kconfig
+++ b/xen/drivers/video/Kconfig
@@ -3,10 +3,10 @@ config VIDEO
bool
config VGA
- bool "VGA support" if !PV_SHIM_EXCLUSIVE
+ bool "VGA support"
select VIDEO
depends on X86
- default y if !PV_SHIM_EXCLUSIVE
+ default y
help
Enable VGA output for the Xen hypervisor.
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"
2025-03-26 5:50 ` [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE" Penny Zheng
@ 2025-03-28 23:56 ` Stefano Stabellini
2025-03-31 6:29 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-28 23:56 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
On Wed, 26 Mar 2025, Penny Zheng wrote:
> We intend to remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally
> equivalent "if !...") in Kconfig file, since negative dependancy will badly
> affect allyesconfig.
> This commit is based on "x86: provide an inverted Kconfig control for
> shim-exclusive mode"[1]
>
> [1] https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> xen/arch/x86/Kconfig | 6 ++----
> xen/arch/x86/hvm/Kconfig | 1 -
> xen/drivers/video/Kconfig | 4 ++--
> 3 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 18efdb2e31..1e5df84b25 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -137,7 +137,6 @@ config XEN_IBT
>
> config SHADOW_PAGING
> bool "Shadow Paging"
> - default !PV_SHIM_EXCLUSIVE
> depends on PV || HVM
> help
>
> @@ -169,7 +168,6 @@ config BIGMEM
> config TBOOT
> bool "Xen tboot support (UNSUPPORTED)"
> depends on INTEL && UNSUPPORTED
> - default !PV_SHIM_EXCLUSIVE
> select CRYPTO
> help
> Allows support for Trusted Boot using the Intel(R) Trusted Execution
> @@ -279,10 +277,11 @@ config PV_SHIM_EXCLUSIVE
> Build Xen in a way which unconditionally assumes PV_SHIM mode. This
> option is only intended for use when building a dedicated PV Shim
> firmware, and will not function correctly in other scenarios.
> + Features, like tboot, shadow page, VGA, HVM, Hyper-V Guest, etc,
> + are unavailable in shim-exclusive mode.
I don't know if we want to add these two lines or not. Either way:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> If unsure, say N.
>
> -if !PV_SHIM_EXCLUSIVE
>
> config HYPERV_GUEST
> bool "Hyper-V Guest"
> @@ -292,7 +291,6 @@ config HYPERV_GUEST
>
> If unsure, say N.
>
> -endif
>
> config REQUIRE_NX
> bool "Require NX (No eXecute) support"
> diff --git a/xen/arch/x86/hvm/Kconfig b/xen/arch/x86/hvm/Kconfig
> index 2def0f98e2..b903764bda 100644
> --- a/xen/arch/x86/hvm/Kconfig
> +++ b/xen/arch/x86/hvm/Kconfig
> @@ -1,6 +1,5 @@
> menuconfig HVM
> bool "HVM support"
> - depends on !PV_SHIM_EXCLUSIVE
> default !PV_SHIM
> select COMPAT
> select IOREQ_SERVER
> diff --git a/xen/drivers/video/Kconfig b/xen/drivers/video/Kconfig
> index 245030beea..66ee1e7c9c 100644
> --- a/xen/drivers/video/Kconfig
> +++ b/xen/drivers/video/Kconfig
> @@ -3,10 +3,10 @@ config VIDEO
> bool
>
> config VGA
> - bool "VGA support" if !PV_SHIM_EXCLUSIVE
> + bool "VGA support"
> select VIDEO
> depends on X86
> - default y if !PV_SHIM_EXCLUSIVE
> + default y
> help
> Enable VGA output for the Xen hypervisor.
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"
2025-03-28 23:56 ` Stefano Stabellini
@ 2025-03-31 6:29 ` Jan Beulich
2025-04-01 8:41 ` Penny, Zheng
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2025-03-31 6:29 UTC (permalink / raw)
To: Stefano Stabellini, Penny Zheng
Cc: xen-devel, ray.huang, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall
On 29.03.2025 00:56, Stefano Stabellini wrote:
> On Wed, 26 Mar 2025, Penny Zheng wrote:
>> We intend to remove all "depends on !PV_SHIM_EXCLUSIVE" (also the functionally
>> equivalent "if !...") in Kconfig file, since negative dependancy will badly
>> affect allyesconfig.
>> This commit is based on "x86: provide an inverted Kconfig control for
>> shim-exclusive mode"[1]
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Where's this coming from, if I may ask?
>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>> ---
>> xen/arch/x86/Kconfig | 6 ++----
>> xen/arch/x86/hvm/Kconfig | 1 -
>> xen/drivers/video/Kconfig | 4 ++--
>> 3 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 18efdb2e31..1e5df84b25 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -137,7 +137,6 @@ config XEN_IBT
>>
>> config SHADOW_PAGING
>> bool "Shadow Paging"
>> - default !PV_SHIM_EXCLUSIVE
>> depends on PV || HVM
>> help
>>
>> @@ -169,7 +168,6 @@ config BIGMEM
>> config TBOOT
>> bool "Xen tboot support (UNSUPPORTED)"
>> depends on INTEL && UNSUPPORTED
>> - default !PV_SHIM_EXCLUSIVE
>> select CRYPTO
>> help
>> Allows support for Trusted Boot using the Intel(R) Trusted Execution
>> @@ -279,10 +277,11 @@ config PV_SHIM_EXCLUSIVE
>> Build Xen in a way which unconditionally assumes PV_SHIM mode. This
>> option is only intended for use when building a dedicated PV Shim
>> firmware, and will not function correctly in other scenarios.
>> + Features, like tboot, shadow page, VGA, HVM, Hyper-V Guest, etc,
>> + are unavailable in shim-exclusive mode.
>
> I don't know if we want to add these two lines or not. Either way:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
I don't think we should add that. It's also wrong for shadow in the first
place (where it was a default only anyway, not a dependency).
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"
2025-03-31 6:29 ` Jan Beulich
@ 2025-04-01 8:41 ` Penny, Zheng
2025-04-01 9:01 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Penny, Zheng @ 2025-04-01 8:41 UTC (permalink / raw)
To: Jan Beulich, Stefano Stabellini
Cc: xen-devel@lists.xenproject.org, Huang, Ray, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Orzel, Michal, Julien Grall
[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, March 31, 2025 2:30 PM
> To: Stefano Stabellini <sstabellini@kernel.org>; Penny, Zheng
> <penny.zheng@amd.com>
> Cc: xen-devel@lists.xenproject.org; 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>
> Subject: Re: [PATCH v2 01/19] xen/x86: remove "depends
> on !PV_SHIM_EXCLUSIVE"
>
> On 29.03.2025 00:56, Stefano Stabellini wrote:
> > On Wed, 26 Mar 2025, Penny Zheng wrote:
> >> We intend to remove all "depends on !PV_SHIM_EXCLUSIVE" (also the
> >> functionally equivalent "if !...") in Kconfig file, since negative
> >> dependancy will badly affect allyesconfig.
> >> This commit is based on "x86: provide an inverted Kconfig control for
> >> shim-exclusive mode"[1]
> >>
> >> [1]
> >> https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Where's this coming from, if I may ask?
>
I said in the commit message, this commit is based on your commit "x86: provide an inverted Kconfig control for
shim-exclusive mode"[1].
So I think I shall add-in the original author, if it is not the rule, I'll remove it.
> >> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> >> ---
> >> xen/arch/x86/Kconfig | 6 ++----
> >> xen/arch/x86/hvm/Kconfig | 1 -
> >> xen/drivers/video/Kconfig | 4 ++--
> >> 3 files changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index
> >> 18efdb2e31..1e5df84b25 100644
> >> --- a/xen/arch/x86/Kconfig
> >> +++ b/xen/arch/x86/Kconfig
> >> @@ -137,7 +137,6 @@ config XEN_IBT
> >>
> >> config SHADOW_PAGING
> >> bool "Shadow Paging"
> >> - default !PV_SHIM_EXCLUSIVE
> >> depends on PV || HVM
> >> help
> >>
> >> @@ -169,7 +168,6 @@ config BIGMEM
> >> config TBOOT
> >> bool "Xen tboot support (UNSUPPORTED)"
> >> depends on INTEL && UNSUPPORTED
> >> - default !PV_SHIM_EXCLUSIVE
> >> select CRYPTO
> >> help
> >> Allows support for Trusted Boot using the Intel(R) Trusted
> >> Execution @@ -279,10 +277,11 @@ config PV_SHIM_EXCLUSIVE
> >> Build Xen in a way which unconditionally assumes PV_SHIM mode. This
> >> option is only intended for use when building a dedicated PV Shim
> >> firmware, and will not function correctly in other scenarios.
> >> + Features, like tboot, shadow page, VGA, HVM, Hyper-V Guest, etc,
> >> + are unavailable in shim-exclusive mode.
> >
> > I don't know if we want to add these two lines or not. Either way:
> >
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
> I don't think we should add that. It's also wrong for shadow in the first place (where
> it was a default only anyway, not a dependency).
Ack, I'll remove
>
> Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"
2025-04-01 8:41 ` Penny, Zheng
@ 2025-04-01 9:01 ` Jan Beulich
2025-04-01 9:57 ` Penny, Zheng
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 9:01 UTC (permalink / raw)
To: Penny, Zheng
Cc: xen-devel@lists.xenproject.org, Huang, Ray, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Orzel, Michal, Julien Grall,
Stefano Stabellini
On 01.04.2025 10:41, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, March 31, 2025 2:30 PM
>>
>> On 29.03.2025 00:56, Stefano Stabellini wrote:
>>> On Wed, 26 Mar 2025, Penny Zheng wrote:
>>>> We intend to remove all "depends on !PV_SHIM_EXCLUSIVE" (also the
>>>> functionally equivalent "if !...") in Kconfig file, since negative
>>>> dependancy will badly affect allyesconfig.
>>>> This commit is based on "x86: provide an inverted Kconfig control for
>>>> shim-exclusive mode"[1]
>>>>
>>>> [1]
>>>> https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Where's this coming from, if I may ask?
>
> I said in the commit message, this commit is based on your commit "x86: provide an inverted Kconfig control for
> shim-exclusive mode"[1].
I don't think this belongs there. Also recall what I said elsewhere about
"This commit ..." and alike not being appropriate wording for commit messages.
> So I think I shall add-in the original author, if it is not the rule, I'll remove it.
Please remove it. You necessarily touch a few of the same places, but that's
about it. I accept this route being taken, but I don't agree with it. I don't
want to be viewed as a co-author in such a case.
However, you having gone from that patch (which had an entirely different
intention), has lead to the patch here being incomplete. At least my
understanding of Andrew's original request was to not only prune Kconfig-s of
the dependency, but also e.g. various Makefile-s. Possibly even .c and .h
ones. That clearly wasn't necessary with the approach I had taken. Please
consult with Andrew to confirm.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"
2025-04-01 9:01 ` Jan Beulich
@ 2025-04-01 9:57 ` Penny, Zheng
0 siblings, 0 replies; 55+ messages in thread
From: Penny, Zheng @ 2025-04-01 9:57 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xenproject.org, Huang, Ray, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Orzel, Michal, Julien Grall,
Stefano Stabellini
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 1, 2025 5:02 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: xen-devel@lists.xenproject.org; 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>
> Subject: Re: [PATCH v2 01/19] xen/x86: remove "depends
> on !PV_SHIM_EXCLUSIVE"
>
> On 01.04.2025 10:41, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Monday, March 31, 2025 2:30 PM
> >>
> >> On 29.03.2025 00:56, Stefano Stabellini wrote:
> >>> On Wed, 26 Mar 2025, Penny Zheng wrote:
> >>>> We intend to remove all "depends on !PV_SHIM_EXCLUSIVE" (also the
> >>>> functionally equivalent "if !...") in Kconfig file, since negative
> >>>> dependancy will badly affect allyesconfig.
> >>>> This commit is based on "x86: provide an inverted Kconfig control
> >>>> for shim-exclusive mode"[1]
> >>>>
> >>>> [1]
> >>>> https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> Where's this coming from, if I may ask?
> >
> > I said in the commit message, this commit is based on your commit
> > "x86: provide an inverted Kconfig control for shim-exclusive mode"[1].
>
> I don't think this belongs there. Also recall what I said elsewhere about "This
> commit ..." and alike not being appropriate wording for commit messages.
>
Understood, I'll remove "This commit is based on xxx".
> > So I think I shall add-in the original author, if it is not the rule, I'll remove it.
>
> Please remove it. You necessarily touch a few of the same places, but that's about
> it. I accept this route being taken, but I don't agree with it. I don't want to be viewed
> as a co-author in such a case.
>
Understood, I'll remove.
> However, you having gone from that patch (which had an entirely different
> intention), has lead to the patch here being incomplete. At least my understanding
> of Andrew's original request was to not only prune Kconfig-s of the dependency,
> but also e.g. various Makefile-s. Possibly even .c and .h ones. That clearly wasn't
> necessary with the approach I had taken. Please consult with Andrew to confirm.
>
> Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 02/19] xen: introduce CONFIG_SYSCTL
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
2025-03-26 5:50 ` [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE" Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-27 9:57 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 03/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL Penny Zheng
` (16 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Stefano Stabellini, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Sergiy Kibrik, Penny Zheng
From: Stefano Stabellini <stefano.stabellini@amd.com>
We intend to introduces a new Kconfig CONFIG_SYSCTL, which shall only
be disabled on some dom0less systems, to reduce Xen footprint.
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- complement missing commit message
- re-placing the kconfig at the file end
- complement a "help" statement
---
xen/common/Kconfig | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index a6aa2c5c14..fe9303c7c1 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -545,4 +545,15 @@ config BUDDY_ALLOCATOR_SIZE
Amount of memory reserved for the buddy allocator to serve Xen heap,
working alongside the colored one.
+menu "Supported hypercall interfaces"
+ visible if EXPERT
+
+config SYSCTL
+ bool "Enable sysctl hypercall"
+ default y
+ help
+ This option shall only be disabled on some dom0less systems,
+ to reduce Xen footprint.
+endmenu
+
endmenu
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 02/19] xen: introduce CONFIG_SYSCTL
2025-03-26 5:50 ` [PATCH v2 02/19] xen: introduce CONFIG_SYSCTL Penny Zheng
@ 2025-03-27 9:57 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-03-27 9:57 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Stefano Stabellini, Andrew Cooper, Anthony PERARD,
Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Sergiy Kibrik, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> We intend to introduces a new Kconfig CONFIG_SYSCTL, which shall only
> be disabled on some dom0less systems, to reduce Xen footprint.
Nit: "We intend to ..." takes about future work, yet the new control is ...
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -545,4 +545,15 @@ config BUDDY_ALLOCATOR_SIZE
> Amount of memory reserved for the buddy allocator to serve Xen heap,
> working alongside the colored one.
>
> +menu "Supported hypercall interfaces"
> + visible if EXPERT
> +
> +config SYSCTL
> + bool "Enable sysctl hypercall"
> + default y
> + help
> + This option shall only be disabled on some dom0less systems,
> + to reduce Xen footprint.
> +endmenu
> +
> endmenu
... introduced right here.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 03/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
2025-03-26 5:50 ` [PATCH v2 01/19] xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE" Penny Zheng
2025-03-26 5:50 ` [PATCH v2 02/19] xen: introduce CONFIG_SYSCTL Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:06 ` Stefano Stabellini
2025-03-26 5:50 ` [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole Penny Zheng
` (15 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel; +Cc: ray.huang, Penny Zheng, Daniel P. Smith
As function xsm_sysctl() is solely invoked in sysctl.c, we need to
wrap around it with CONFIG_SYSCTL
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2
- remove unnecessary redundancy in dummy handler
- re-place #ifdef into function body
---
xen/include/xsm/xsm.h | 4 ++++
xen/xsm/dummy.c | 2 ++
xen/xsm/flask/hooks.c | 4 ++++
3 files changed, 10 insertions(+)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 24acc16125..22e2429f52 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -261,7 +261,11 @@ static inline int xsm_domctl(xsm_default_t def, struct domain *d,
static inline int xsm_sysctl(xsm_default_t def, int cmd)
{
+#ifdef CONFIG_SYSCTL
return alternative_call(xsm_ops.sysctl, cmd);
+#else
+ return -EOPNOTSUPP;
+#endif
}
static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 93fbfc43cc..93a0665ecc 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -22,7 +22,9 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
.sysctl_scheduler_op = xsm_sysctl_scheduler_op,
.set_target = xsm_set_target,
.domctl = xsm_domctl,
+#ifdef CONFIG_SYSCTL
.sysctl = xsm_sysctl,
+#endif
.readconsole = xsm_readconsole,
.evtchn_unbound = xsm_evtchn_unbound,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 6a53487ea4..befe9336ee 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -856,6 +856,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned int cmd,
}
}
+#ifdef CONFIG_SYSCTL
static int cf_check flask_sysctl(int cmd)
{
switch ( cmd )
@@ -933,6 +934,7 @@ static int cf_check flask_sysctl(int cmd)
return avc_unknown_permission("sysctl", cmd);
}
}
+#endif
static int cf_check flask_readconsole(uint32_t clear)
{
@@ -1884,7 +1886,9 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
.sysctl_scheduler_op = flask_sysctl_scheduler_op,
.set_target = flask_set_target,
.domctl = flask_domctl,
+#ifdef CONFIG_SYSCTL
.sysctl = flask_sysctl,
+#endif
.readconsole = flask_readconsole,
.evtchn_unbound = flask_evtchn_unbound,
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (2 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 03/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:07 ` Stefano Stabellini
2025-04-01 12:24 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 05/19] xen/sysctl: make CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL Penny Zheng
` (14 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Daniel P. Smith
The following functions is to deal with XEN_SYSCTL_readconsole sub-op, and
shall be wrapped:
- xsm_readconsole
- read_console_ring
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- remove redundant dummy handle
- add transient #ifdef in sysctl.c for correct compilation
---
xen/common/sysctl.c | 2 ++
xen/drivers/char/console.c | 2 ++
xen/include/xsm/xsm.h | 4 ++++
xen/xsm/dummy.c | 2 +-
xen/xsm/flask/hooks.c | 4 ++--
5 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c2d99ae12e..dfc87a2846 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -58,6 +58,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
switch ( op->cmd )
{
+#ifdef CONFIG_SYSCTL
case XEN_SYSCTL_readconsole:
ret = xsm_readconsole(XSM_HOOK, op->u.readconsole.clear);
if ( ret )
@@ -66,6 +67,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = read_console_ring(&op->u.readconsole);
break;
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_tbuf_op:
ret = tb_control(&op->u.tbuf_op);
break;
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index c3150fbdb7..64f7e146a7 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -336,6 +336,7 @@ static void conring_puts(const char *str, size_t len)
conringc = conringp - conring_size;
}
+#ifdef CONFIG_SYSCTL
long read_console_ring(struct xen_sysctl_readconsole *op)
{
XEN_GUEST_HANDLE_PARAM(char) str;
@@ -378,6 +379,7 @@ long read_console_ring(struct xen_sysctl_readconsole *op)
return 0;
}
+#endif /* CONFIG_SYSCTL */
/*
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 22e2429f52..042a99449f 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -270,7 +270,11 @@ static inline int xsm_sysctl(xsm_default_t def, int cmd)
static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
{
+#ifdef CONFIG_SYSCTL
return alternative_call(xsm_ops.readconsole, clear);
+#else
+ return -EOPNOTSUPP;
+#endif
}
static inline int xsm_evtchn_unbound(
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 93a0665ecc..cd0e844fcf 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -24,8 +24,8 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
.domctl = xsm_domctl,
#ifdef CONFIG_SYSCTL
.sysctl = xsm_sysctl,
-#endif
.readconsole = xsm_readconsole,
+#endif
.evtchn_unbound = xsm_evtchn_unbound,
.evtchn_interdomain = xsm_evtchn_interdomain,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index befe9336ee..df7e10775b 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -934,7 +934,6 @@ static int cf_check flask_sysctl(int cmd)
return avc_unknown_permission("sysctl", cmd);
}
}
-#endif
static int cf_check flask_readconsole(uint32_t clear)
{
@@ -945,6 +944,7 @@ static int cf_check flask_readconsole(uint32_t clear)
return domain_has_xen(current->domain, perms);
}
+#endif /* CONFIG_SYSCTL */
static inline uint32_t resource_to_perm(uint8_t access)
{
@@ -1888,8 +1888,8 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
.domctl = flask_domctl,
#ifdef CONFIG_SYSCTL
.sysctl = flask_sysctl,
-#endif
.readconsole = flask_readconsole,
+#endif
.evtchn_unbound = flask_evtchn_unbound,
.evtchn_interdomain = flask_evtchn_interdomain,
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole
2025-03-26 5:50 ` [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole Penny Zheng
@ 2025-03-29 0:07 ` Stefano Stabellini
2025-04-01 12:24 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:07 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Daniel P. Smith
On Wed, 26 Mar 2025, Penny Zheng wrote:
> The following functions is to deal with XEN_SYSCTL_readconsole sub-op, and
> shall be wrapped:
> - xsm_readconsole
> - read_console_ring
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole
2025-03-26 5:50 ` [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole Penny Zheng
2025-03-29 0:07 ` Stefano Stabellini
@ 2025-04-01 12:24 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 12:24 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Daniel P. Smith, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -58,6 +58,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>
> switch ( op->cmd )
> {
> +#ifdef CONFIG_SYSCTL
> case XEN_SYSCTL_readconsole:
> ret = xsm_readconsole(XSM_HOOK, op->u.readconsole.clear);
> if ( ret )
> @@ -66,6 +67,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> ret = read_console_ring(&op->u.readconsole);
> break;
>
> +#endif /* CONFIG_SYSCTL */
> case XEN_SYSCTL_tbuf_op:
Nit: Can the #endif please live ahead of the blank line?
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 05/19] xen/sysctl: make CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (3 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 04/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-26 5:50 ` [PATCH v2 06/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id Penny Zheng
` (13 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
Users could only access trace buffers via hypercal XEN_SYSCTL_tbuf_op,
so this commit makes CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/common/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index fe9303c7c1..5b55ca6eaa 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -513,6 +513,7 @@ config DTB_FILE
config TRACEBUFFER
bool "Enable tracing infrastructure" if EXPERT
default y
+ depends on SYSCTL
help
Enable tracing infrastructure and pre-defined tracepoints within Xen.
This will allow live information about Xen's execution and performance
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 06/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (4 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 05/19] xen/sysctl: make CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-04-01 12:27 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 07/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op Penny Zheng
` (12 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Dario Faggioli, Juergen Gross,
George Dunlap, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
The following function shall be wrapped:
- scheduler_id
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/common/sched/core.c | 2 ++
xen/common/sysctl.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 9043414290..13fdf57e57 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2069,11 +2069,13 @@ long do_set_timer_op(s_time_t timeout)
return 0;
}
+#ifdef CONFIG_SYSCTL
/* scheduler_id - fetch ID of current scheduler */
int scheduler_id(void)
{
return operations.sched_id;
}
+#endif
/* Adjust scheduling parameter for a given domain. */
long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index dfc87a2846..c3646517bc 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -72,10 +72,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = tb_control(&op->u.tbuf_op);
break;
+#ifdef CONFIG_SYSCTL
case XEN_SYSCTL_sched_id:
op->u.sched_id.sched_id = scheduler_id();
break;
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_getdomaininfolist:
{
struct domain *d;
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 06/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id
2025-03-26 5:50 ` [PATCH v2 06/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id Penny Zheng
@ 2025-04-01 12:27 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 12:27 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Dario Faggioli, Juergen Gross, George Dunlap,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -72,10 +72,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> ret = tb_control(&op->u.tbuf_op);
> break;
>
> +#ifdef CONFIG_SYSCTL
> case XEN_SYSCTL_sched_id:
> op->u.sched_id.sched_id = scheduler_id();
> break;
>
> +#endif /* CONFIG_SYSCTL */
> case XEN_SYSCTL_getdomaininfolist:
Same as in the earlier patch (and I may not repeat this if later in the series
this re-occurs): #endif ahead of the blank line please.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 07/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (5 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 06/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:07 ` Stefano Stabellini
2025-03-26 5:50 ` [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op Penny Zheng
` (11 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
perfc_control() and perfc_copy_info() are responsible for providing control
of perf counters via XEN_SYSCTL_perfc_op in DOM0, so they both shall
be wrapped.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
- add transient #ifdef in sysctl.c for correct compilation
---
xen/common/perfc.c | 2 ++
xen/common/sysctl.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/xen/common/perfc.c b/xen/common/perfc.c
index 8302b7cf6d..0f3b89af2c 100644
--- a/xen/common/perfc.c
+++ b/xen/common/perfc.c
@@ -149,6 +149,7 @@ void cf_check perfc_reset(unsigned char key)
}
}
+#ifdef CONFIG_SYSCTL
static struct xen_sysctl_perfc_desc perfc_d[NR_PERFCTRS];
static xen_sysctl_perfc_val_t *perfc_vals;
static unsigned int perfc_nbr_vals;
@@ -265,6 +266,7 @@ int perfc_control(struct xen_sysctl_perfc_op *pc)
return rc;
}
+#endif /* CONFIG_SYSCTL */
/*
* Local variables:
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c3646517bc..0da82d6e31 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -117,11 +117,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
}
break;
+#ifdef CONFIG_SYSCTL
#ifdef CONFIG_PERF_COUNTERS
case XEN_SYSCTL_perfc_op:
ret = perfc_control(&op->u.perfc_op);
break;
#endif
+#endif /* CONFIG_SYSCTL */
#ifdef CONFIG_DEBUG_LOCK_PROFILE
case XEN_SYSCTL_lockprof_op:
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 07/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op
2025-03-26 5:50 ` [PATCH v2 07/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op Penny Zheng
@ 2025-03-29 0:07 ` Stefano Stabellini
0 siblings, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:07 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On Wed, 26 Mar 2025, Penny Zheng wrote:
> perfc_control() and perfc_copy_info() are responsible for providing control
> of perf counters via XEN_SYSCTL_perfc_op in DOM0, so they both shall
> be wrapped.
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (6 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 07/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:07 ` Stefano Stabellini
2025-04-01 12:36 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 09/19] xen/pmstat: clean up pmstat.c Penny Zheng
` (10 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
The following function is only to serve spinlock profiling via
XEN_SYSCTL_lockprof_op, so it shall be wrapped:
- spinlock_profile_control
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- add transient #ifdef in sysctl.c for correct compilation
---
xen/common/spinlock.c | 2 ++
xen/common/sysctl.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 38caa10a2e..0389293b09 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -690,6 +690,7 @@ void cf_check spinlock_profile_reset(unsigned char key)
spinlock_profile_iterate(spinlock_profile_reset_elem, NULL);
}
+#ifdef CONFIG_SYSCTL
typedef struct {
struct xen_sysctl_lockprof_op *pc;
int rc;
@@ -749,6 +750,7 @@ int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc)
return rc;
}
+#endif /* CONFIG_SYSCTL */
void _lock_profile_register_struct(
int32_t type, struct lock_profile_qhead *qhead, int32_t idx)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 0da82d6e31..4602d84203 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -123,13 +123,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = perfc_control(&op->u.perfc_op);
break;
#endif
-#endif /* CONFIG_SYSCTL */
#ifdef CONFIG_DEBUG_LOCK_PROFILE
case XEN_SYSCTL_lockprof_op:
ret = spinlock_profile_control(&op->u.lockprof_op);
break;
#endif
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_debug_keys:
{
char c;
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op
2025-03-26 5:50 ` [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op Penny Zheng
@ 2025-03-29 0:07 ` Stefano Stabellini
2025-04-01 12:36 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:07 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On Wed, 26 Mar 2025, Penny Zheng wrote:
> The following function is only to serve spinlock profiling via
> XEN_SYSCTL_lockprof_op, so it shall be wrapped:
> - spinlock_profile_control
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op
2025-03-26 5:50 ` [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op Penny Zheng
2025-03-29 0:07 ` Stefano Stabellini
@ 2025-04-01 12:36 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 12:36 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -123,13 +123,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> ret = perfc_control(&op->u.perfc_op);
> break;
> #endif
> -#endif /* CONFIG_SYSCTL */
>
> #ifdef CONFIG_DEBUG_LOCK_PROFILE
> case XEN_SYSCTL_lockprof_op:
> ret = spinlock_profile_control(&op->u.lockprof_op);
> break;
> #endif
> +#endif /* CONFIG_SYSCTL */
> case XEN_SYSCTL_debug_keys:
In such a situation please take the opportunity and add the blank line
that was missing ahead of the subsequent case label.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 09/19] xen/pmstat: clean up pmstat.c
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (7 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 08/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:41 ` Stefano Stabellini
2025-04-01 12:55 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS Penny Zheng
` (9 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel; +Cc: ray.huang, Penny Zheng, Jan Beulich
We intend to move the following functions into drivers/acpi/pmstat.c, as they
are all designed for performance statistic:
- cpufreq_residency_update
- cpufreq_statistic_reset
- cpufreq_statistic_update
- cpufreq_statistic_init
- cpufreq_statistic_exit
and moving out acpi_set_pdc_bits(), as it is the handler for sub-hypercall
XEN_PM_PDC, and shall stay with the other handlers together in
drivers/cpufreq/cpufreq.c.
This commit also applies various style corrections while moving these
functions
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- new commit
---
xen/drivers/acpi/pmstat.c | 199 ++++++++++++++++++----
xen/drivers/cpufreq/cpufreq.c | 31 ++++
xen/drivers/cpufreq/utility.c | 162 ------------------
xen/include/acpi/cpufreq/processor_perf.h | 2 -
4 files changed, 199 insertions(+), 195 deletions(-)
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index df309e27b4..4e3b76dbf3 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -43,6 +43,174 @@
DEFINE_PER_CPU_READ_MOSTLY(struct pm_px *, cpufreq_statistic_data);
+DEFINE_PER_CPU(spinlock_t, cpufreq_statistic_lock);
+
+/*********************************************************************
+ * Px STATISTIC INFO *
+ *********************************************************************/
+
+static void cpufreq_residency_update(unsigned int cpu, uint8_t state)
+{
+ uint64_t now, total_idle_ns;
+ int64_t delta;
+ struct pm_px *pxpt = per_cpu(cpufreq_statistic_data, cpu);
+
+ total_idle_ns = get_cpu_idle_time(cpu);
+ now = NOW();
+
+ delta = (now - pxpt->prev_state_wall) -
+ (total_idle_ns - pxpt->prev_idle_wall);
+
+ if ( likely(delta >= 0) )
+ pxpt->u.pt[state].residency += delta;
+
+ pxpt->prev_state_wall = now;
+ pxpt->prev_idle_wall = total_idle_ns;
+}
+
+void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to)
+{
+ struct pm_px *pxpt;
+ const struct processor_pminfo *pmpt = processor_pminfo[cpu];
+ spinlock_t *cpufreq_statistic_lock =
+ &per_cpu(cpufreq_statistic_lock, cpu);
+
+ spin_lock(cpufreq_statistic_lock);
+
+ pxpt = per_cpu(cpufreq_statistic_data, cpu);
+ if ( !pxpt || !pmpt ) {
+ spin_unlock(cpufreq_statistic_lock);
+ return;
+ }
+
+ pxpt->u.last = from;
+ pxpt->u.cur = to;
+ pxpt->u.pt[to].count++;
+
+ cpufreq_residency_update(cpu, from);
+
+ (*(pxpt->u.trans_pt + from * pmpt->perf.state_count + to))++;
+
+ spin_unlock(cpufreq_statistic_lock);
+}
+
+int cpufreq_statistic_init(unsigned int cpu)
+{
+ uint32_t i, count;
+ struct pm_px *pxpt;
+ const struct processor_pminfo *pmpt = processor_pminfo[cpu];
+ spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
+
+ spin_lock_init(cpufreq_statistic_lock);
+
+ if ( !pmpt )
+ return -EINVAL;
+
+ spin_lock(cpufreq_statistic_lock);
+
+ pxpt = per_cpu(cpufreq_statistic_data, cpu);
+ if ( pxpt )
+ {
+ spin_unlock(cpufreq_statistic_lock);
+ return 0;
+ }
+
+ count = pmpt->perf.state_count;
+
+ pxpt = xzalloc(struct pm_px);
+ if ( !pxpt )
+ {
+ spin_unlock(cpufreq_statistic_lock);
+ return -ENOMEM;
+ }
+ per_cpu(cpufreq_statistic_data, cpu) = pxpt;
+
+ pxpt->u.trans_pt = xzalloc_array(uint64_t, count * count);
+ if ( !pxpt->u.trans_pt )
+ {
+ xfree(pxpt);
+ spin_unlock(cpufreq_statistic_lock);
+ return -ENOMEM;
+ }
+
+ pxpt->u.pt = xzalloc_array(struct pm_px_val, count);
+ if ( !pxpt->u.pt )
+ {
+ xfree(pxpt->u.trans_pt);
+ xfree(pxpt);
+ spin_unlock(cpufreq_statistic_lock);
+ return -ENOMEM;
+ }
+
+ pxpt->u.total = pmpt->perf.state_count;
+ pxpt->u.usable = pmpt->perf.state_count - pmpt->perf.platform_limit;
+
+ for ( i = 0; i < pmpt->perf.state_count; i++ )
+ pxpt->u.pt[i].freq = pmpt->perf.states[i].core_frequency;
+
+ pxpt->prev_state_wall = NOW();
+ pxpt->prev_idle_wall = get_cpu_idle_time(cpu);
+
+ spin_unlock(cpufreq_statistic_lock);
+
+ return 0;
+}
+
+void cpufreq_statistic_exit(unsigned int cpu)
+{
+ struct pm_px *pxpt;
+ spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
+
+ spin_lock(cpufreq_statistic_lock);
+
+ pxpt = per_cpu(cpufreq_statistic_data, cpu);
+ if ( !pxpt )
+ {
+ spin_unlock(cpufreq_statistic_lock);
+ return;
+ }
+
+ xfree(pxpt->u.trans_pt);
+ xfree(pxpt->u.pt);
+ xfree(pxpt);
+ per_cpu(cpufreq_statistic_data, cpu) = NULL;
+
+ spin_unlock(cpufreq_statistic_lock);
+}
+
+static void cpufreq_statistic_reset(unsigned int cpu)
+{
+ uint32_t i, j, count;
+ struct pm_px *pxpt;
+ const struct processor_pminfo *pmpt = processor_pminfo[cpu];
+ spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
+
+ spin_lock(cpufreq_statistic_lock);
+
+ pxpt = per_cpu(cpufreq_statistic_data, cpu);
+ if ( !pmpt || !pxpt || !pxpt->u.pt || !pxpt->u.trans_pt )
+ {
+ spin_unlock(cpufreq_statistic_lock);
+ return;
+ }
+
+ count = pmpt->perf.state_count;
+
+ for ( i = 0; i < count; i++ )
+ {
+ pxpt->u.pt[i].residency = 0;
+ pxpt->u.pt[i].count = 0;
+
+ for ( j = 0; j < count; j++ )
+ *(pxpt->u.trans_pt + i * count + j) = 0;
+ }
+
+ pxpt->prev_state_wall = NOW();
+ pxpt->prev_idle_wall = get_cpu_idle_time(cpu);
+
+ spin_unlock(cpufreq_statistic_lock);
+}
+
/*
* Get PM statistic info
*/
@@ -522,34 +690,3 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
return ret;
}
-
-int acpi_set_pdc_bits(uint32_t acpi_id, XEN_GUEST_HANDLE(uint32) pdc)
-{
- u32 bits[3];
- int ret;
-
- if ( copy_from_guest(bits, pdc, 2) )
- ret = -EFAULT;
- else if ( bits[0] != ACPI_PDC_REVISION_ID || !bits[1] )
- ret = -EINVAL;
- else if ( copy_from_guest_offset(bits + 2, pdc, 2, 1) )
- ret = -EFAULT;
- else
- {
- u32 mask = 0;
-
- if ( xen_processor_pmbits & XEN_PROCESSOR_PM_CX )
- mask |= ACPI_PDC_C_MASK | ACPI_PDC_SMP_C1PT;
- if ( xen_processor_pmbits & XEN_PROCESSOR_PM_PX )
- mask |= ACPI_PDC_P_MASK | ACPI_PDC_SMP_C1PT;
- if ( xen_processor_pmbits & XEN_PROCESSOR_PM_TX )
- mask |= ACPI_PDC_T_MASK | ACPI_PDC_SMP_C1PT;
- bits[2] &= (ACPI_PDC_C_MASK | ACPI_PDC_P_MASK | ACPI_PDC_T_MASK |
- ACPI_PDC_SMP_C1PT) & ~mask;
- ret = arch_acpi_set_pdc_bits(acpi_id, bits, mask);
- }
- if ( !ret && __copy_to_guest_offset(pdc, 2, bits + 2, 1) )
- ret = -EFAULT;
-
- return ret;
-}
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 4a103c6de9..29fcfbe609 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -582,6 +582,37 @@ out:
return ret;
}
+int acpi_set_pdc_bits(uint32_t acpi_id, XEN_GUEST_HANDLE(uint32) pdc)
+{
+ u32 bits[3];
+ int ret;
+
+ if ( copy_from_guest(bits, pdc, 2) )
+ ret = -EFAULT;
+ else if ( bits[0] != ACPI_PDC_REVISION_ID || !bits[1] )
+ ret = -EINVAL;
+ else if ( copy_from_guest_offset(bits + 2, pdc, 2, 1) )
+ ret = -EFAULT;
+ else
+ {
+ u32 mask = 0;
+
+ if ( xen_processor_pmbits & XEN_PROCESSOR_PM_CX )
+ mask |= ACPI_PDC_C_MASK | ACPI_PDC_SMP_C1PT;
+ if ( xen_processor_pmbits & XEN_PROCESSOR_PM_PX )
+ mask |= ACPI_PDC_P_MASK | ACPI_PDC_SMP_C1PT;
+ if ( xen_processor_pmbits & XEN_PROCESSOR_PM_TX )
+ mask |= ACPI_PDC_T_MASK | ACPI_PDC_SMP_C1PT;
+ bits[2] &= (ACPI_PDC_C_MASK | ACPI_PDC_P_MASK | ACPI_PDC_T_MASK |
+ ACPI_PDC_SMP_C1PT) & ~mask;
+ ret = arch_acpi_set_pdc_bits(acpi_id, bits, mask);
+ }
+ if ( !ret && __copy_to_guest_offset(pdc, 2, bits + 2, 1) )
+ ret = -EFAULT;
+
+ return ret;
+}
+
static void cpufreq_cmdline_common_para(struct cpufreq_policy *new_policy)
{
if (usr_max_freq)
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index e690a484f1..723045b240 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -35,168 +35,6 @@ struct cpufreq_driver __read_mostly cpufreq_driver;
struct processor_pminfo *__read_mostly processor_pminfo[NR_CPUS];
DEFINE_PER_CPU_READ_MOSTLY(struct cpufreq_policy *, cpufreq_cpu_policy);
-DEFINE_PER_CPU(spinlock_t, cpufreq_statistic_lock);
-
-/*********************************************************************
- * Px STATISTIC INFO *
- *********************************************************************/
-
-void cpufreq_residency_update(unsigned int cpu, uint8_t state)
-{
- uint64_t now, total_idle_ns;
- int64_t delta;
- struct pm_px *pxpt = per_cpu(cpufreq_statistic_data, cpu);
-
- total_idle_ns = get_cpu_idle_time(cpu);
- now = NOW();
-
- delta = (now - pxpt->prev_state_wall) -
- (total_idle_ns - pxpt->prev_idle_wall);
-
- if ( likely(delta >= 0) )
- pxpt->u.pt[state].residency += delta;
-
- pxpt->prev_state_wall = now;
- pxpt->prev_idle_wall = total_idle_ns;
-}
-
-void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to)
-{
- struct pm_px *pxpt;
- struct processor_pminfo *pmpt = processor_pminfo[cpu];
- spinlock_t *cpufreq_statistic_lock =
- &per_cpu(cpufreq_statistic_lock, cpu);
-
- spin_lock(cpufreq_statistic_lock);
-
- pxpt = per_cpu(cpufreq_statistic_data, cpu);
- if ( !pxpt || !pmpt ) {
- spin_unlock(cpufreq_statistic_lock);
- return;
- }
-
- pxpt->u.last = from;
- pxpt->u.cur = to;
- pxpt->u.pt[to].count++;
-
- cpufreq_residency_update(cpu, from);
-
- (*(pxpt->u.trans_pt + from * pmpt->perf.state_count + to))++;
-
- spin_unlock(cpufreq_statistic_lock);
-}
-
-int cpufreq_statistic_init(unsigned int cpu)
-{
- uint32_t i, count;
- struct pm_px *pxpt;
- const struct processor_pminfo *pmpt = processor_pminfo[cpu];
- spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
-
- spin_lock_init(cpufreq_statistic_lock);
-
- if ( !pmpt )
- return -EINVAL;
-
- spin_lock(cpufreq_statistic_lock);
-
- pxpt = per_cpu(cpufreq_statistic_data, cpu);
- if ( pxpt ) {
- spin_unlock(cpufreq_statistic_lock);
- return 0;
- }
-
- count = pmpt->perf.state_count;
-
- pxpt = xzalloc(struct pm_px);
- if ( !pxpt ) {
- spin_unlock(cpufreq_statistic_lock);
- return -ENOMEM;
- }
- per_cpu(cpufreq_statistic_data, cpu) = pxpt;
-
- pxpt->u.trans_pt = xzalloc_array(uint64_t, count * count);
- if (!pxpt->u.trans_pt) {
- xfree(pxpt);
- spin_unlock(cpufreq_statistic_lock);
- return -ENOMEM;
- }
-
- pxpt->u.pt = xzalloc_array(struct pm_px_val, count);
- if (!pxpt->u.pt) {
- xfree(pxpt->u.trans_pt);
- xfree(pxpt);
- spin_unlock(cpufreq_statistic_lock);
- return -ENOMEM;
- }
-
- pxpt->u.total = pmpt->perf.state_count;
- pxpt->u.usable = pmpt->perf.state_count - pmpt->perf.platform_limit;
-
- for (i=0; i < pmpt->perf.state_count; i++)
- pxpt->u.pt[i].freq = pmpt->perf.states[i].core_frequency;
-
- pxpt->prev_state_wall = NOW();
- pxpt->prev_idle_wall = get_cpu_idle_time(cpu);
-
- spin_unlock(cpufreq_statistic_lock);
-
- return 0;
-}
-
-void cpufreq_statistic_exit(unsigned int cpu)
-{
- struct pm_px *pxpt;
- spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
-
- spin_lock(cpufreq_statistic_lock);
-
- pxpt = per_cpu(cpufreq_statistic_data, cpu);
- if (!pxpt) {
- spin_unlock(cpufreq_statistic_lock);
- return;
- }
-
- xfree(pxpt->u.trans_pt);
- xfree(pxpt->u.pt);
- xfree(pxpt);
- per_cpu(cpufreq_statistic_data, cpu) = NULL;
-
- spin_unlock(cpufreq_statistic_lock);
-}
-
-void cpufreq_statistic_reset(unsigned int cpu)
-{
- uint32_t i, j, count;
- struct pm_px *pxpt;
- const struct processor_pminfo *pmpt = processor_pminfo[cpu];
- spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
-
- spin_lock(cpufreq_statistic_lock);
-
- pxpt = per_cpu(cpufreq_statistic_data, cpu);
- if ( !pmpt || !pxpt || !pxpt->u.pt || !pxpt->u.trans_pt ) {
- spin_unlock(cpufreq_statistic_lock);
- return;
- }
-
- count = pmpt->perf.state_count;
-
- for (i=0; i < count; i++) {
- pxpt->u.pt[i].residency = 0;
- pxpt->u.pt[i].count = 0;
-
- for (j=0; j < count; j++)
- *(pxpt->u.trans_pt + i*count + j) = 0;
- }
-
- pxpt->prev_state_wall = NOW();
- pxpt->prev_idle_wall = get_cpu_idle_time(cpu);
-
- spin_unlock(cpufreq_statistic_lock);
-}
-
-
/*********************************************************************
* FREQUENCY TABLE HELPERS *
*********************************************************************/
diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/acpi/cpufreq/processor_perf.h
index 301104e16f..941e177b6d 100644
--- a/xen/include/acpi/cpufreq/processor_perf.h
+++ b/xen/include/acpi/cpufreq/processor_perf.h
@@ -9,11 +9,9 @@
unsigned int powernow_register_driver(void);
unsigned int get_measured_perf(unsigned int cpu, unsigned int flag);
-void cpufreq_residency_update(unsigned int cpu, uint8_t state);
void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to);
int cpufreq_statistic_init(unsigned int cpu);
void cpufreq_statistic_exit(unsigned int cpu);
-void cpufreq_statistic_reset(unsigned int cpu);
int cpufreq_limit_change(unsigned int cpu);
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 09/19] xen/pmstat: clean up pmstat.c
2025-03-26 5:50 ` [PATCH v2 09/19] xen/pmstat: clean up pmstat.c Penny Zheng
@ 2025-03-29 0:41 ` Stefano Stabellini
2025-04-01 12:55 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:41 UTC (permalink / raw)
To: Penny Zheng; +Cc: xen-devel, ray.huang, Jan Beulich
On Wed, 26 Mar 2025, Penny Zheng wrote:
> We intend to move the following functions into drivers/acpi/pmstat.c, as they
> are all designed for performance statistic:
> - cpufreq_residency_update
> - cpufreq_statistic_reset
> - cpufreq_statistic_update
> - cpufreq_statistic_init
> - cpufreq_statistic_exit
> and moving out acpi_set_pdc_bits(), as it is the handler for sub-hypercall
> XEN_PM_PDC, and shall stay with the other handlers together in
> drivers/cpufreq/cpufreq.c.
> This commit also applies various style corrections while moving these
> functions
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
I checked the diff and everything checks out.
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 09/19] xen/pmstat: clean up pmstat.c
2025-03-26 5:50 ` [PATCH v2 09/19] xen/pmstat: clean up pmstat.c Penny Zheng
2025-03-29 0:41 ` Stefano Stabellini
@ 2025-04-01 12:55 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 12:55 UTC (permalink / raw)
To: Penny Zheng; +Cc: ray.huang, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> We intend to move the following functions into drivers/acpi/pmstat.c, as they
> are all designed for performance statistic:
"We intend to ..." describes future plans. Yet this is what you're doing in this
very patch.
> - cpufreq_residency_update
> - cpufreq_statistic_reset
> - cpufreq_statistic_update
> - cpufreq_statistic_init
> - cpufreq_statistic_exit
> and moving out acpi_set_pdc_bits(), as it is the handler for sub-hypercall
> XEN_PM_PDC, and shall stay with the other handlers together in
> drivers/cpufreq/cpufreq.c.
> This commit also applies various style corrections while moving these
> functions
Nit - I'd like to remind you of how to (not) word commit messages.
As to what the sentence says - you staying vague leaves unclear which
style violations may have been left in place, for perhaps a good
reason. For example I observe u32 in code being moved.
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - new commit
> ---
> xen/drivers/acpi/pmstat.c | 199 ++++++++++++++++++----
> xen/drivers/cpufreq/cpufreq.c | 31 ++++
> xen/drivers/cpufreq/utility.c | 162 ------------------
> xen/include/acpi/cpufreq/processor_perf.h | 2 -
> 4 files changed, 199 insertions(+), 195 deletions(-)
Nit: Considering that code is being moved _into_ pmstat.c, "clean up"
in the subject is somewhat misleading. Maybe "consolidate code into
pmstat.c"?
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -43,6 +43,174 @@
>
> DEFINE_PER_CPU_READ_MOSTLY(struct pm_px *, cpufreq_statistic_data);
>
> +DEFINE_PER_CPU(spinlock_t, cpufreq_statistic_lock);
If you really move everything that's statistics-related, then both of
the above items ought to be possible to become static now.
> +/*********************************************************************
> + * Px STATISTIC INFO *
> + *********************************************************************/
> +
> +static void cpufreq_residency_update(unsigned int cpu, uint8_t state)
> +{
> + uint64_t now, total_idle_ns;
> + int64_t delta;
> + struct pm_px *pxpt = per_cpu(cpufreq_statistic_data, cpu);
> +
> + total_idle_ns = get_cpu_idle_time(cpu);
> + now = NOW();
> +
> + delta = (now - pxpt->prev_state_wall) -
> + (total_idle_ns - pxpt->prev_idle_wall);
> +
> + if ( likely(delta >= 0) )
> + pxpt->u.pt[state].residency += delta;
> +
> + pxpt->prev_state_wall = now;
> + pxpt->prev_idle_wall = total_idle_ns;
> +}
> +
> +void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to)
> +{
> + struct pm_px *pxpt;
> + const struct processor_pminfo *pmpt = processor_pminfo[cpu];
> + spinlock_t *cpufreq_statistic_lock =
> + &per_cpu(cpufreq_statistic_lock, cpu);
> +
> + spin_lock(cpufreq_statistic_lock);
> +
> + pxpt = per_cpu(cpufreq_statistic_data, cpu);
> + if ( !pxpt || !pmpt ) {
Hmm, you said style corrections are being made, yet the brace here is
misplaced.
> + spin_unlock(cpufreq_statistic_lock);
> + return;
> + }
> +
> + pxpt->u.last = from;
> + pxpt->u.cur = to;
> + pxpt->u.pt[to].count++;
> +
> + cpufreq_residency_update(cpu, from);
> +
> + (*(pxpt->u.trans_pt + from * pmpt->perf.state_count + to))++;
I came across this line the other day, iirc when reviewing you other
series. I find it expremely odd that this doesn't use array notation:
pxpt->u.trans_pt[from * pmpt->perf.state_count + to]++;
Could you please switch to that, unless of course you see an issue with
it?
> + spin_unlock(cpufreq_statistic_lock);
> +}
> +
> +int cpufreq_statistic_init(unsigned int cpu)
> +{
> + uint32_t i, count;
Here any elsewhere - converting to unsigned int would also fall under
style corrections.
> +static void cpufreq_statistic_reset(unsigned int cpu)
> +{
> + uint32_t i, j, count;
> + struct pm_px *pxpt;
> + const struct processor_pminfo *pmpt = processor_pminfo[cpu];
> + spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock, cpu);
> +
> + spin_lock(cpufreq_statistic_lock);
> +
> + pxpt = per_cpu(cpufreq_statistic_data, cpu);
> + if ( !pmpt || !pxpt || !pxpt->u.pt || !pxpt->u.trans_pt )
> + {
> + spin_unlock(cpufreq_statistic_lock);
> + return;
> + }
> +
> + count = pmpt->perf.state_count;
> +
> + for ( i = 0; i < count; i++ )
> + {
> + pxpt->u.pt[i].residency = 0;
> + pxpt->u.pt[i].count = 0;
> +
> + for ( j = 0; j < count; j++ )
> + *(pxpt->u.trans_pt + i * count + j) = 0;
Preferrably array notation again, please.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (8 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 09/19] xen/pmstat: clean up pmstat.c Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:35 ` Stefano Stabellini
2025-04-01 13:09 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 11/19] xen/sysctl: wrap around XEN_SYSCTL_page_offline_op Penny Zheng
` (8 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
We intend to introduce CONFIG_PM_STATS for wrapping all operations
regarding performance management statistics.
The major codes reside in xen/drivers/acpi/pmstat.c, including two main
pm-related sysctl op: do_get_pm_info() and do_pm_op().
So This commit also makes CONFIG_PM_STATS depend on CONFIG_SYSCTL
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- rename to CONFIG_PM_STATS
- fix indention and stray semicolon
- make code movements into a new commit
- No need to wrap inline functions and declarations
---
xen/arch/x86/acpi/cpu_idle.c | 2 ++
xen/arch/x86/acpi/cpufreq/hwp.c | 6 ++++++
xen/arch/x86/acpi/cpufreq/powernow.c | 4 ++++
xen/common/Kconfig | 5 +++++
xen/common/sysctl.c | 4 ++--
xen/drivers/acpi/Makefile | 2 +-
xen/drivers/cpufreq/cpufreq_misc_governors.c | 2 ++
xen/drivers/cpufreq/cpufreq_ondemand.c | 2 ++
xen/include/acpi/cpufreq/processor_perf.h | 14 ++++++++++++++
9 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 420198406d..b537ac4cd6 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -1487,6 +1487,7 @@ static void amd_cpuidle_init(struct acpi_processor_power *power)
vendor_override = -1;
}
+#ifdef CONFIG_PM_STATS
uint32_t pmstat_get_cx_nr(unsigned int cpu)
{
return processor_powers[cpu] ? processor_powers[cpu]->count : 0;
@@ -1606,6 +1607,7 @@ int pmstat_reset_cx_stat(unsigned int cpu)
{
return 0;
}
+#endif /* CONFIG_PM_STATS */
void cpuidle_disable_deep_cstate(void)
{
diff --git a/xen/arch/x86/acpi/cpufreq/hwp.c b/xen/arch/x86/acpi/cpufreq/hwp.c
index d5fa3d47ca..98e9d46890 100644
--- a/xen/arch/x86/acpi/cpufreq/hwp.c
+++ b/xen/arch/x86/acpi/cpufreq/hwp.c
@@ -466,6 +466,7 @@ static int cf_check hwp_cpufreq_cpu_exit(struct cpufreq_policy *policy)
return 0;
}
+#ifdef CONFIG_PM_STATS
/*
* The SDM reads like turbo should be disabled with MSR_IA32_PERF_CTL and
* PERF_CTL_TURBO_DISENGAGE, but that does not seem to actually work, at least
@@ -508,6 +509,7 @@ static int cf_check hwp_cpufreq_update(unsigned int cpu, struct cpufreq_policy *
return per_cpu(hwp_drv_data, cpu)->ret;
}
+#endif /* CONFIG_PM_STATS */
static const struct cpufreq_driver __initconst_cf_clobber
hwp_cpufreq_driver = {
@@ -516,9 +518,12 @@ hwp_cpufreq_driver = {
.target = hwp_cpufreq_target,
.init = hwp_cpufreq_cpu_init,
.exit = hwp_cpufreq_cpu_exit,
+#ifdef CONFIG_PM_STATS
.update = hwp_cpufreq_update,
+#endif
};
+#ifdef CONFIG_PM_STATS
int get_hwp_para(unsigned int cpu,
struct xen_cppc_para *cppc_para)
{
@@ -639,6 +644,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
return hwp_cpufreq_target(policy, 0, 0);
}
+#endif /* CONFIG_PM_STATS */
int __init hwp_register_driver(void)
{
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index 69364e1855..08494d0902 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -49,6 +49,7 @@ static void cf_check transition_pstate(void *pstate)
wrmsrl(MSR_PSTATE_CTRL, *(unsigned int *)pstate);
}
+#ifdef CONFIG_PM_STATS
static void cf_check update_cpb(void *data)
{
struct cpufreq_policy *policy = data;
@@ -77,6 +78,7 @@ static int cf_check powernow_cpufreq_update(
return 0;
}
+#endif /* CONFIG_PM_STATS */
static int cf_check powernow_cpufreq_target(
struct cpufreq_policy *policy,
@@ -324,7 +326,9 @@ powernow_cpufreq_driver = {
.target = powernow_cpufreq_target,
.init = powernow_cpufreq_cpu_init,
.exit = powernow_cpufreq_cpu_exit,
+#ifdef CONFIG_PM_STATS
.update = powernow_cpufreq_update
+#endif
};
unsigned int __init powernow_register_driver(void)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 5b55ca6eaa..426fa8fcc2 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -557,4 +557,9 @@ config SYSCTL
to reduce Xen footprint.
endmenu
+config PM_STATS
+ bool "Enable Performance Management Statistics"
+ depends on ACPI && HAS_CPUFREQ && SYSCTL
+ default y
+
endmenu
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 4602d84203..c17f1b3dec 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -176,7 +176,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
op->u.availheap.avail_bytes <<= PAGE_SHIFT;
break;
-#if defined (CONFIG_ACPI) && defined (CONFIG_HAS_CPUFREQ)
+#ifdef CONFIG_PM_STATS
case XEN_SYSCTL_get_pmstat:
ret = do_get_pm_info(&op->u.get_pmstat);
break;
@@ -186,7 +186,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
if ( ret == -EAGAIN )
copyback = 1;
break;
-#endif
+#endif /* CONFIG_PM_STATS */
case XEN_SYSCTL_page_offline_op:
{
diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
index 2fc5230253..a32a505e30 100644
--- a/xen/drivers/acpi/Makefile
+++ b/xen/drivers/acpi/Makefile
@@ -5,7 +5,7 @@ obj-$(CONFIG_X86) += apei/
obj-bin-y += tables.init.o
obj-$(CONFIG_ACPI_NUMA) += numa.o
obj-y += osl.o
-obj-$(CONFIG_HAS_CPUFREQ) += pmstat.o
+obj-$(CONFIG_PM_STATS) += pmstat.o
obj-$(CONFIG_X86) += hwregs.o
obj-$(CONFIG_X86) += reboot.o
diff --git a/xen/drivers/cpufreq/cpufreq_misc_governors.c b/xen/drivers/cpufreq/cpufreq_misc_governors.c
index 0327fad23b..c3877e9f0a 100644
--- a/xen/drivers/cpufreq/cpufreq_misc_governors.c
+++ b/xen/drivers/cpufreq/cpufreq_misc_governors.c
@@ -64,6 +64,7 @@ static int cf_check cpufreq_governor_userspace(
return ret;
}
+#ifdef CONFIG_PM_STATS
int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq)
{
struct cpufreq_policy *policy;
@@ -80,6 +81,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq)
return __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L);
}
+#endif /* CONFIG_PM_STATS */
static bool __init cf_check
cpufreq_userspace_handle_option(const char *name, const char *val)
diff --git a/xen/drivers/cpufreq/cpufreq_ondemand.c b/xen/drivers/cpufreq/cpufreq_ondemand.c
index 06cfc88d30..de10e03ca4 100644
--- a/xen/drivers/cpufreq/cpufreq_ondemand.c
+++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
@@ -57,6 +57,7 @@ static struct dbs_tuners {
static DEFINE_PER_CPU(struct timer, dbs_timer);
+#ifdef CONFIG_PM_STATS
int write_ondemand_sampling_rate(unsigned int sampling_rate)
{
if ( (sampling_rate > MAX_SAMPLING_RATE / MICROSECS(1)) ||
@@ -93,6 +94,7 @@ int get_cpufreq_ondemand_para(uint32_t *sampling_rate_max,
return 0;
}
+#endif /* CONFIG_PM_STATS */
static void dbs_check_cpu(struct cpu_dbs_info_s *this_dbs_info)
{
diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/acpi/cpufreq/processor_perf.h
index 941e177b6d..b4b3ed7964 100644
--- a/xen/include/acpi/cpufreq/processor_perf.h
+++ b/xen/include/acpi/cpufreq/processor_perf.h
@@ -9,9 +9,23 @@
unsigned int powernow_register_driver(void);
unsigned int get_measured_perf(unsigned int cpu, unsigned int flag);
+#ifdef CONFIG_PM_STATS
void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to);
int cpufreq_statistic_init(unsigned int cpu);
void cpufreq_statistic_exit(unsigned int cpu);
+#else
+static inline void cpufreq_statistic_update(unsigned int cpu, uint8_t from,
+ uint8_t to)
+{
+}
+static inline int cpufreq_statistic_init(unsigned int cpu)
+{
+ return 0;
+}
+static inline void cpufreq_statistic_exit(unsigned int cpu)
+{
+}
+#endif /* CONFIG_PM_STATS */
int cpufreq_limit_change(unsigned int cpu);
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-03-26 5:50 ` [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS Penny Zheng
@ 2025-03-29 0:35 ` Stefano Stabellini
2025-04-01 13:09 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:35 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
On Wed, 26 Mar 2025, Penny Zheng wrote:
> We intend to introduce CONFIG_PM_STATS for wrapping all operations
> regarding performance management statistics.
> The major codes reside in xen/drivers/acpi/pmstat.c, including two main
> pm-related sysctl op: do_get_pm_info() and do_pm_op().
> So This commit also makes CONFIG_PM_STATS depend on CONFIG_SYSCTL
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - rename to CONFIG_PM_STATS
> - fix indention and stray semicolon
> - make code movements into a new commit
> - No need to wrap inline functions and declarations
> ---
> xen/arch/x86/acpi/cpu_idle.c | 2 ++
> xen/arch/x86/acpi/cpufreq/hwp.c | 6 ++++++
> xen/arch/x86/acpi/cpufreq/powernow.c | 4 ++++
> xen/common/Kconfig | 5 +++++
> xen/common/sysctl.c | 4 ++--
> xen/drivers/acpi/Makefile | 2 +-
> xen/drivers/cpufreq/cpufreq_misc_governors.c | 2 ++
> xen/drivers/cpufreq/cpufreq_ondemand.c | 2 ++
> xen/include/acpi/cpufreq/processor_perf.h | 14 ++++++++++++++
> 9 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index 420198406d..b537ac4cd6 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -1487,6 +1487,7 @@ static void amd_cpuidle_init(struct acpi_processor_power *power)
> vendor_override = -1;
> }
>
> +#ifdef CONFIG_PM_STATS
> uint32_t pmstat_get_cx_nr(unsigned int cpu)
> {
> return processor_powers[cpu] ? processor_powers[cpu]->count : 0;
> @@ -1606,6 +1607,7 @@ int pmstat_reset_cx_stat(unsigned int cpu)
> {
> return 0;
> }
> +#endif /* CONFIG_PM_STATS */
>
> void cpuidle_disable_deep_cstate(void)
> {
> diff --git a/xen/arch/x86/acpi/cpufreq/hwp.c b/xen/arch/x86/acpi/cpufreq/hwp.c
> index d5fa3d47ca..98e9d46890 100644
> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -466,6 +466,7 @@ static int cf_check hwp_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> return 0;
> }
>
> +#ifdef CONFIG_PM_STATS
> /*
> * The SDM reads like turbo should be disabled with MSR_IA32_PERF_CTL and
> * PERF_CTL_TURBO_DISENGAGE, but that does not seem to actually work, at least
> @@ -508,6 +509,7 @@ static int cf_check hwp_cpufreq_update(unsigned int cpu, struct cpufreq_policy *
>
> return per_cpu(hwp_drv_data, cpu)->ret;
> }
> +#endif /* CONFIG_PM_STATS */
>
> static const struct cpufreq_driver __initconst_cf_clobber
> hwp_cpufreq_driver = {
> @@ -516,9 +518,12 @@ hwp_cpufreq_driver = {
> .target = hwp_cpufreq_target,
> .init = hwp_cpufreq_cpu_init,
> .exit = hwp_cpufreq_cpu_exit,
> +#ifdef CONFIG_PM_STATS
> .update = hwp_cpufreq_update,
> +#endif
> };
>
> +#ifdef CONFIG_PM_STATS
> int get_hwp_para(unsigned int cpu,
> struct xen_cppc_para *cppc_para)
> {
> @@ -639,6 +644,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
>
> return hwp_cpufreq_target(policy, 0, 0);
> }
> +#endif /* CONFIG_PM_STATS */
>
> int __init hwp_register_driver(void)
> {
> diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
> index 69364e1855..08494d0902 100644
> --- a/xen/arch/x86/acpi/cpufreq/powernow.c
> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c
> @@ -49,6 +49,7 @@ static void cf_check transition_pstate(void *pstate)
> wrmsrl(MSR_PSTATE_CTRL, *(unsigned int *)pstate);
> }
>
> +#ifdef CONFIG_PM_STATS
> static void cf_check update_cpb(void *data)
> {
> struct cpufreq_policy *policy = data;
> @@ -77,6 +78,7 @@ static int cf_check powernow_cpufreq_update(
>
> return 0;
> }
> +#endif /* CONFIG_PM_STATS */
>
> static int cf_check powernow_cpufreq_target(
> struct cpufreq_policy *policy,
> @@ -324,7 +326,9 @@ powernow_cpufreq_driver = {
> .target = powernow_cpufreq_target,
> .init = powernow_cpufreq_cpu_init,
> .exit = powernow_cpufreq_cpu_exit,
> +#ifdef CONFIG_PM_STATS
> .update = powernow_cpufreq_update
> +#endif
> };
>
> unsigned int __init powernow_register_driver(void)
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 5b55ca6eaa..426fa8fcc2 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -557,4 +557,9 @@ config SYSCTL
> to reduce Xen footprint.
> endmenu
>
> +config PM_STATS
> + bool "Enable Performance Management Statistics"
> + depends on ACPI && HAS_CPUFREQ && SYSCTL
> + default y
This is visible so it would need a description, such as:
Enable collection of performance management statistics to aid in
analyzing and tuning power/performance characteristics of the system.
This option may introduce slight overhead due to additional tracking.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-03-26 5:50 ` [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS Penny Zheng
2025-03-29 0:35 ` Stefano Stabellini
@ 2025-04-01 13:09 ` Jan Beulich
2025-04-16 3:54 ` Penny, Zheng
1 sibling, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 13:09 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Andrew Cooper, Roger Pau Monné, Anthony PERARD,
Michal Orzel, Julien Grall, Stefano Stabellini, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> We intend to introduce CONFIG_PM_STATS for wrapping all operations
> regarding performance management statistics.
> The major codes reside in xen/drivers/acpi/pmstat.c, including two main
> pm-related sysctl op: do_get_pm_info() and do_pm_op().
> So This commit also makes CONFIG_PM_STATS depend on CONFIG_SYSCTL
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - rename to CONFIG_PM_STATS
> - fix indention and stray semicolon
> - make code movements into a new commit
> - No need to wrap inline functions and declarations
> ---
> xen/arch/x86/acpi/cpu_idle.c | 2 ++
> xen/arch/x86/acpi/cpufreq/hwp.c | 6 ++++++
> xen/arch/x86/acpi/cpufreq/powernow.c | 4 ++++
> xen/common/Kconfig | 5 +++++
> xen/common/sysctl.c | 4 ++--
> xen/drivers/acpi/Makefile | 2 +-
> xen/drivers/cpufreq/cpufreq_misc_governors.c | 2 ++
> xen/drivers/cpufreq/cpufreq_ondemand.c | 2 ++
> xen/include/acpi/cpufreq/processor_perf.h | 14 ++++++++++++++
> 9 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index 420198406d..b537ac4cd6 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -1487,6 +1487,7 @@ static void amd_cpuidle_init(struct acpi_processor_power *power)
> vendor_override = -1;
> }
>
> +#ifdef CONFIG_PM_STATS
> uint32_t pmstat_get_cx_nr(unsigned int cpu)
> {
> return processor_powers[cpu] ? processor_powers[cpu]->count : 0;
> @@ -1606,6 +1607,7 @@ int pmstat_reset_cx_stat(unsigned int cpu)
> {
> return 0;
> }
> +#endif /* CONFIG_PM_STATS */
>
> void cpuidle_disable_deep_cstate(void)
> {
> diff --git a/xen/arch/x86/acpi/cpufreq/hwp.c b/xen/arch/x86/acpi/cpufreq/hwp.c
> index d5fa3d47ca..98e9d46890 100644
> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -466,6 +466,7 @@ static int cf_check hwp_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> return 0;
> }
>
> +#ifdef CONFIG_PM_STATS
> /*
> * The SDM reads like turbo should be disabled with MSR_IA32_PERF_CTL and
> * PERF_CTL_TURBO_DISENGAGE, but that does not seem to actually work, at least
> @@ -508,6 +509,7 @@ static int cf_check hwp_cpufreq_update(unsigned int cpu, struct cpufreq_policy *
>
> return per_cpu(hwp_drv_data, cpu)->ret;
> }
> +#endif /* CONFIG_PM_STATS */
>
> static const struct cpufreq_driver __initconst_cf_clobber
> hwp_cpufreq_driver = {
> @@ -516,9 +518,12 @@ hwp_cpufreq_driver = {
> .target = hwp_cpufreq_target,
> .init = hwp_cpufreq_cpu_init,
> .exit = hwp_cpufreq_cpu_exit,
> +#ifdef CONFIG_PM_STATS
> .update = hwp_cpufreq_update,
> +#endif
> };
Something's wrong here: The .update hook is actually making changes, so is
definitely not (only) about statistics. Same for the powernow driver.
> +#ifdef CONFIG_PM_STATS
> int get_hwp_para(unsigned int cpu,
> struct xen_cppc_para *cppc_para)
> {
> @@ -639,6 +644,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
>
> return hwp_cpufreq_target(policy, 0, 0);
> }
> +#endif /* CONFIG_PM_STATS */
This also isn't about statistics, but about getting / setting parameters.
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -557,4 +557,9 @@ config SYSCTL
> to reduce Xen footprint.
> endmenu
>
> +config PM_STATS
> + bool "Enable Performance Management Statistics"
> + depends on ACPI && HAS_CPUFREQ && SYSCTL
> + default y
As per above - either name, prompt and the description that Stefano suggested
are wrong, or it is too much that is being covered by this new control.
> --- a/xen/drivers/cpufreq/cpufreq_misc_governors.c
> +++ b/xen/drivers/cpufreq/cpufreq_misc_governors.c
> @@ -64,6 +64,7 @@ static int cf_check cpufreq_governor_userspace(
> return ret;
> }
>
> +#ifdef CONFIG_PM_STATS
> int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq)
> {
> struct cpufreq_policy *policy;
> @@ -80,6 +81,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq)
>
> return __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L);
> }
> +#endif /* CONFIG_PM_STATS */
Here the name of the function also makes pretty clear that it isn't about
statistics.
> --- a/xen/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
> @@ -57,6 +57,7 @@ static struct dbs_tuners {
>
> static DEFINE_PER_CPU(struct timer, dbs_timer);
>
> +#ifdef CONFIG_PM_STATS
> int write_ondemand_sampling_rate(unsigned int sampling_rate)
> {
> if ( (sampling_rate > MAX_SAMPLING_RATE / MICROSECS(1)) ||
> @@ -93,6 +94,7 @@ int get_cpufreq_ondemand_para(uint32_t *sampling_rate_max,
>
> return 0;
> }
> +#endif /* CONFIG_PM_STATS */
Same for the ones here.
> --- a/xen/include/acpi/cpufreq/processor_perf.h
> +++ b/xen/include/acpi/cpufreq/processor_perf.h
> @@ -9,9 +9,23 @@
>
> unsigned int powernow_register_driver(void);
> unsigned int get_measured_perf(unsigned int cpu, unsigned int flag);
> +#ifdef CONFIG_PM_STATS
> void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to);
> int cpufreq_statistic_init(unsigned int cpu);
> void cpufreq_statistic_exit(unsigned int cpu);
> +#else
> +static inline void cpufreq_statistic_update(unsigned int cpu, uint8_t from,
> + uint8_t to)
> +{
> +}
This could do with both braces moved to the line with the closing parenthesis.
> +static inline int cpufreq_statistic_init(unsigned int cpu)
> +{
> + return 0;
> +}
> +static inline void cpufreq_statistic_exit(unsigned int cpu)
> +{
> +}
Same here.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread* RE: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-04-01 13:09 ` Jan Beulich
@ 2025-04-16 3:54 ` Penny, Zheng
2025-04-16 6:37 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Penny, Zheng @ 2025-04-16 3:54 UTC (permalink / raw)
To: Jan Beulich, xen-devel@lists.xenproject.org; +Cc: Stabellini, Stefano
[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 1, 2025 9:10 PM
> To: Penny, Zheng <penny.zheng@amd.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 v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
>
> On 26.03.2025 06:50, Penny Zheng wrote:
> > We intend to introduce CONFIG_PM_STATS for wrapping all operations
> > regarding performance management statistics.
> > The major codes reside in xen/drivers/acpi/pmstat.c, including two
> > main pm-related sysctl op: do_get_pm_info() and do_pm_op().
> > So This commit also makes CONFIG_PM_STATS depend on
> CONFIG_SYSCTL
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> > ---
> > v1 -> v2:
> > - rename to CONFIG_PM_STATS
> > - fix indention and stray semicolon
> > - make code movements into a new commit
> > - No need to wrap inline functions and declarations
> > ---
> > xen/arch/x86/acpi/cpu_idle.c | 2 ++
> > xen/arch/x86/acpi/cpufreq/hwp.c | 6 ++++++
> > xen/arch/x86/acpi/cpufreq/powernow.c | 4 ++++
> > xen/common/Kconfig | 5 +++++
> > xen/common/sysctl.c | 4 ++--
> > xen/drivers/acpi/Makefile | 2 +-
> > xen/drivers/cpufreq/cpufreq_misc_governors.c | 2 ++
> > xen/drivers/cpufreq/cpufreq_ondemand.c | 2 ++
> > xen/include/acpi/cpufreq/processor_perf.h | 14 ++++++++++++++
> > 9 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/acpi/cpu_idle.c
> > b/xen/arch/x86/acpi/cpu_idle.c index 420198406d..b537ac4cd6 100644
> > --- a/xen/arch/x86/acpi/cpu_idle.c
> > +++ b/xen/arch/x86/acpi/cpu_idle.c
> > @@ -1487,6 +1487,7 @@ static void amd_cpuidle_init(struct
> acpi_processor_power *power)
> > vendor_override = -1;
> > }
> >
> > +#ifdef CONFIG_PM_STATS
> > uint32_t pmstat_get_cx_nr(unsigned int cpu) {
> > return processor_powers[cpu] ? processor_powers[cpu]->count : 0;
> > @@ -1606,6 +1607,7 @@ int pmstat_reset_cx_stat(unsigned int cpu) {
> > return 0;
> > }
> > +#endif /* CONFIG_PM_STATS */
> >
> > void cpuidle_disable_deep_cstate(void) { diff --git
> > a/xen/arch/x86/acpi/cpufreq/hwp.c b/xen/arch/x86/acpi/cpufreq/hwp.c
> > index d5fa3d47ca..98e9d46890 100644
> > --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> > +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> > @@ -466,6 +466,7 @@ static int cf_check hwp_cpufreq_cpu_exit(struct
> cpufreq_policy *policy)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PM_STATS
> > /*
> > * The SDM reads like turbo should be disabled with MSR_IA32_PERF_CTL
> and
> > * PERF_CTL_TURBO_DISENGAGE, but that does not seem to actually work,
> > at least @@ -508,6 +509,7 @@ static int cf_check
> > hwp_cpufreq_update(unsigned int cpu, struct cpufreq_policy *
> >
> > return per_cpu(hwp_drv_data, cpu)->ret; }
> > +#endif /* CONFIG_PM_STATS */
> >
> > static const struct cpufreq_driver __initconst_cf_clobber
> > hwp_cpufreq_driver = { @@ -516,9 +518,12 @@ hwp_cpufreq_driver = {
> > .target = hwp_cpufreq_target,
> > .init = hwp_cpufreq_cpu_init,
> > .exit = hwp_cpufreq_cpu_exit,
> > +#ifdef CONFIG_PM_STATS
> > .update = hwp_cpufreq_update,
> > +#endif
> > };
>
> Something's wrong here: The .update hook is actually making changes, so is
> definitely not (only) about statistics. Same for the powernow driver.
>
> > +#ifdef CONFIG_PM_STATS
> > int get_hwp_para(unsigned int cpu,
> > struct xen_cppc_para *cppc_para) { @@ -639,6 +644,7
> > @@ int set_hwp_para(struct cpufreq_policy *policy,
> >
> > return hwp_cpufreq_target(policy, 0, 0); }
> > +#endif /* CONFIG_PM_STATS */
>
> This also isn't about statistics, but about getting / setting parameters.
>
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -557,4 +557,9 @@ config SYSCTL
> > to reduce Xen footprint.
> > endmenu
> >
> > +config PM_STATS
> > + bool "Enable Performance Management Statistics"
> > + depends on ACPI && HAS_CPUFREQ && SYSCTL
> > + default y
>
> As per above - either name, prompt and the description that Stefano suggested
> are wrong, or it is too much that is being covered by this new control.
>
We have two sysctl-op on performance, do_get_pm_info() and do_pm_op().
I think do_get_pm_info() is to collect PM statistic info, which could be wrapped with
CONFIG_PM_STATS, while maybe do_pm_op() is more focusing on performance tuning.
How about we introduce another Kconfig CONFIG_PM_TUNE to wrap do_pm_op()
and related helpers? I suggest to introduce a new file pmtune.c to contain.
Or any better suggestion?
>
> Jan
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-04-16 3:54 ` Penny, Zheng
@ 2025-04-16 6:37 ` Jan Beulich
2025-04-16 8:38 ` Penny, Zheng
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2025-04-16 6:37 UTC (permalink / raw)
To: Penny, Zheng; +Cc: Stabellini, Stefano, xen-devel@lists.xenproject.org
On 16.04.2025 05:54, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, April 1, 2025 9:10 PM
>>
>> On 26.03.2025 06:50, Penny Zheng wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -557,4 +557,9 @@ config SYSCTL
>>> to reduce Xen footprint.
>>> endmenu
>>>
>>> +config PM_STATS
>>> + bool "Enable Performance Management Statistics"
>>> + depends on ACPI && HAS_CPUFREQ && SYSCTL
>>> + default y
>>
>> As per above - either name, prompt and the description that Stefano suggested
>> are wrong, or it is too much that is being covered by this new control.
>>
>
> We have two sysctl-op on performance, do_get_pm_info() and do_pm_op().
> I think do_get_pm_info() is to collect PM statistic info, which could be wrapped with
> CONFIG_PM_STATS, while maybe do_pm_op() is more focusing on performance tuning.
> How about we introduce another Kconfig CONFIG_PM_TUNE to wrap do_pm_op()
> and related helpers? I suggest to introduce a new file pmtune.c to contain.
> Or any better suggestion?
"tune" is too narrow imo. "ctrl" may be an option, but how about simply pm-op.c,
fitting do_pm_op() pretty nicely? Question is what else is going to end up in
that file.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-04-16 6:37 ` Jan Beulich
@ 2025-04-16 8:38 ` Penny, Zheng
2025-04-16 8:45 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Penny, Zheng @ 2025-04-16 8:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: Stabellini, Stefano, xen-devel@lists.xenproject.org
[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, April 16, 2025 2:37 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Stabellini, Stefano <stefano.stabellini@amd.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
>
> On 16.04.2025 05:54, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Tuesday, April 1, 2025 9:10 PM
> >>
> >> On 26.03.2025 06:50, Penny Zheng wrote:
> >>> --- a/xen/common/Kconfig
> >>> +++ b/xen/common/Kconfig
> >>> @@ -557,4 +557,9 @@ config SYSCTL
> >>> to reduce Xen footprint.
> >>> endmenu
> >>>
> >>> +config PM_STATS
> >>> + bool "Enable Performance Management Statistics"
> >>> + depends on ACPI && HAS_CPUFREQ && SYSCTL
> >>> + default y
> >>
> >> As per above - either name, prompt and the description that Stefano
> >> suggested are wrong, or it is too much that is being covered by this new
> control.
> >>
> >
> > We have two sysctl-op on performance, do_get_pm_info() and do_pm_op().
> > I think do_get_pm_info() is to collect PM statistic info, which could
> > be wrapped with CONFIG_PM_STATS, while maybe do_pm_op() is more
> focusing on performance tuning.
> > How about we introduce another Kconfig CONFIG_PM_TUNE to wrap
> > do_pm_op() and related helpers? I suggest to introduce a new file pmtune.c to
> contain.
> > Or any better suggestion?
>
> "tune" is too narrow imo. "ctrl" may be an option, but how about simply pm-op.c,
> fitting do_pm_op() pretty nicely? Question is what else is going to end up in that
> file.
>
The following functions will be included in pm_op.c and wrapped with PM_CTRL or PM_OP
- get_cpufreq_para()
- set_cpufreq_para()
- set_cpufreq_gov()
- set_cpufreq_cppc()
- cpufreq_driver_getavg()
- cpufreq_update_turbo()
- cpufreq_get_turbo_status()
And description for PM_STATS, I'll take stefano's suggestion and for PM_CTRL/PM_OP, maybe it will be
```
Enable userspace performance management control to do power/performance analyzing and tuning
```
> Jan
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
2025-04-16 8:38 ` Penny, Zheng
@ 2025-04-16 8:45 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-16 8:45 UTC (permalink / raw)
To: Penny, Zheng; +Cc: Stabellini, Stefano, xen-devel@lists.xenproject.org
On 16.04.2025 10:38, Penny, Zheng wrote:
> [Public]
>
> Hi,
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, April 16, 2025 2:37 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Stabellini, Stefano <stefano.stabellini@amd.com>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
>>
>> On 16.04.2025 05:54, Penny, Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Tuesday, April 1, 2025 9:10 PM
>>>>
>>>> On 26.03.2025 06:50, Penny Zheng wrote:
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -557,4 +557,9 @@ config SYSCTL
>>>>> to reduce Xen footprint.
>>>>> endmenu
>>>>>
>>>>> +config PM_STATS
>>>>> + bool "Enable Performance Management Statistics"
>>>>> + depends on ACPI && HAS_CPUFREQ && SYSCTL
>>>>> + default y
>>>>
>>>> As per above - either name, prompt and the description that Stefano
>>>> suggested are wrong, or it is too much that is being covered by this new
>> control.
>>>>
>>>
>>> We have two sysctl-op on performance, do_get_pm_info() and do_pm_op().
>>> I think do_get_pm_info() is to collect PM statistic info, which could
>>> be wrapped with CONFIG_PM_STATS, while maybe do_pm_op() is more
>> focusing on performance tuning.
>>> How about we introduce another Kconfig CONFIG_PM_TUNE to wrap
>>> do_pm_op() and related helpers? I suggest to introduce a new file pmtune.c to
>> contain.
>>> Or any better suggestion?
>>
>> "tune" is too narrow imo. "ctrl" may be an option, but how about simply pm-op.c,
>> fitting do_pm_op() pretty nicely? Question is what else is going to end up in that
>> file.
>>
>
> The following functions will be included in pm_op.c
To avoid another round trip just for this - I wrote pm-op.c for a reason.
> and wrapped with PM_CTRL or PM_OP
> - get_cpufreq_para()
> - set_cpufreq_para()
> - set_cpufreq_gov()
> - set_cpufreq_cppc()
> - cpufreq_driver_getavg()
> - cpufreq_update_turbo()
> - cpufreq_get_turbo_status()
> And description for PM_STATS, I'll take stefano's suggestion and for PM_CTRL/PM_OP, maybe it will be
> ```
> Enable userspace performance management control to do power/performance analyzing and tuning
> ```
Reads okay at the first glance.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 11/19] xen/sysctl: wrap around XEN_SYSCTL_page_offline_op
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (9 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-26 5:50 ` [PATCH v2 12/19] xen/sysctl: wrap around XEN_SYSCTL_cpupool_op Penny Zheng
` (7 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Daniel P. Smith
The following functions are only to deal with XEN_SYSCTL_page_offline_op,
then shall be wrapped:
- xsm_page_offline
- online_page
- query_page_offline
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1 -> v2:
- add transient #ifdef in sysctl.c for correct compilation
- no need to wrap declarations
- place the #ifdef inside the function body to have less redundancy
---
xen/common/page_alloc.c | 2 ++
xen/common/sysctl.c | 2 ++
xen/include/xsm/xsm.h | 6 ++++++
xen/xsm/dummy.c | 2 ++
xen/xsm/flask/hooks.c | 6 ++++++
5 files changed, 18 insertions(+)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index bd4538c28d..cc2ad4423a 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1758,6 +1758,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
return 0;
}
+#ifdef CONFIG_SYSCTL
/*
* Online the memory.
* The caller should make sure end_pfn <= max_page,
@@ -1842,6 +1843,7 @@ int query_page_offline(mfn_t mfn, uint32_t *status)
return 0;
}
+#endif /* CONFIG_SYSCTL */
/*
* This function should only be called with valid pages from the same NUMA
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c17f1b3dec..3129fc602d 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -188,6 +188,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
break;
#endif /* CONFIG_PM_STATS */
+#ifdef CONFIG_SYSCTL
case XEN_SYSCTL_page_offline_op:
{
uint32_t *status, *ptr;
@@ -248,6 +249,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
copyback = 0;
}
break;
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_cpupool_op:
ret = cpupool_do_sysctl(&op->u.cpupool_op);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 042a99449f..5ac99904c4 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -138,7 +138,9 @@ struct xsm_ops {
int (*resource_setup_gsi)(int gsi);
int (*resource_setup_misc)(void);
+#ifdef CONFIG_SYSCTL
int (*page_offline)(uint32_t cmd);
+#endif
int (*hypfs_op)(void);
long (*do_xsm_op)(XEN_GUEST_HANDLE_PARAM(void) op);
@@ -597,7 +599,11 @@ static inline int xsm_resource_setup_misc(xsm_default_t def)
static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
{
+#ifdef CONFIG_SYSCTL
return alternative_call(xsm_ops.page_offline, cmd);
+#else
+ return -EOPNOTSUPP;
+#endif
}
static inline int xsm_hypfs_op(xsm_default_t def)
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index cd0e844fcf..d46413ad8c 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -96,7 +96,9 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
.resource_setup_gsi = xsm_resource_setup_gsi,
.resource_setup_misc = xsm_resource_setup_misc,
+#ifdef CONFIG_SYSCTL
.page_offline = xsm_page_offline,
+#endif
.hypfs_op = xsm_hypfs_op,
.hvm_param = xsm_hvm_param,
.hvm_param_altp2mhvm = xsm_hvm_param_altp2mhvm,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index df7e10775b..39f8331207 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1206,10 +1206,12 @@ static int cf_check flask_resource_unplug_core(void)
return avc_current_has_perm(SECINITSID_DOMXEN, SECCLASS_RESOURCE, RESOURCE__UNPLUG, NULL);
}
+#ifdef CONFIG_SYSCTL
static int flask_resource_use_core(void)
{
return avc_current_has_perm(SECINITSID_DOMXEN, SECCLASS_RESOURCE, RESOURCE__USE, NULL);
}
+#endif
static int cf_check flask_resource_plug_pci(uint32_t machine_bdf)
{
@@ -1274,6 +1276,7 @@ static int cf_check flask_resource_setup_misc(void)
return avc_current_has_perm(SECINITSID_XEN, SECCLASS_RESOURCE, RESOURCE__SETUP, NULL);
}
+#ifdef CONFIG_SYSCTL
static inline int cf_check flask_page_offline(uint32_t cmd)
{
switch ( cmd )
@@ -1288,6 +1291,7 @@ static inline int cf_check flask_page_offline(uint32_t cmd)
return avc_unknown_permission("page_offline", cmd);
}
}
+#endif
static inline int cf_check flask_hypfs_op(void)
{
@@ -1948,7 +1952,9 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
.resource_setup_gsi = flask_resource_setup_gsi,
.resource_setup_misc = flask_resource_setup_misc,
+#ifdef CONFIG_SYSCTL
.page_offline = flask_page_offline,
+#endif
.hypfs_op = flask_hypfs_op,
.hvm_param = flask_hvm_param,
.hvm_param_altp2mhvm = flask_hvm_param_altp2mhvm,
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 12/19] xen/sysctl: wrap around XEN_SYSCTL_cpupool_op
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (10 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 11/19] xen/sysctl: wrap around XEN_SYSCTL_page_offline_op Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-26 5:50 ` [PATCH v2 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op Penny Zheng
` (6 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Dario Faggioli, Juergen Gross,
George Dunlap, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
Function cpupool_do_sysctl is designed for doing cpupool related sysctl
operations, and shall be wrapped.
The following static functions are only called by cpupool_do_sysctl(), then
shall be wrapped too:
- cpupool_get_next_by_id
- cpupool_destroy
- cpupool_unassign_cpu_helper
- cpupool_unassign_cpu
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1 -> v2:
- no need to wrap declaration
- add transient #ifdef in sysctl.c for correct compilation
---
xen/common/sched/cpupool.c | 8 ++++++++
xen/common/sysctl.c | 2 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 3d02c7b706..f5459c2779 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -241,10 +241,12 @@ struct cpupool *cpupool_get_by_id(unsigned int poolid)
return __cpupool_get_by_id(poolid, true);
}
+#ifdef CONFIG_SYSCTL
static struct cpupool *cpupool_get_next_by_id(unsigned int poolid)
{
return __cpupool_get_by_id(poolid, false);
}
+#endif /* CONFIG_SYSCTL */
void cpupool_put(struct cpupool *pool)
{
@@ -352,6 +354,7 @@ static struct cpupool *cpupool_create(unsigned int poolid,
return ERR_PTR(ret);
}
+#ifdef CONFIG_SYSCTL
/*
* destroys the given cpupool
* returns 0 on success, 1 else
@@ -379,6 +382,7 @@ static int cpupool_destroy(struct cpupool *c)
debugtrace_printk("cpupool_destroy(pool=%u)\n", c->cpupool_id);
return 0;
}
+#endif /* CONFIG_SYSCTL */
/*
* Move domain to another cpupool
@@ -568,6 +572,7 @@ static int cpupool_unassign_cpu_start(struct cpupool *c, unsigned int cpu)
return ret;
}
+#ifdef CONFIG_SYSCTL
static long cf_check cpupool_unassign_cpu_helper(void *info)
{
struct cpupool *c = info;
@@ -633,6 +638,7 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
}
return continue_hypercall_on_cpu(work_cpu, cpupool_unassign_cpu_helper, c);
}
+#endif /* CONFIG_SYSCTL */
/*
* add a new domain to a cpupool
@@ -810,6 +816,7 @@ static void cpupool_cpu_remove_forced(unsigned int cpu)
rcu_read_unlock(&sched_res_rculock);
}
+#ifdef CONFIG_SYSCTL
/*
* do cpupool related sysctl operations
*/
@@ -975,6 +982,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
return ret;
}
+#endif /* CONFIG_SYSCTL */
unsigned int cpupool_get_id(const struct domain *d)
{
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 3129fc602d..0292233c7e 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -249,12 +249,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
copyback = 0;
}
break;
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_cpupool_op:
ret = cpupool_do_sysctl(&op->u.cpupool_op);
break;
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_scheduler_op:
ret = sched_adjust_global(&op->u.scheduler_op);
break;
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (11 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 12/19] xen/sysctl: wrap around XEN_SYSCTL_cpupool_op Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-26 15:22 ` Stewart Hildebrand
2025-03-26 5:50 ` [PATCH v2 14/19] xen: make avail_domheap_pages() static Penny Zheng
` (5 subsequent siblings)
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel, xen-devel
Cc: ray.huang, Penny Zheng, Nathan Studer, Stewart Hildebrand,
Dario Faggioli, Juergen Gross, George Dunlap, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini, Daniel P. Smith
Function sched_adjust_global is designed for XEN_SYSCTL_scheduler_op, so
itself and its calling flow, like .adjust_global, shall all be wrapped.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1 -> v2:
- no need to wrap declarations
- add transient #ifdef in sysctl.c for correct compilation
---
xen/common/sched/arinc653.c | 6 ++++++
xen/common/sched/core.c | 2 ++
xen/common/sched/credit.c | 4 ++++
xen/common/sched/credit2.c | 4 ++++
xen/common/sched/private.h | 4 ++++
xen/common/sysctl.c | 2 +-
xen/include/xsm/xsm.h | 4 ++++
xen/xsm/dummy.c | 2 ++
xen/xsm/flask/hooks.c | 4 ++++
9 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
index 432ccfe662..3c014c9934 100644
--- a/xen/common/sched/arinc653.c
+++ b/xen/common/sched/arinc653.c
@@ -220,6 +220,7 @@ static void update_schedule_units(const struct scheduler *ops)
SCHED_PRIV(ops)->schedule[i].unit_id);
}
+#ifdef CONFIG_SYSCTL
/**
* This function is called by the adjust_global scheduler hook to put
* in place a new ARINC653 schedule.
@@ -334,6 +335,7 @@ arinc653_sched_get(
return 0;
}
+#endif /* CONFIG_SYSCTL */
/**************************************************************************
* Scheduler callback functions *
@@ -653,6 +655,7 @@ a653_switch_sched(struct scheduler *new_ops, unsigned int cpu,
return &sr->_lock;
}
+#ifdef CONFIG_SYSCTL
/**
* Xen scheduler callback function to perform a global (not domain-specific)
* adjustment. It is used by the ARINC 653 scheduler to put in place a new
@@ -692,6 +695,7 @@ a653sched_adjust_global(const struct scheduler *ops,
return rc;
}
+#endif /* CONFIG_SYSCTL */
/**
* This structure defines our scheduler for Xen.
@@ -726,7 +730,9 @@ static const struct scheduler sched_arinc653_def = {
.switch_sched = a653_switch_sched,
.adjust = NULL,
+#ifdef CONFIG_SYSCTL
.adjust_global = a653sched_adjust_global,
+#endif
.dump_settings = NULL,
.dump_cpu_state = NULL,
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 13fdf57e57..ea95dea65a 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2112,6 +2112,7 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
return ret;
}
+#ifdef CONFIG_SYSCTL
long sched_adjust_global(struct xen_sysctl_scheduler_op *op)
{
struct cpupool *pool;
@@ -2140,6 +2141,7 @@ long sched_adjust_global(struct xen_sysctl_scheduler_op *op)
return rc;
}
+#endif /* CONFIG_SYSCTL */
static void vcpu_periodic_timer_work_locked(struct vcpu *v)
{
diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index a6bb321e7d..6dcf6b2c8b 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -1256,6 +1256,7 @@ __csched_set_tslice(struct csched_private *prv, unsigned int timeslice_ms)
prv->credit = prv->credits_per_tslice * prv->ncpus;
}
+#ifdef CONFIG_SYSCTL
static int cf_check
csched_sys_cntl(const struct scheduler *ops,
struct xen_sysctl_scheduler_op *sc)
@@ -1298,6 +1299,7 @@ csched_sys_cntl(const struct scheduler *ops,
out:
return rc;
}
+#endif /* CONFIG_SYSCTL */
static void *cf_check
csched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
@@ -2288,7 +2290,9 @@ static const struct scheduler sched_credit_def = {
.adjust = csched_dom_cntl,
.adjust_affinity= csched_aff_cntl,
+#ifdef CONFIG_SYSCTL
.adjust_global = csched_sys_cntl,
+#endif
.pick_resource = csched_res_pick,
.do_schedule = csched_schedule,
diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 0a83f23725..0b3b61df57 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -3131,6 +3131,7 @@ csched2_aff_cntl(const struct scheduler *ops, struct sched_unit *unit,
__clear_bit(__CSFLAG_pinned, &svc->flags);
}
+#ifdef CONFIG_SYSCTL
static int cf_check csched2_sys_cntl(
const struct scheduler *ops, struct xen_sysctl_scheduler_op *sc)
{
@@ -3162,6 +3163,7 @@ static int cf_check csched2_sys_cntl(
return 0;
}
+#endif /* CONFIG_SYSCTL */
static void *cf_check
csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
@@ -4232,7 +4234,9 @@ static const struct scheduler sched_credit2_def = {
.adjust = csched2_dom_cntl,
.adjust_affinity= csched2_aff_cntl,
+#ifdef CONFIG_SYSCTL
.adjust_global = csched2_sys_cntl,
+#endif
.pick_resource = csched2_res_pick,
.migrate = csched2_unit_migrate,
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index c0e7c96d24..d6884550cd 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -356,8 +356,10 @@ struct scheduler {
struct sched_unit *unit,
const struct cpumask *hard,
const struct cpumask *soft);
+#ifdef CONFIG_SYSCTL
int (*adjust_global) (const struct scheduler *ops,
struct xen_sysctl_scheduler_op *sc);
+#endif
void (*dump_settings) (const struct scheduler *ops);
void (*dump_cpu_state) (const struct scheduler *ops, int cpu);
void (*move_timers) (const struct scheduler *ops,
@@ -510,11 +512,13 @@ static inline int sched_adjust_dom(const struct scheduler *s, struct domain *d,
return s->adjust ? s->adjust(s, d, op) : 0;
}
+#ifdef CONFIG_SYSCTL
static inline int sched_adjust_cpupool(const struct scheduler *s,
struct xen_sysctl_scheduler_op *op)
{
return s->adjust_global ? s->adjust_global(s, op) : 0;
}
+#endif
static inline void sched_move_timers(const struct scheduler *s,
struct sched_resource *sr)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 0292233c7e..ccce7fe963 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -254,11 +254,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = cpupool_do_sysctl(&op->u.cpupool_op);
break;
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_scheduler_op:
ret = sched_adjust_global(&op->u.scheduler_op);
break;
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_physinfo:
{
struct xen_sysctl_physinfo *pi = &op->u.physinfo;
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 5ac99904c4..6e1789c314 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -57,7 +57,9 @@ struct xsm_ops {
int (*domain_create)(struct domain *d, uint32_t ssidref);
int (*getdomaininfo)(struct domain *d);
int (*domctl_scheduler_op)(struct domain *d, int op);
+#ifdef CONFIG_SYSCTL
int (*sysctl_scheduler_op)(int op);
+#endif
int (*set_target)(struct domain *d, struct domain *e);
int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
int (*sysctl)(int cmd);
@@ -244,10 +246,12 @@ static inline int xsm_domctl_scheduler_op(
return alternative_call(xsm_ops.domctl_scheduler_op, d, cmd);
}
+#ifdef CONFIG_SYSCTL
static inline int xsm_sysctl_scheduler_op(xsm_default_t def, int cmd)
{
return alternative_call(xsm_ops.sysctl_scheduler_op, cmd);
}
+#endif
static inline int xsm_set_target(
xsm_default_t def, struct domain *d, struct domain *e)
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index d46413ad8c..8d44f5bfb6 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -19,7 +19,9 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
.domain_create = xsm_domain_create,
.getdomaininfo = xsm_getdomaininfo,
.domctl_scheduler_op = xsm_domctl_scheduler_op,
+#ifdef CONFIG_SYSCTL
.sysctl_scheduler_op = xsm_sysctl_scheduler_op,
+#endif
.set_target = xsm_set_target,
.domctl = xsm_domctl,
#ifdef CONFIG_SYSCTL
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 39f8331207..64c9531346 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -626,6 +626,7 @@ static int cf_check flask_domctl_scheduler_op(struct domain *d, int op)
}
}
+#ifdef CONFIG_SYSCTL
static int cf_check flask_sysctl_scheduler_op(int op)
{
switch ( op )
@@ -640,6 +641,7 @@ static int cf_check flask_sysctl_scheduler_op(int op)
return avc_unknown_permission("sysctl_scheduler_op", op);
}
}
+#endif
static int cf_check flask_set_target(struct domain *d, struct domain *t)
{
@@ -1887,7 +1889,9 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
.domain_create = flask_domain_create,
.getdomaininfo = flask_getdomaininfo,
.domctl_scheduler_op = flask_domctl_scheduler_op,
+#ifdef CONFIG_SYSCTL
.sysctl_scheduler_op = flask_sysctl_scheduler_op,
+#endif
.set_target = flask_set_target,
.domctl = flask_domctl,
#ifdef CONFIG_SYSCTL
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op
2025-03-26 5:50 ` [PATCH v2 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op Penny Zheng
@ 2025-03-26 15:22 ` Stewart Hildebrand
0 siblings, 0 replies; 55+ messages in thread
From: Stewart Hildebrand @ 2025-03-26 15:22 UTC (permalink / raw)
To: Penny Zheng, xen-devel, xen-devel
Cc: ray.huang, Nathan Studer, Stewart Hildebrand, Dario Faggioli,
Juergen Gross, George Dunlap, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Daniel P. Smith
On 3/26/25 01:50, Penny Zheng wrote:
> Function sched_adjust_global is designed for XEN_SYSCTL_scheduler_op, so
> itself and its calling flow, like .adjust_global, shall all be wrapped.
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
For arinc653 only:
Acked-by: Stewart Hildebrand <stewart@stew.dk> #a653
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 14/19] xen: make avail_domheap_pages() static
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (12 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-29 0:11 ` Stefano Stabellini
2025-03-31 6:33 ` Jan Beulich
2025-03-26 5:50 ` [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo Penny Zheng
` (4 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
Function avail_domheap_pages() is only invoked by get_outstanding_claims(),
so it could be inlined into its sole caller.
Move up avail_heap_pages() to avoid declaration before
get_outstanding_claims().
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- let avail_domheap_pages() being inlined into its sole caller
- move up avail_heap_pages()
---
xen/common/page_alloc.c | 51 ++++++++++++++++++-----------------------
xen/include/xen/mm.h | 1 -
2 files changed, 22 insertions(+), 30 deletions(-)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index cc2ad4423a..5803a1ef4e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -488,6 +488,27 @@ static long total_avail_pages;
static DEFINE_SPINLOCK(heap_lock);
static long outstanding_claims; /* total outstanding claims by all domains */
+static unsigned long avail_heap_pages(
+ unsigned int zone_lo, unsigned int zone_hi, unsigned int node)
+{
+ unsigned int i, zone;
+ unsigned long free_pages = 0;
+
+ if ( zone_hi >= NR_ZONES )
+ zone_hi = NR_ZONES - 1;
+
+ for_each_online_node(i)
+ {
+ if ( !avail[i] )
+ continue;
+ for ( zone = zone_lo; zone <= zone_hi; zone++ )
+ if ( (node == -1) || (node == i) )
+ free_pages += avail[i][zone];
+ }
+
+ return free_pages;
+}
+
unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
{
ASSERT(rspin_is_locked(&d->page_alloc_lock));
@@ -584,7 +605,7 @@ void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
{
spin_lock(&heap_lock);
*outstanding_pages = outstanding_claims;
- *free_pages = avail_domheap_pages();
+ *free_pages = avail_heap_pages(MEMZONE_XEN + 1, NR_ZONES - 1, -1);
spin_unlock(&heap_lock);
}
@@ -1964,27 +1985,6 @@ static void init_heap_pages(
}
}
-static unsigned long avail_heap_pages(
- unsigned int zone_lo, unsigned int zone_hi, unsigned int node)
-{
- unsigned int i, zone;
- unsigned long free_pages = 0;
-
- if ( zone_hi >= NR_ZONES )
- zone_hi = NR_ZONES - 1;
-
- for_each_online_node(i)
- {
- if ( !avail[i] )
- continue;
- for ( zone = zone_lo; zone <= zone_hi; zone++ )
- if ( (node == -1) || (node == i) )
- free_pages += avail[i][zone];
- }
-
- return free_pages;
-}
-
/*************************
* COLORED SIDE-ALLOCATOR
*
@@ -2795,13 +2795,6 @@ unsigned long avail_domheap_pages_region(
return avail_heap_pages(zone_lo, zone_hi, node);
}
-unsigned long avail_domheap_pages(void)
-{
- return avail_heap_pages(MEMZONE_XEN + 1,
- NR_ZONES - 1,
- -1);
-}
-
unsigned long avail_node_heap_pages(unsigned int nodeid)
{
return avail_heap_pages(MEMZONE_XEN, NR_ZONES -1, nodeid);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index ae1c48a615..eda57486cf 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -140,7 +140,6 @@ struct page_info *alloc_domheap_pages(
void free_domheap_pages(struct page_info *pg, unsigned int order);
unsigned long avail_domheap_pages_region(
unsigned int node, unsigned int min_width, unsigned int max_width);
-unsigned long avail_domheap_pages(void);
unsigned long avail_node_heap_pages(unsigned int nodeid);
#define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
#define free_domheap_page(p) (free_domheap_pages(p,0))
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 14/19] xen: make avail_domheap_pages() static
2025-03-26 5:50 ` [PATCH v2 14/19] xen: make avail_domheap_pages() static Penny Zheng
@ 2025-03-29 0:11 ` Stefano Stabellini
2025-03-31 6:33 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:11 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On Wed, 26 Mar 2025, Penny Zheng wrote:
> Function avail_domheap_pages() is only invoked by get_outstanding_claims(),
> so it could be inlined into its sole caller.
> Move up avail_heap_pages() to avoid declaration before
> get_outstanding_claims().
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 14/19] xen: make avail_domheap_pages() static
2025-03-26 5:50 ` [PATCH v2 14/19] xen: make avail_domheap_pages() static Penny Zheng
2025-03-29 0:11 ` Stefano Stabellini
@ 2025-03-31 6:33 ` Jan Beulich
1 sibling, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-03-31 6:33 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> Function avail_domheap_pages() is only invoked by get_outstanding_claims(),
> so it could be inlined into its sole caller.
> Move up avail_heap_pages() to avoid declaration before
> get_outstanding_claims().
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - let avail_domheap_pages() being inlined into its sole caller
> - move up avail_heap_pages()
With which both title and description are stale now.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (13 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 14/19] xen: make avail_domheap_pages() static Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-27 9:35 ` Oleksii Kurochko
2025-03-29 0:13 ` Stefano Stabellini
2025-03-26 5:50 ` [PATCH v2 16/19] xen/sysctl: make CONFIG_COVERAGE depend on CONFIG_SYSCTL Penny Zheng
` (3 subsequent siblings)
18 siblings, 2 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné,
Alistair Francis, Bob Eshleman, Connor Davis, Oleksii Kurochko
The following functions are only used to deal with XEN_SYSCTL_physinfo,
then they shall be wrapped:
- arch_do_physinfo
- get_outstanding_claims
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- no need to wrap declaration
- add transient #ifdef in sysctl.c for correct compilation
---
xen/arch/arm/sysctl.c | 2 ++
xen/arch/riscv/stubs.c | 2 ++
xen/arch/x86/sysctl.c | 2 ++
xen/common/page_alloc.c | 2 ++
xen/common/sysctl.c | 2 +-
5 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index 32cab4feff..2d350b700a 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -15,6 +15,7 @@
#include <asm/arm64/sve.h>
#include <public/sysctl.h>
+#ifdef CONFIG_SYSCTL
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
@@ -22,6 +23,7 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
}
+#endif
long arch_do_sysctl(struct xen_sysctl *sysctl,
XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 5951b0ce91..7b3f748886 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -328,10 +328,12 @@ long arch_do_sysctl(struct xen_sysctl *sysctl,
BUG_ON("unimplemented");
}
+#ifdef CONFIG_SYSCTL
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
BUG_ON("unimplemented");
}
+#endif /* CONFIG_SYSCTL */
/* p2m.c */
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 1b04947516..d7da476379 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -91,6 +91,7 @@ static long cf_check smt_up_down_helper(void *data)
return ret;
}
+#ifdef CONFIG_SYSCTL
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
memcpy(pi->hw_cap, boot_cpu_data.x86_capability,
@@ -104,6 +105,7 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
}
+#endif
long arch_do_sysctl(
struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5803a1ef4e..36424a9245 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -601,6 +601,7 @@ out:
return ret;
}
+#ifdef CONFIG_SYSCTL
void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
{
spin_lock(&heap_lock);
@@ -608,6 +609,7 @@ void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
*free_pages = avail_heap_pages(MEMZONE_XEN + 1, NR_ZONES - 1, -1);
spin_unlock(&heap_lock);
}
+#endif /* CONFIG_SYSCTL */
static bool __read_mostly first_node_initialised;
#ifndef CONFIG_SEPARATE_XENHEAP
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index ccce7fe963..76622503e2 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -258,7 +258,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = sched_adjust_global(&op->u.scheduler_op);
break;
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_physinfo:
{
struct xen_sysctl_physinfo *pi = &op->u.physinfo;
@@ -301,6 +300,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
}
break;
+#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_numainfo:
{
unsigned int i, j, num_nodes;
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo
2025-03-26 5:50 ` [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo Penny Zheng
@ 2025-03-27 9:35 ` Oleksii Kurochko
2025-03-27 9:58 ` Jan Beulich
2025-03-29 0:13 ` Stefano Stabellini
1 sibling, 1 reply; 55+ messages in thread
From: Oleksii Kurochko @ 2025-03-27 9:35 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: ray.huang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis
[-- Attachment #1: Type: text/plain, Size: 4166 bytes --]
On 3/26/25 6:50 AM, Penny Zheng wrote:
> The following functions are only used to deal with XEN_SYSCTL_physinfo,
> then they shall be wrapped:
> - arch_do_physinfo
> - get_outstanding_claims
>
> Signed-off-by: Penny Zheng<Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - no need to wrap declaration
> - add transient #ifdef in sysctl.c for correct compilation
> ---
> xen/arch/arm/sysctl.c | 2 ++
> xen/arch/riscv/stubs.c | 2 ++
> xen/arch/x86/sysctl.c | 2 ++
> xen/common/page_alloc.c | 2 ++
> xen/common/sysctl.c | 2 +-
> 5 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index 32cab4feff..2d350b700a 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -15,6 +15,7 @@
> #include <asm/arm64/sve.h>
> #include <public/sysctl.h>
>
> +#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
> @@ -22,6 +23,7 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
> XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> }
> +#endif
>
> long arch_do_sysctl(struct xen_sysctl *sysctl,
> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 5951b0ce91..7b3f748886 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -328,10 +328,12 @@ long arch_do_sysctl(struct xen_sysctl *sysctl,
> BUG_ON("unimplemented");
> }
>
> +#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> BUG_ON("unimplemented");
> }
> +#endif /* CONFIG_SYSCTL */
Considering that now we will have CONFIG_SYSCTL, I think it would be better just to drop
definition of arch_do_physinfo() from riscv/stubs.c as it was added to make common code build
for RISC-V happy.
Thanks.
~ Oleksii
>
> /* p2m.c */
>
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index 1b04947516..d7da476379 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -91,6 +91,7 @@ static long cf_check smt_up_down_helper(void *data)
> return ret;
> }
>
> +#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> memcpy(pi->hw_cap, boot_cpu_data.x86_capability,
> @@ -104,6 +105,7 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
> }
> +#endif
>
> long arch_do_sysctl(
> struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 5803a1ef4e..36424a9245 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -601,6 +601,7 @@ out:
> return ret;
> }
>
> +#ifdef CONFIG_SYSCTL
> void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
> {
> spin_lock(&heap_lock);
> @@ -608,6 +609,7 @@ void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
> *free_pages = avail_heap_pages(MEMZONE_XEN + 1, NR_ZONES - 1, -1);
> spin_unlock(&heap_lock);
> }
> +#endif /* CONFIG_SYSCTL */
>
> static bool __read_mostly first_node_initialised;
> #ifndef CONFIG_SEPARATE_XENHEAP
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index ccce7fe963..76622503e2 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -258,7 +258,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> ret = sched_adjust_global(&op->u.scheduler_op);
> break;
>
> -#endif /* CONFIG_SYSCTL */
> case XEN_SYSCTL_physinfo:
> {
> struct xen_sysctl_physinfo *pi = &op->u.physinfo;
> @@ -301,6 +300,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> }
> break;
>
> +#endif /* CONFIG_SYSCTL */
> case XEN_SYSCTL_numainfo:
> {
> unsigned int i, j, num_nodes;
[-- Attachment #2: Type: text/html, Size: 4541 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo
2025-03-27 9:35 ` Oleksii Kurochko
@ 2025-03-27 9:58 ` Jan Beulich
2025-03-27 10:25 ` Oleksii Kurochko
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2025-03-27 9:58 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: ray.huang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Penny Zheng, xen-devel
On 27.03.2025 10:35, Oleksii Kurochko wrote:
>
> On 3/26/25 6:50 AM, Penny Zheng wrote:
>> The following functions are only used to deal with XEN_SYSCTL_physinfo,
>> then they shall be wrapped:
>> - arch_do_physinfo
>> - get_outstanding_claims
>>
>> Signed-off-by: Penny Zheng<Penny.Zheng@amd.com>
>> ---
>> v1 -> v2:
>> - no need to wrap declaration
>> - add transient #ifdef in sysctl.c for correct compilation
>> ---
>> xen/arch/arm/sysctl.c | 2 ++
>> xen/arch/riscv/stubs.c | 2 ++
>> xen/arch/x86/sysctl.c | 2 ++
>> xen/common/page_alloc.c | 2 ++
>> xen/common/sysctl.c | 2 +-
>> 5 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
>> index 32cab4feff..2d350b700a 100644
>> --- a/xen/arch/arm/sysctl.c
>> +++ b/xen/arch/arm/sysctl.c
>> @@ -15,6 +15,7 @@
>> #include <asm/arm64/sve.h>
>> #include <public/sysctl.h>
>>
>> +#ifdef CONFIG_SYSCTL
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>> {
>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
>> @@ -22,6 +23,7 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>> pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>> XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>> }
>> +#endif
>>
>> long arch_do_sysctl(struct xen_sysctl *sysctl,
>> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
>> index 5951b0ce91..7b3f748886 100644
>> --- a/xen/arch/riscv/stubs.c
>> +++ b/xen/arch/riscv/stubs.c
>> @@ -328,10 +328,12 @@ long arch_do_sysctl(struct xen_sysctl *sysctl,
>> BUG_ON("unimplemented");
>> }
>>
>> +#ifdef CONFIG_SYSCTL
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>> {
>> BUG_ON("unimplemented");
>> }
>> +#endif /* CONFIG_SYSCTL */
>
> Considering that now we will have CONFIG_SYSCTL, I think it would be better just to drop
> definition of arch_do_physinfo() from riscv/stubs.c as it was added to make common code build
> for RISC-V happy.
Wouldn't that require SYSCTL=n then for RISC-V, which better wouldn't be done
(as it would need undoing later on)?
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo
2025-03-27 9:58 ` Jan Beulich
@ 2025-03-27 10:25 ` Oleksii Kurochko
0 siblings, 0 replies; 55+ messages in thread
From: Oleksii Kurochko @ 2025-03-27 10:25 UTC (permalink / raw)
To: Jan Beulich
Cc: ray.huang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Penny Zheng, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]
On 3/27/25 10:58 AM, Jan Beulich wrote:
> On 27.03.2025 10:35, Oleksii Kurochko wrote:
>> On 3/26/25 6:50 AM, Penny Zheng wrote:
>>> The following functions are only used to deal with XEN_SYSCTL_physinfo,
>>> then they shall be wrapped:
>>> - arch_do_physinfo
>>> - get_outstanding_claims
>>>
>>> Signed-off-by: Penny Zheng<Penny.Zheng@amd.com>
>>> ---
>>> v1 -> v2:
>>> - no need to wrap declaration
>>> - add transient #ifdef in sysctl.c for correct compilation
>>> ---
>>> xen/arch/arm/sysctl.c | 2 ++
>>> xen/arch/riscv/stubs.c | 2 ++
>>> xen/arch/x86/sysctl.c | 2 ++
>>> xen/common/page_alloc.c | 2 ++
>>> xen/common/sysctl.c | 2 +-
>>> 5 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
>>> index 32cab4feff..2d350b700a 100644
>>> --- a/xen/arch/arm/sysctl.c
>>> +++ b/xen/arch/arm/sysctl.c
>>> @@ -15,6 +15,7 @@
>>> #include <asm/arm64/sve.h>
>>> #include <public/sysctl.h>
>>>
>>> +#ifdef CONFIG_SYSCTL
>>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>> {
>>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
>>> @@ -22,6 +23,7 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>> pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>>> XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>>> }
>>> +#endif
>>>
>>> long arch_do_sysctl(struct xen_sysctl *sysctl,
>>> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
>>> index 5951b0ce91..7b3f748886 100644
>>> --- a/xen/arch/riscv/stubs.c
>>> +++ b/xen/arch/riscv/stubs.c
>>> @@ -328,10 +328,12 @@ long arch_do_sysctl(struct xen_sysctl *sysctl,
>>> BUG_ON("unimplemented");
>>> }
>>>
>>> +#ifdef CONFIG_SYSCTL
>>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>> {
>>> BUG_ON("unimplemented");
>>> }
>>> +#endif /* CONFIG_SYSCTL */
>> Considering that now we will have CONFIG_SYSCTL, I think it would be better just to drop
>> definition of arch_do_physinfo() from riscv/stubs.c as it was added to make common code build
>> for RISC-V happy.
> Wouldn't that require SYSCTL=n then for RISC-V, which better wouldn't be done
> (as it would need undoing later on)?
I missed that SYSCTL=y by default.
Then it will be really better to have #ifdef CONFIG_SYSCTL instead of removing
arch_do_physinfo() from riscv/stub.c. (the same is true then for arch_do_sysctl()
in the next patch)
~ Oleksii
[-- Attachment #2: Type: text/html, Size: 3121 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo
2025-03-26 5:50 ` [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo Penny Zheng
2025-03-27 9:35 ` Oleksii Kurochko
@ 2025-03-29 0:13 ` Stefano Stabellini
2025-04-01 14:29 ` Jan Beulich
1 sibling, 1 reply; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:13 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné,
Alistair Francis, Bob Eshleman, Connor Davis, Oleksii Kurochko
On Wed, 26 Mar 2025, Penny Zheng wrote:
> The following functions are only used to deal with XEN_SYSCTL_physinfo,
> then they shall be wrapped:
> - arch_do_physinfo
> - get_outstanding_claims
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo
2025-03-29 0:13 ` Stefano Stabellini
@ 2025-04-01 14:29 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 14:29 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Oleksii Kurochko, Stefano Stabellini
On 29.03.2025 01:13, Stefano Stabellini wrote:
> On Wed, 26 Mar 2025, Penny Zheng wrote:
>> The following functions are only used to deal with XEN_SYSCTL_physinfo,
>> then they shall be wrapped:
>> - arch_do_physinfo
>> - get_outstanding_claims
>>
>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
With earlier, series-wide nit taken care of here:
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 16/19] xen/sysctl: make CONFIG_COVERAGE depend on CONFIG_SYSCTL
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (14 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-26 5:50 ` [PATCH v2 17/19] xen/sysctl: make CONFIG_LIVEPATCH " Penny Zheng
` (2 subsequent siblings)
18 siblings, 0 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
All coverage-related op shall be wrapped around with CONFIG_SYSCTL,
so this commit makes CONFIG_COVERAGE depend on CONFIG_SYSCTL.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1 -> v2:
- commit message refactor
---
---
xen/Kconfig.debug | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index f7cc5ffaab..84d26b3f46 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -37,7 +37,7 @@ config SELF_TESTS
config COVERAGE
bool "Code coverage support"
- depends on !LIVEPATCH
+ depends on !LIVEPATCH && SYSCTL
select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS if !ENFORCE_UNIQUE_SYMBOLS
help
Enable code coverage support.
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 17/19] xen/sysctl: make CONFIG_LIVEPATCH depend on CONFIG_SYSCTL
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (15 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 16/19] xen/sysctl: make CONFIG_COVERAGE depend on CONFIG_SYSCTL Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-26 5:50 ` [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl Penny Zheng
2025-03-26 5:50 ` [PATCH v2 19/19] xen/sysctl: wrap around sysctl hypercall Penny Zheng
18 siblings, 0 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
LIVEPATCH mechanism relies on LIVEPATCH_SYSCTL hypercall, so CONFIG_LIVEPATCH
shall depend on CONFIG_SYSCTL
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1 -> v2:
- commit message refactor
---
xen/common/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 426fa8fcc2..a7e1147e8a 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -421,7 +421,7 @@ config CRYPTO
config LIVEPATCH
bool "Live patching support"
default X86
- depends on "$(XEN_HAS_BUILD_ID)" = "y"
+ depends on "$(XEN_HAS_BUILD_ID)" = "y" && SYSCTL
select CC_SPLIT_SECTIONS
help
Allows a running Xen hypervisor to be dynamically patched using
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (16 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 17/19] xen/sysctl: make CONFIG_LIVEPATCH " Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-03-27 9:39 ` Oleksii Kurochko
` (2 more replies)
2025-03-26 5:50 ` [PATCH v2 19/19] xen/sysctl: wrap around sysctl hypercall Penny Zheng
18 siblings, 3 replies; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné,
Alistair Francis, Bob Eshleman, Connor Davis, Oleksii Kurochko,
Stefano Stabellini, Sergiy Kibrik
Function arch_do_sysctl is to perform arch-specific sysctl op.
Some functions, like psr_get_info for x86, DTB overlay support for arm,
are solely available through sysctl op, then they all shall be wrapped
with CONFIG_SYSCTL
Also, remove all #ifdef CONFIG_SYSCTL-s in arch-specific sysctl.c, as
we put the guardian in Makefile for the whole file.
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
- use "depends on" for config OVERLAY_DTB
- no need to wrap declaration
- add transient #ifdef in sysctl.c for correct compilation
---
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/Makefile | 2 +-
xen/arch/arm/sysctl.c | 2 --
xen/arch/riscv/stubs.c | 2 +-
xen/arch/x86/Makefile | 2 +-
xen/arch/x86/psr.c | 18 ++++++++++++++++++
xen/arch/x86/sysctl.c | 2 --
xen/common/sysctl.c | 2 ++
8 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ffdff1f0a3..aa1b4a6e6b 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -141,6 +141,7 @@ config HAS_ITS
config OVERLAY_DTB
bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
+ depends on SYSCTL
help
Dynamic addition/removal of Xen device tree nodes using a dtbo.
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 4837ad467a..7c6015b84d 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -54,7 +54,7 @@ obj-y += smpboot.o
obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o
-obj-y += sysctl.o
+obj-$(CONFIG_SYSCTL) += sysctl.o
obj-y += time.o
obj-y += traps.o
obj-y += vcpreg.o
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index 2d350b700a..32cab4feff 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -15,7 +15,6 @@
#include <asm/arm64/sve.h>
#include <public/sysctl.h>
-#ifdef CONFIG_SYSCTL
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
@@ -23,7 +22,6 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
}
-#endif
long arch_do_sysctl(struct xen_sysctl *sysctl,
XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 7b3f748886..ae865e1972 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from,
/* sysctl.c */
+#ifdef CONFIG_SYSCTL
long arch_do_sysctl(struct xen_sysctl *sysctl,
XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
{
BUG_ON("unimplemented");
}
-#ifdef CONFIG_SYSCTL
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
BUG_ON("unimplemented");
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f59c9665fd..837eafcbc0 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
obj-y += domctl.o
obj-y += platform_hypercall.o
obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
-obj-y += sysctl.o
+obj-$(CONFIG_SYSCTL) += sysctl.o
endif
extra-y += asm-macros.i
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 5815a35335..499d320e61 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -133,9 +133,11 @@ static const struct feat_props {
*/
enum psr_type alt_type;
+#ifdef CONFIG_SYSCTL
/* get_feat_info is used to return feature HW info through sysctl. */
bool (*get_feat_info)(const struct feat_node *feat,
uint32_t data[], unsigned int array_len);
+#endif
/* write_msr is used to write out feature MSR register. */
void (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
@@ -418,6 +420,7 @@ static bool mba_init_feature(const struct cpuid_leaf *regs,
return true;
}
+#ifdef CONFIG_SYSCTL
static bool cf_check cat_get_feat_info(
const struct feat_node *feat, uint32_t data[], unsigned int array_len)
{
@@ -430,6 +433,7 @@ static bool cf_check cat_get_feat_info(
return true;
}
+#endif /* CONFIG_SYSCTL */
/* L3 CAT props */
static void cf_check l3_cat_write_msr(
@@ -442,11 +446,14 @@ static const struct feat_props l3_cat_props = {
.cos_num = 1,
.type[0] = PSR_TYPE_L3_CBM,
.alt_type = PSR_TYPE_UNKNOWN,
+#ifdef CONFIG_SYSCTL
.get_feat_info = cat_get_feat_info,
+#endif
.write_msr = l3_cat_write_msr,
.sanitize = cat_check_cbm,
};
+#ifdef CONFIG_SYSCTL
/* L3 CDP props */
static bool cf_check l3_cdp_get_feat_info(
const struct feat_node *feat, uint32_t data[], uint32_t array_len)
@@ -458,6 +465,7 @@ static bool cf_check l3_cdp_get_feat_info(
return true;
}
+#endif /* CONFIG_SYSCTL */
static void cf_check l3_cdp_write_msr(
unsigned int cos, uint32_t val, enum psr_type type)
@@ -473,7 +481,9 @@ static const struct feat_props l3_cdp_props = {
.type[0] = PSR_TYPE_L3_DATA,
.type[1] = PSR_TYPE_L3_CODE,
.alt_type = PSR_TYPE_L3_CBM,
+#ifdef CONFIG_SYSCTL
.get_feat_info = l3_cdp_get_feat_info,
+#endif
.write_msr = l3_cdp_write_msr,
.sanitize = cat_check_cbm,
};
@@ -489,11 +499,14 @@ static const struct feat_props l2_cat_props = {
.cos_num = 1,
.type[0] = PSR_TYPE_L2_CBM,
.alt_type = PSR_TYPE_UNKNOWN,
+#ifdef CONFIG_SYSCTL
.get_feat_info = cat_get_feat_info,
+#endif
.write_msr = l2_cat_write_msr,
.sanitize = cat_check_cbm,
};
+#ifdef CONFIG_SYSCTL
/* MBA props */
static bool cf_check mba_get_feat_info(
const struct feat_node *feat, uint32_t data[], unsigned int array_len)
@@ -508,6 +521,7 @@ static bool cf_check mba_get_feat_info(
return true;
}
+#endif /* CONFIG_SYSCTL */
static void cf_check mba_write_msr(
unsigned int cos, uint32_t val, enum psr_type type)
@@ -545,7 +559,9 @@ static const struct feat_props mba_props = {
.cos_num = 1,
.type[0] = PSR_TYPE_MBA_THRTL,
.alt_type = PSR_TYPE_UNKNOWN,
+#ifdef CONFIG_SYSCTL
.get_feat_info = mba_get_feat_info,
+#endif
.write_msr = mba_write_msr,
.sanitize = mba_sanitize_thrtl,
};
@@ -808,6 +824,7 @@ static struct psr_socket_info *get_socket_info(unsigned int socket)
return socket_info + socket;
}
+#ifdef CONFIG_SYSCTL
int psr_get_info(unsigned int socket, enum psr_type type,
uint32_t data[], unsigned int array_len)
{
@@ -839,6 +856,7 @@ int psr_get_info(unsigned int socket, enum psr_type type,
return -EINVAL;
}
+#endif /* CONFIG_SYSCTL */
int psr_get_val(struct domain *d, unsigned int socket,
uint32_t *val, enum psr_type type)
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index d7da476379..1b04947516 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -91,7 +91,6 @@ static long cf_check smt_up_down_helper(void *data)
return ret;
}
-#ifdef CONFIG_SYSCTL
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
memcpy(pi->hw_cap, boot_cpu_data.x86_capability,
@@ -105,7 +104,6 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
}
-#endif
long arch_do_sysctl(
struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 76622503e2..b0e2e43c7e 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -490,8 +490,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
break;
default:
+#ifdef CONFIG_SYSCTL
ret = arch_do_sysctl(op, u_sysctl);
copyback = 0;
+#endif
break;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl
2025-03-26 5:50 ` [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl Penny Zheng
@ 2025-03-27 9:39 ` Oleksii Kurochko
2025-03-29 0:21 ` Stefano Stabellini
2025-04-01 14:46 ` Jan Beulich
2 siblings, 0 replies; 55+ messages in thread
From: Oleksii Kurochko @ 2025-03-27 9:39 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: ray.huang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Jan Beulich, Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Stefano Stabellini, Sergiy Kibrik
[-- Attachment #1: Type: text/plain, Size: 8891 bytes --]
On 3/26/25 6:50 AM, Penny Zheng wrote:
> Function arch_do_sysctl is to perform arch-specific sysctl op.
> Some functions, like psr_get_info for x86, DTB overlay support for arm,
> are solely available through sysctl op, then they all shall be wrapped
> with CONFIG_SYSCTL
> Also, remove all #ifdef CONFIG_SYSCTL-s in arch-specific sysctl.c, as
> we put the guardian in Makefile for the whole file.
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@amd.com>
> Signed-off-by: Sergiy Kibrik<Sergiy_Kibrik@epam.com>
> Signed-off-by: Penny Zheng<Penny.Zheng@amd.com>
> ---
> - use "depends on" for config OVERLAY_DTB
> - no need to wrap declaration
> - add transient #ifdef in sysctl.c for correct compilation
> ---
> xen/arch/arm/Kconfig | 1 +
> xen/arch/arm/Makefile | 2 +-
> xen/arch/arm/sysctl.c | 2 --
> xen/arch/riscv/stubs.c | 2 +-
> xen/arch/x86/Makefile | 2 +-
> xen/arch/x86/psr.c | 18 ++++++++++++++++++
> xen/arch/x86/sysctl.c | 2 --
> xen/common/sysctl.c | 2 ++
> 8 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ffdff1f0a3..aa1b4a6e6b 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -141,6 +141,7 @@ config HAS_ITS
>
> config OVERLAY_DTB
> bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> + depends on SYSCTL
> help
> Dynamic addition/removal of Xen device tree nodes using a dtbo.
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 4837ad467a..7c6015b84d 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -54,7 +54,7 @@ obj-y += smpboot.o
> obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
> obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
> obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
> obj-y += time.o
> obj-y += traps.o
> obj-y += vcpreg.o
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index 2d350b700a..32cab4feff 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -15,7 +15,6 @@
> #include <asm/arm64/sve.h>
> #include <public/sysctl.h>
>
> -#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
> @@ -23,7 +22,6 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
> XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> }
> -#endif
>
> long arch_do_sysctl(struct xen_sysctl *sysctl,
> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 7b3f748886..ae865e1972 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from,
>
> /* sysctl.c */
>
> +#ifdef CONFIG_SYSCTL
> long arch_do_sysctl(struct xen_sysctl *sysctl,
> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> BUG_ON("unimplemented");
> }
The same as in the previous patch. We can just drop this function for RISC-V as now we
have CONFIG_SYSCTL. This stub was introduced only to make common code build for RISC-V happy.
Thanks.
~ Oleksii
>
> -#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> BUG_ON("unimplemented");
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f59c9665fd..837eafcbc0 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
> obj-y += domctl.o
> obj-y += platform_hypercall.o
> obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
> endif
>
> extra-y += asm-macros.i
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 5815a35335..499d320e61 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -133,9 +133,11 @@ static const struct feat_props {
> */
> enum psr_type alt_type;
>
> +#ifdef CONFIG_SYSCTL
> /* get_feat_info is used to return feature HW info through sysctl. */
> bool (*get_feat_info)(const struct feat_node *feat,
> uint32_t data[], unsigned int array_len);
> +#endif
>
> /* write_msr is used to write out feature MSR register. */
> void (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
> @@ -418,6 +420,7 @@ static bool mba_init_feature(const struct cpuid_leaf *regs,
> return true;
> }
>
> +#ifdef CONFIG_SYSCTL
> static bool cf_check cat_get_feat_info(
> const struct feat_node *feat, uint32_t data[], unsigned int array_len)
> {
> @@ -430,6 +433,7 @@ static bool cf_check cat_get_feat_info(
>
> return true;
> }
> +#endif /* CONFIG_SYSCTL */
>
> /* L3 CAT props */
> static void cf_check l3_cat_write_msr(
> @@ -442,11 +446,14 @@ static const struct feat_props l3_cat_props = {
> .cos_num = 1,
> .type[0] = PSR_TYPE_L3_CBM,
> .alt_type = PSR_TYPE_UNKNOWN,
> +#ifdef CONFIG_SYSCTL
> .get_feat_info = cat_get_feat_info,
> +#endif
> .write_msr = l3_cat_write_msr,
> .sanitize = cat_check_cbm,
> };
>
> +#ifdef CONFIG_SYSCTL
> /* L3 CDP props */
> static bool cf_check l3_cdp_get_feat_info(
> const struct feat_node *feat, uint32_t data[], uint32_t array_len)
> @@ -458,6 +465,7 @@ static bool cf_check l3_cdp_get_feat_info(
>
> return true;
> }
> +#endif /* CONFIG_SYSCTL */
>
> static void cf_check l3_cdp_write_msr(
> unsigned int cos, uint32_t val, enum psr_type type)
> @@ -473,7 +481,9 @@ static const struct feat_props l3_cdp_props = {
> .type[0] = PSR_TYPE_L3_DATA,
> .type[1] = PSR_TYPE_L3_CODE,
> .alt_type = PSR_TYPE_L3_CBM,
> +#ifdef CONFIG_SYSCTL
> .get_feat_info = l3_cdp_get_feat_info,
> +#endif
> .write_msr = l3_cdp_write_msr,
> .sanitize = cat_check_cbm,
> };
> @@ -489,11 +499,14 @@ static const struct feat_props l2_cat_props = {
> .cos_num = 1,
> .type[0] = PSR_TYPE_L2_CBM,
> .alt_type = PSR_TYPE_UNKNOWN,
> +#ifdef CONFIG_SYSCTL
> .get_feat_info = cat_get_feat_info,
> +#endif
> .write_msr = l2_cat_write_msr,
> .sanitize = cat_check_cbm,
> };
>
> +#ifdef CONFIG_SYSCTL
> /* MBA props */
> static bool cf_check mba_get_feat_info(
> const struct feat_node *feat, uint32_t data[], unsigned int array_len)
> @@ -508,6 +521,7 @@ static bool cf_check mba_get_feat_info(
>
> return true;
> }
> +#endif /* CONFIG_SYSCTL */
>
> static void cf_check mba_write_msr(
> unsigned int cos, uint32_t val, enum psr_type type)
> @@ -545,7 +559,9 @@ static const struct feat_props mba_props = {
> .cos_num = 1,
> .type[0] = PSR_TYPE_MBA_THRTL,
> .alt_type = PSR_TYPE_UNKNOWN,
> +#ifdef CONFIG_SYSCTL
> .get_feat_info = mba_get_feat_info,
> +#endif
> .write_msr = mba_write_msr,
> .sanitize = mba_sanitize_thrtl,
> };
> @@ -808,6 +824,7 @@ static struct psr_socket_info *get_socket_info(unsigned int socket)
> return socket_info + socket;
> }
>
> +#ifdef CONFIG_SYSCTL
> int psr_get_info(unsigned int socket, enum psr_type type,
> uint32_t data[], unsigned int array_len)
> {
> @@ -839,6 +856,7 @@ int psr_get_info(unsigned int socket, enum psr_type type,
>
> return -EINVAL;
> }
> +#endif /* CONFIG_SYSCTL */
>
> int psr_get_val(struct domain *d, unsigned int socket,
> uint32_t *val, enum psr_type type)
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index d7da476379..1b04947516 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -91,7 +91,6 @@ static long cf_check smt_up_down_helper(void *data)
> return ret;
> }
>
> -#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> memcpy(pi->hw_cap, boot_cpu_data.x86_capability,
> @@ -105,7 +104,6 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
> }
> -#endif
>
> long arch_do_sysctl(
> struct xen_sysctl *sysctl, XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 76622503e2..b0e2e43c7e 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -490,8 +490,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> break;
>
> default:
> +#ifdef CONFIG_SYSCTL
> ret = arch_do_sysctl(op, u_sysctl);
> copyback = 0;
> +#endif
> break;
> }
>
[-- Attachment #2: Type: text/html, Size: 9047 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl
2025-03-26 5:50 ` [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl Penny Zheng
2025-03-27 9:39 ` Oleksii Kurochko
@ 2025-03-29 0:21 ` Stefano Stabellini
2025-04-01 14:46 ` Jan Beulich
2 siblings, 0 replies; 55+ messages in thread
From: Stefano Stabellini @ 2025-03-29 0:21 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, ray.huang, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
Anthony PERARD, Jan Beulich, Roger Pau Monné,
Alistair Francis, Bob Eshleman, Connor Davis, Oleksii Kurochko,
Stefano Stabellini, Sergiy Kibrik
On Wed, 26 Mar 2025, Penny Zheng wrote:
> Function arch_do_sysctl is to perform arch-specific sysctl op.
> Some functions, like psr_get_info for x86, DTB overlay support for arm,
> are solely available through sysctl op, then they all shall be wrapped
> with CONFIG_SYSCTL
> Also, remove all #ifdef CONFIG_SYSCTL-s in arch-specific sysctl.c, as
> we put the guardian in Makefile for the whole file.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl
2025-03-26 5:50 ` [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl Penny Zheng
2025-03-27 9:39 ` Oleksii Kurochko
2025-03-29 0:21 ` Stefano Stabellini
@ 2025-04-01 14:46 ` Jan Beulich
2025-04-18 9:46 ` Penny, Zheng
2 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 14:46 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Oleksii Kurochko, Stefano Stabellini, Sergiy Kibrik,
xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> Function arch_do_sysctl is to perform arch-specific sysctl op.
> Some functions, like psr_get_info for x86, DTB overlay support for arm,
> are solely available through sysctl op, then they all shall be wrapped
> with CONFIG_SYSCTL
> Also, remove all #ifdef CONFIG_SYSCTL-s in arch-specific sysctl.c, as
> we put the guardian in Makefile for the whole file.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> - use "depends on" for config OVERLAY_DTB
> - no need to wrap declaration
> - add transient #ifdef in sysctl.c for correct compilation
> ---
> xen/arch/arm/Kconfig | 1 +
> xen/arch/arm/Makefile | 2 +-
> xen/arch/arm/sysctl.c | 2 --
> xen/arch/riscv/stubs.c | 2 +-
> xen/arch/x86/Makefile | 2 +-
> xen/arch/x86/psr.c | 18 ++++++++++++++++++
> xen/arch/x86/sysctl.c | 2 --
> xen/common/sysctl.c | 2 ++
> 8 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ffdff1f0a3..aa1b4a6e6b 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -141,6 +141,7 @@ config HAS_ITS
>
> config OVERLAY_DTB
> bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> + depends on SYSCTL
> help
> Dynamic addition/removal of Xen device tree nodes using a dtbo.
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 4837ad467a..7c6015b84d 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -54,7 +54,7 @@ obj-y += smpboot.o
> obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
> obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
> obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
> obj-y += time.o
> obj-y += traps.o
> obj-y += vcpreg.o
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index 2d350b700a..32cab4feff 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -15,7 +15,6 @@
> #include <asm/arm64/sve.h>
> #include <public/sysctl.h>
>
> -#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
> @@ -23,7 +22,6 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
> XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> }
> -#endif
>
> long arch_do_sysctl(struct xen_sysctl *sysctl,
> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 7b3f748886..ae865e1972 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from,
>
> /* sysctl.c */
>
> +#ifdef CONFIG_SYSCTL
> long arch_do_sysctl(struct xen_sysctl *sysctl,
> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> BUG_ON("unimplemented");
> }
>
> -#ifdef CONFIG_SYSCTL
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> BUG_ON("unimplemented");
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f59c9665fd..837eafcbc0 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
> obj-y += domctl.o
> obj-y += platform_hypercall.o
> obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
> endif
I think I had indicated before that this shouldn't stay inside the conditional,
but move back up. Whether that is to happen here or while addressing my
respective comment on patch 01 I can't easily tell.
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -490,8 +490,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> break;
>
> default:
> +#ifdef CONFIG_SYSCTL
> ret = arch_do_sysctl(op, u_sysctl);
> copyback = 0;
> +#endif
> break;
> }
This isn't enough. "ret" is 0 when reaching the default: label, but may not
stay 0 for the return from the function. I understand (expect) this is going to
be dropped again in the next patch, but even if only transiently needed this
should be kept correct imo. Things might be different if patch 02 introduced
the option without a prompt, i.e. always enabled. Then all the #ifdef-ary added
up to here would be merely syntactic sugar. In fact in that case you could omit
all the transient #ifdef that the last patch is going to remove again. Please
consider going that route.
Otherwise I think the #endif also needs moving up, for copyback to still be
cleared here.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread* RE: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl
2025-04-01 14:46 ` Jan Beulich
@ 2025-04-18 9:46 ` Penny, Zheng
2025-04-22 6:50 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Penny, Zheng @ 2025-04-18 9:46 UTC (permalink / raw)
To: Jan Beulich
Cc: Huang, Ray, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Orzel, Michal, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Oleksii Kurochko, Stabellini, Stefano,
Sergiy Kibrik, xen-devel@lists.xenproject.org
[Public]
Hi，
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 1, 2025 10:47 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>; Alistair Francis
> <alistair.francis@wdc.com>; Bob Eshleman <bobbyeshleman@gmail.com>;
> Connor Davis <connojdavis@gmail.com>; Oleksii Kurochko
> <oleksii.kurochko@gmail.com>; Stabellini, Stefano
> <stefano.stabellini@amd.com>; Sergiy Kibrik <Sergiy_Kibrik@epam.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific
> arch_do_sysctl
>
> On 26.03.2025 06:50, Penny Zheng wrote:
> > Function arch_do_sysctl is to perform arch-specific sysctl op.
> > Some functions, like psr_get_info for x86, DTB overlay support for
> > arm, are solely available through sysctl op, then they all shall be
> > wrapped with CONFIG_SYSCTL Also, remove all #ifdef CONFIG_SYSCTL-s in
> > arch-specific sysctl.c, as we put the guardian in Makefile for the
> > whole file.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> > ---
> > - use "depends on" for config OVERLAY_DTB
> > - no need to wrap declaration
> > - add transient #ifdef in sysctl.c for correct compilation
> > ---
> > xen/arch/arm/Kconfig | 1 +
> > xen/arch/arm/Makefile | 2 +-
> > xen/arch/arm/sysctl.c | 2 --
> > xen/arch/riscv/stubs.c | 2 +-
> > xen/arch/x86/Makefile | 2 +-
> > xen/arch/x86/psr.c | 18 ++++++++++++++++++
> > xen/arch/x86/sysctl.c | 2 --
> > xen/common/sysctl.c | 2 ++
> > 8 files changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > ffdff1f0a3..aa1b4a6e6b 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -141,6 +141,7 @@ config HAS_ITS
> >
> > config OVERLAY_DTB
> > bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> > + depends on SYSCTL
> > help
> > Dynamic addition/removal of Xen device tree nodes using a dtbo.
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index
> > 4837ad467a..7c6015b84d 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -54,7 +54,7 @@ obj-y += smpboot.o
> > obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
> > obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
> > obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o -obj-y += sysctl.o
> > +obj-$(CONFIG_SYSCTL) += sysctl.o
> > obj-y += time.o
> > obj-y += traps.o
> > obj-y += vcpreg.o
> > diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c index
> > 2d350b700a..32cab4feff 100644
> > --- a/xen/arch/arm/sysctl.c
> > +++ b/xen/arch/arm/sysctl.c
> > @@ -15,7 +15,6 @@
> > #include <asm/arm64/sve.h>
> > #include <public/sysctl.h>
> >
> > -#ifdef CONFIG_SYSCTL
> > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {
> > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm |
> > XEN_SYSCTL_PHYSCAP_hap; @@ -23,7 +22,6 @@ void
> arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> > pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
> >
> > XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); } -#endif
> >
> > long arch_do_sysctl(struct xen_sysctl *sysctl,
> > XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index
> > 7b3f748886..ae865e1972 100644
> > --- a/xen/arch/riscv/stubs.c
> > +++ b/xen/arch/riscv/stubs.c
> > @@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to,
> > const void __user *from,
> >
> > /* sysctl.c */
> >
> > +#ifdef CONFIG_SYSCTL
> > long arch_do_sysctl(struct xen_sysctl *sysctl,
> > XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) {
> > BUG_ON("unimplemented");
> > }
> >
> > -#ifdef CONFIG_SYSCTL
> > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {
> > BUG_ON("unimplemented");
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index
> > f59c9665fd..837eafcbc0 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) obj-y +=
> > domctl.o obj-y += platform_hypercall.o
> > obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o -obj-y +=
> > sysctl.o
> > +obj-$(CONFIG_SYSCTL) += sysctl.o
> > endif
>
> I think I had indicated before that this shouldn't stay inside the conditional, but
> move back up. Whether that is to happen here or while addressing my respective
> comment on patch 01 I can't easily tell.
>
We want that "PV_SHIM_EXCLUSIVE likely wants / needs sorting as
a prereq anyway", does the prereq here mean that prereq in kconfig,
something like
```
config SYSCTL
depends on xxx
```
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -490,8 +490,10 @@ long
> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > break;
> >
> > default:
> > +#ifdef CONFIG_SYSCTL
> > ret = arch_do_sysctl(op, u_sysctl);
> > copyback = 0;
> > +#endif
> > break;
> > }
>
> This isn't enough. "ret" is 0 when reaching the default: label, but may not stay 0 for
> the return from the function. I understand (expect) this is going to be dropped
> again in the next patch, but even if only transiently needed this should be kept
> correct imo. Things might be different if patch 02 introduced the option without a
> prompt, i.e. always enabled. Then all the #ifdef-ary added up to here would be
> merely syntactic sugar. In fact in that case you could omit all the transient #ifdef
> that the last patch is going to remove again. Please consider going that route.
>
> Otherwise I think the #endif also needs moving up, for copyback to still be cleared
> here.
>
I'll change it to as follows to complement case for CONFIG_SYSCTL==n, plz correct me if I understand wrongly here:
```
default:
+#ifdef CONFIG_SYSCTL
ret = arch_do_sysctl(op, u_sysctl);
+#else
+ ret = -EOPNOTSUPP;
+#endif
copyback = 0;
break;
```
> Jan
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl
2025-04-18 9:46 ` Penny, Zheng
@ 2025-04-22 6:50 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-22 6:50 UTC (permalink / raw)
To: Penny, Zheng
Cc: Huang, Ray, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Orzel, Michal, Volodymyr Babchuk, Andrew Cooper, Anthony PERARD,
Roger Pau Monné, Alistair Francis, Bob Eshleman,
Connor Davis, Oleksii Kurochko, Stabellini, Stefano,
Sergiy Kibrik, xen-devel@lists.xenproject.org
On 18.04.2025 11:46, Penny, Zheng wrote:
> [Public]
>
> Hi，
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, April 1, 2025 10:47 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>; Alistair Francis
>> <alistair.francis@wdc.com>; Bob Eshleman <bobbyeshleman@gmail.com>;
>> Connor Davis <connojdavis@gmail.com>; Oleksii Kurochko
>> <oleksii.kurochko@gmail.com>; Stabellini, Stefano
>> <stefano.stabellini@amd.com>; Sergiy Kibrik <Sergiy_Kibrik@epam.com>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific
>> arch_do_sysctl
>>
>> On 26.03.2025 06:50, Penny Zheng wrote:
>>> Function arch_do_sysctl is to perform arch-specific sysctl op.
>>> Some functions, like psr_get_info for x86, DTB overlay support for
>>> arm, are solely available through sysctl op, then they all shall be
>>> wrapped with CONFIG_SYSCTL Also, remove all #ifdef CONFIG_SYSCTL-s in
>>> arch-specific sysctl.c, as we put the guardian in Makefile for the
>>> whole file.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>> ---
>>> - use "depends on" for config OVERLAY_DTB
>>> - no need to wrap declaration
>>> - add transient #ifdef in sysctl.c for correct compilation
>>> ---
>>> xen/arch/arm/Kconfig | 1 +
>>> xen/arch/arm/Makefile | 2 +-
>>> xen/arch/arm/sysctl.c | 2 --
>>> xen/arch/riscv/stubs.c | 2 +-
>>> xen/arch/x86/Makefile | 2 +-
>>> xen/arch/x86/psr.c | 18 ++++++++++++++++++
>>> xen/arch/x86/sysctl.c | 2 --
>>> xen/common/sysctl.c | 2 ++
>>> 8 files changed, 24 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
>>> ffdff1f0a3..aa1b4a6e6b 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -141,6 +141,7 @@ config HAS_ITS
>>>
>>> config OVERLAY_DTB
>>> bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
>>> + depends on SYSCTL
>>> help
>>> Dynamic addition/removal of Xen device tree nodes using a dtbo.
>>>
>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index
>>> 4837ad467a..7c6015b84d 100644
>>> --- a/xen/arch/arm/Makefile
>>> +++ b/xen/arch/arm/Makefile
>>> @@ -54,7 +54,7 @@ obj-y += smpboot.o
>>> obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
>>> obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
>>> obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o -obj-y += sysctl.o
>>> +obj-$(CONFIG_SYSCTL) += sysctl.o
>>> obj-y += time.o
>>> obj-y += traps.o
>>> obj-y += vcpreg.o
>>> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c index
>>> 2d350b700a..32cab4feff 100644
>>> --- a/xen/arch/arm/sysctl.c
>>> +++ b/xen/arch/arm/sysctl.c
>>> @@ -15,7 +15,6 @@
>>> #include <asm/arm64/sve.h>
>>> #include <public/sysctl.h>
>>>
>>> -#ifdef CONFIG_SYSCTL
>>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {
>>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm |
>>> XEN_SYSCTL_PHYSCAP_hap; @@ -23,7 +22,6 @@ void
>> arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>> pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>>>
>>> XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); } -#endif
>>>
>>> long arch_do_sysctl(struct xen_sysctl *sysctl,
>>> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index
>>> 7b3f748886..ae865e1972 100644
>>> --- a/xen/arch/riscv/stubs.c
>>> +++ b/xen/arch/riscv/stubs.c
>>> @@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to,
>>> const void __user *from,
>>>
>>> /* sysctl.c */
>>>
>>> +#ifdef CONFIG_SYSCTL
>>> long arch_do_sysctl(struct xen_sysctl *sysctl,
>>> XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) {
>>> BUG_ON("unimplemented");
>>> }
>>>
>>> -#ifdef CONFIG_SYSCTL
>>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {
>>> BUG_ON("unimplemented");
>>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index
>>> f59c9665fd..837eafcbc0 100644
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) obj-y +=
>>> domctl.o obj-y += platform_hypercall.o
>>> obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o -obj-y +=
>>> sysctl.o
>>> +obj-$(CONFIG_SYSCTL) += sysctl.o
>>> endif
>>
>> I think I had indicated before that this shouldn't stay inside the conditional, but
>> move back up. Whether that is to happen here or while addressing my respective
>> comment on patch 01 I can't easily tell.
>
> We want that "PV_SHIM_EXCLUSIVE likely wants / needs sorting as
> a prereq anyway", does the prereq here mean that prereq in kconfig,
> something like
> ```
> config SYSCTL
> depends on xxx
> ```
I'm sorry, but I fear I can't interpret what you're saying (possibly asking).
>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -490,8 +490,10 @@ long
>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>> break;
>>>
>>> default:
>>> +#ifdef CONFIG_SYSCTL
>>> ret = arch_do_sysctl(op, u_sysctl);
>>> copyback = 0;
>>> +#endif
>>> break;
>>> }
>>
>> This isn't enough. "ret" is 0 when reaching the default: label, but may not stay 0 for
>> the return from the function. I understand (expect) this is going to be dropped
>> again in the next patch, but even if only transiently needed this should be kept
>> correct imo. Things might be different if patch 02 introduced the option without a
>> prompt, i.e. always enabled. Then all the #ifdef-ary added up to here would be
>> merely syntactic sugar. In fact in that case you could omit all the transient #ifdef
>> that the last patch is going to remove again. Please consider going that route.
>>
>> Otherwise I think the #endif also needs moving up, for copyback to still be cleared
>> here.
>>
>
> I'll change it to as follows to complement case for CONFIG_SYSCTL==n, plz correct me if I understand wrongly here:
> ```
> default:
> +#ifdef CONFIG_SYSCTL
> ret = arch_do_sysctl(op, u_sysctl);
> +#else
> + ret = -EOPNOTSUPP;
> +#endif
> copyback = 0;
> break;
> ```
This is an option, yes, yet I'd like my other outline to be taken into consideration,
too (for imo resulting in less churn overall).
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 19/19] xen/sysctl: wrap around sysctl hypercall
2025-03-26 5:50 [PATCH v2 00/19] xen: introduce CONFIG_SYSCTL Penny Zheng
` (17 preceding siblings ...)
2025-03-26 5:50 ` [PATCH v2 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl Penny Zheng
@ 2025-03-26 5:50 ` Penny Zheng
2025-04-01 14:49 ` Jan Beulich
18 siblings, 1 reply; 55+ messages in thread
From: Penny Zheng @ 2025-03-26 5:50 UTC (permalink / raw)
To: xen-devel
Cc: ray.huang, Stefano Stabellini, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Sergiy Kibrik, Penny Zheng
From: Stefano Stabellini <stefano.stabellini@amd.com>
We intend to wrap around sysctl hypercall, and also correct the order
alphabetically
We also need to removes all transient "#ifdef CONFIG_SYSCTL"-s in sysctl.c.
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- remove all transient "#ifdef CONFIG_SYSCTL"-s in sysctl.c
---
xen/common/Makefile | 2 +-
xen/common/sysctl.c | 10 ----------
xen/include/hypercall-defs.c | 8 ++++++--
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/xen/common/Makefile b/xen/common/Makefile
index ac23120d7d..d45dfeb5b6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -69,7 +69,7 @@ obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o
ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
obj-y += domctl.o
obj-$(CONFIG_VM_EVENT) += monitor.o
-obj-y += sysctl.o
+obj-$(CONFIG_SYSCTL) += sysctl.o
endif
extra-y := symbols-dummy.o
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index b0e2e43c7e..ba8c5e2266 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -58,7 +58,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
switch ( op->cmd )
{
-#ifdef CONFIG_SYSCTL
case XEN_SYSCTL_readconsole:
ret = xsm_readconsole(XSM_HOOK, op->u.readconsole.clear);
if ( ret )
@@ -67,17 +66,14 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = read_console_ring(&op->u.readconsole);
break;
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_tbuf_op:
ret = tb_control(&op->u.tbuf_op);
break;
-#ifdef CONFIG_SYSCTL
case XEN_SYSCTL_sched_id:
op->u.sched_id.sched_id = scheduler_id();
break;
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_getdomaininfolist:
{
struct domain *d;
@@ -117,7 +113,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
}
break;
-#ifdef CONFIG_SYSCTL
#ifdef CONFIG_PERF_COUNTERS
case XEN_SYSCTL_perfc_op:
ret = perfc_control(&op->u.perfc_op);
@@ -129,7 +124,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
ret = spinlock_profile_control(&op->u.lockprof_op);
break;
#endif
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_debug_keys:
{
char c;
@@ -188,7 +182,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
break;
#endif /* CONFIG_PM_STATS */
-#ifdef CONFIG_SYSCTL
case XEN_SYSCTL_page_offline_op:
{
uint32_t *status, *ptr;
@@ -300,7 +293,6 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
}
break;
-#endif /* CONFIG_SYSCTL */
case XEN_SYSCTL_numainfo:
{
unsigned int i, j, num_nodes;
@@ -490,10 +482,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
break;
default:
-#ifdef CONFIG_SYSCTL
ret = arch_do_sysctl(op, u_sysctl);
copyback = 0;
-#endif
break;
}
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 7720a29ade..322aa6eb09 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -195,10 +195,12 @@ kexec_op(unsigned long op, void *uarg)
dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
#endif
#ifndef CONFIG_PV_SHIM_EXCLUSIVE
-sysctl(xen_sysctl_t *u_sysctl)
domctl(xen_domctl_t *u_domctl)
paging_domctl_cont(xen_domctl_t *u_domctl)
platform_op(xen_platform_op_t *u_xenpf_op)
+#ifdef CONFIG_SYSCTL
+sysctl(xen_sysctl_t *u_sysctl)
+#endif
#endif
#ifdef CONFIG_HVM
hvm_op(unsigned long op, void *arg)
@@ -274,8 +276,10 @@ physdev_op compat do hvm hvm do_arm
hvm_op do do do do do
#endif
#ifndef CONFIG_PV_SHIM_EXCLUSIVE
-sysctl do do do do do
domctl do do do do do
+#ifdef CONFIG_SYSCTL
+sysctl do do do do do
+#endif
#endif
#ifdef CONFIG_KEXEC
kexec_op compat do - - -
--
2.34.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 19/19] xen/sysctl: wrap around sysctl hypercall
2025-03-26 5:50 ` [PATCH v2 19/19] xen/sysctl: wrap around sysctl hypercall Penny Zheng
@ 2025-04-01 14:49 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2025-04-01 14:49 UTC (permalink / raw)
To: Penny Zheng
Cc: ray.huang, Stefano Stabellini, Andrew Cooper, Anthony PERARD,
Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Sergiy Kibrik, xen-devel
On 26.03.2025 06:50, Penny Zheng wrote:
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -69,7 +69,7 @@ obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o
> ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
> obj-y += domctl.o
> obj-$(CONFIG_VM_EVENT) += monitor.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
> endif
Same as for the respective x86 change: Imo this needs to move out of the
conditional again, back into the normal list of objects.
> --- a/xen/include/hypercall-defs.c
> +++ b/xen/include/hypercall-defs.c
> @@ -195,10 +195,12 @@ kexec_op(unsigned long op, void *uarg)
> dm_op(domid_t domid, unsigned int nr_bufs, xen_dm_op_buf_t *bufs)
> #endif
> #ifndef CONFIG_PV_SHIM_EXCLUSIVE
> -sysctl(xen_sysctl_t *u_sysctl)
> domctl(xen_domctl_t *u_domctl)
> paging_domctl_cont(xen_domctl_t *u_domctl)
> platform_op(xen_platform_op_t *u_xenpf_op)
> +#ifdef CONFIG_SYSCTL
> +sysctl(xen_sysctl_t *u_sysctl)
> +#endif
> #endif
If you don't move this out of the enclosing #ifdef, I don't see why you move
it at all.
> @@ -274,8 +276,10 @@ physdev_op compat do hvm hvm do_arm
> hvm_op do do do do do
> #endif
> #ifndef CONFIG_PV_SHIM_EXCLUSIVE
> -sysctl do do do do do
> domctl do do do do do
> +#ifdef CONFIG_SYSCTL
> +sysctl do do do do do
> +#endif
> #endif
Same here.
Jan
^ permalink raw reply [flat|nested] 55+ messages in thread