[PATCH v1 00/19] xen: introduce CONFIG_SYSCTL

[PATCH v1 00/19] xen: introduce CONFIG_SYSCTL

[Penny Zheng]

It can be beneficial for some dom0less systems to further reduce Xen footprint
and disable some hypercalls handling code, which may not to be used & required
in such systems. Each hypercall has a separate option to keep configuration
flexible.

Options to disable hypercalls:
- sysctl
- domctl
- hvm
- physdev
- platform

This patch serie is only focusing on introducing CONFIG_SYSCTL. Different
options will be covered in different patch serie.

Features, like LIVEPATCH, Overlay DTB, which fully rely on sysctl op, are also
being wrapped with proper CONFIG_SYSCTL, to reduce Xen footprint as much as
possible.

It is based on Stefano Stabellini's commit "xen: introduce kconfig options to
disable hypercalls"(
https://lore.kernel.org/xen-devel/20241219092917.3006174-1-Sergiy_Kibrik@epam.com)

Penny Zheng (17):
  xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL
  xen/sysctl: wrap around XEN_SYSCTL_readconsole
  xen/sysctl: make CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL
  xen/sysctl: wrap around XEN_SYSCTL_sched_id
  xen/sysctl: wrap around XEN_SYSCTL_perfc_op
  xen/sysctl: wrap around XEN_SYSCTL_lockprof_op
  xen/sysctl: introduce CONFIG_PM_STATISTIC
  xen/sysctl: make CONFIG_PM_STATISTIC depend on CONFIG_SYSCTL
  xen/page: fix return type of online_page()
  xen/sysctl: wrap around XEN_SYSCTL_page_offline_op
  xen/sysctl: wrap around XEN_SYSCTL_cpupool_op
  xen/sysctl: wrap around XEN_SYSCTL_scheduler_op
  xen: make avail_domheap_pages() static
  xen/sysctl: wrap around XEN_SYSCTL_physinfo
  xen/sysctl: wrap around XEN_SYSCTL_coverage_op
  xen/sysctl: wrap around XEN_SYSCTL_livepatch_op
  xen/sysctl: wrap around arch-specific arch_do_sysctl

Stefano Stabellini (2):
  xen: introduce CONFIG_SYSCTL
  xen/sysctl: wrap around sysctl hypercall

 xen/Kconfig.debug                            |   2 +-
 xen/arch/arm/Kconfig                         |   2 +-
 xen/arch/arm/Makefile                        |   2 +-
 xen/arch/riscv/stubs.c                       |   2 +
 xen/arch/x86/Makefile                        |   2 +-
 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/arch/x86/include/asm/psr.h               |   2 +
 xen/arch/x86/psr.c                           |  18 ++
 xen/common/Kconfig                           |  17 +-
 xen/common/Makefile                          |   2 +-
 xen/common/page_alloc.c                      |  11 +-
 xen/common/perfc.c                           |   2 +
 xen/common/sched/arinc653.c                  |   6 +
 xen/common/sched/core.c                      |   4 +
 xen/common/sched/cpupool.c                   |   9 +
 xen/common/sched/credit.c                    |   4 +
 xen/common/sched/credit2.c                   |   4 +
 xen/common/sched/private.h                   |   4 +
 xen/common/spinlock.c                        |   2 +
 xen/common/sysctl.c                          |   4 +-
 xen/drivers/acpi/Makefile                    |   2 +-
 xen/drivers/acpi/pmstat.c                    | 192 ++++++++++++++++---
 xen/drivers/char/console.c                   |   2 +
 xen/drivers/cpufreq/cpufreq.c                |  31 +++
 xen/drivers/cpufreq/cpufreq_misc_governors.c |   2 +
 xen/drivers/cpufreq/cpufreq_ondemand.c       |   2 +
 xen/drivers/cpufreq/utility.c                | 164 +---------------
 xen/include/acpi/cpufreq/cpufreq.h           |   6 +
 xen/include/acpi/cpufreq/processor_perf.h    |  12 +-
 xen/include/hypercall-defs.c                 |   4 +
 xen/include/xen/acpi.h                       |   2 +
 xen/include/xen/console.h                    |   8 +
 xen/include/xen/hypercall.h                  |   8 +
 xen/include/xen/mm.h                         |  19 +-
 xen/include/xen/perfc.h                      |   8 +
 xen/include/xen/pmstat.h                     |   2 +
 xen/include/xen/sched.h                      |  22 +++
 xen/include/xen/spinlock.h                   |   7 +
 xen/include/xsm/dummy.h                      |  21 ++
 xen/include/xsm/xsm.h                        |  27 +++
 xen/xsm/dummy.c                              |   6 +
 xen/xsm/flask/hooks.c                        |  14 ++
 44 files changed, 464 insertions(+), 208 deletions(-)

-- 
2.34.1

[PATCH v1 01/19] xen: introduce CONFIG_SYSCTL

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@amd.com>

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>
---
 xen/common/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d..72e1d7ea97 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -519,6 +519,15 @@ config TRACEBUFFER
 	  to be collected at run time for debugging or performance analysis.
 	  Memory and execution overhead when not active is minimal.
 
+menu "Supported hypercall interfaces"
+	visible if EXPERT
+
+config SYSCTL
+	bool "Enable sysctl hypercall"
+	depends on !PV_SHIM_EXCLUSIVE
+	default y
+endmenu
+
 config LLC_COLORING
 	bool "Last Level Cache (LLC) coloring" if EXPERT
 	depends on HAS_LLC_COLORING
-- 
2.34.1

RE: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL

[Penny, Zheng]

[Public]

Hi,

> -----Original Message-----
> From: Penny, Zheng <penny.zheng@amd.com>
> Sent: Wednesday, March 12, 2025 12:06 PM
> To: xen-devel@lists.xenproject.org
> Cc: Huang, Ray <Ray.Huang@amd.com>; Stabellini, Stefano
> <stefano.stabellini@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
> <Michal.Orzel@amd.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Sergiy Kibrik <Sergiy_Kibrik@epam.com>; Penny, Zheng
> <penny.zheng@amd.com>
> Subject: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL
>
> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> 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>
> ---
>  xen/common/Kconfig | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig index
> 6166327f4d..72e1d7ea97 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -519,6 +519,15 @@ config TRACEBUFFER
>         to be collected at run time for debugging or performance analysis.
>         Memory and execution overhead when not active is minimal.
>
> +menu "Supported hypercall interfaces"
> +     visible if EXPERT
> +
> +config SYSCTL
> +     bool "Enable sysctl hypercall"
> +     depends on !PV_SHIM_EXCLUSIVE

I'm aware of the commit of "x86: provide an inverted Kconfig control for shim-exclusive mode"[1]
And I'll rebase whenever it gets merged.

> +     default y
> +endmenu
> +
>  config LLC_COLORING
>       bool "Last Level Cache (LLC) coloring" if EXPERT
>       depends on HAS_LLC_COLORING
> --

[1] https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html

> 2.34.1

Many thanks,
Penny

Re: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL

[Jan Beulich]

On 12.03.2025 05:10, Penny, Zheng wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Penny, Zheng <penny.zheng@amd.com>
>> Sent: Wednesday, March 12, 2025 12:06 PM
>> To: xen-devel@lists.xenproject.org
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Stabellini, Stefano
>> <stefano.stabellini@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
>> <Michal.Orzel@amd.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
>> <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Sergiy Kibrik <Sergiy_Kibrik@epam.com>; Penny, Zheng
>> <penny.zheng@amd.com>
>> Subject: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL
>>
>> From: Stefano Stabellini <stefano.stabellini@amd.com>
>>
>> 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>
>> ---
>>  xen/common/Kconfig | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig index
>> 6166327f4d..72e1d7ea97 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -519,6 +519,15 @@ config TRACEBUFFER
>>         to be collected at run time for debugging or performance analysis.
>>         Memory and execution overhead when not active is minimal.
>>
>> +menu "Supported hypercall interfaces"
>> +     visible if EXPERT
>> +
>> +config SYSCTL
>> +     bool "Enable sysctl hypercall"
>> +     depends on !PV_SHIM_EXCLUSIVE
> 
> I'm aware of the commit of "x86: provide an inverted Kconfig control for shim-exclusive mode"[1]
> And I'll rebase whenever it gets merged.

Except that meanwhile it was agreed upon that it won't be merged. Instead
iirc the plan is for someone to go and remove all the "depends on
!PV_SHIM_EXCLUSIVE". Which means new ones better won't be added anymore.

Jan

Re: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL

[Alejandro Vallejo]

Hi,

On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@amd.com>
>

Commit message?

> 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>
> ---
>  xen/common/Kconfig | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 6166327f4d..72e1d7ea97 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -519,6 +519,15 @@ config TRACEBUFFER
>  	  to be collected at run time for debugging or performance analysis.
>  	  Memory and execution overhead when not active is minimal.
>  
> +menu "Supported hypercall interfaces"
> +	visible if EXPERT

Any particular reason for placing it in the middle of the common menu and not
at the end?

> +
> +config SYSCTL
> +	bool "Enable sysctl hypercall"

meganit: Arguably "sysctl" is a hypercall group rather than a hypercall, so
"Enable sysctl hypercalls" sounds (subjectively) more appropriate.

> +	depends on !PV_SHIM_EXCLUSIVE
> +	default y

Do we want a "help" statement stating the scope of the hypercall and the
consequences of disabling it? Nothing major, but at least something that might
convince someone on menuconfig/nconfig that this is indeed nothing to be
touched for a "regular" build of Xen.

> +endmenu
> +
>  config LLC_COLORING
>  	bool "Last Level Cache (LLC) coloring" if EXPERT
>  	depends on HAS_LLC_COLORING

Cheers,
Alejandro

Re: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL

[Jan Beulich]

On 13.03.2025 11:58, Alejandro Vallejo wrote:
> Hi,
> 
> On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
>> From: Stefano Stabellini <stefano.stabellini@amd.com>
>>
> 
> Commit message?
> 
>> 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>
>> ---
>>  xen/common/Kconfig | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index 6166327f4d..72e1d7ea97 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -519,6 +519,15 @@ config TRACEBUFFER
>>  	  to be collected at run time for debugging or performance analysis.
>>  	  Memory and execution overhead when not active is minimal.
>>  
>> +menu "Supported hypercall interfaces"
>> +	visible if EXPERT
> 
> Any particular reason for placing it in the middle of the common menu and not
> at the end?
> 
>> +
>> +config SYSCTL
>> +	bool "Enable sysctl hypercall"
> 
> meganit: Arguably "sysctl" is a hypercall group rather than a hypercall, so
> "Enable sysctl hypercalls" sounds (subjectively) more appropriate.

I disagree. I view it as one hypercall with many sub-ops.

>> +	depends on !PV_SHIM_EXCLUSIVE
>> +	default y
> 
> Do we want a "help" statement stating the scope of the hypercall and the
> consequences of disabling it? Nothing major, but at least something that might
> convince someone on menuconfig/nconfig that this is indeed nothing to be
> touched for a "regular" build of Xen.

Yeah, so help text is certainly wanted.

>> +endmenu

There also wants to be a blank line ahead of this.

Jan

Re: [PATCH v1 01/19] xen: introduce CONFIG_SYSCTL

[Alejandro Vallejo]

On Thu Mar 13, 2025 at 4:24 PM GMT, Jan Beulich wrote:
> On 13.03.2025 11:58, Alejandro Vallejo wrote:
> > Hi,
> > 
> > On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> >> From: Stefano Stabellini <stefano.stabellini@amd.com>
> >>
> > 
> > Commit message?
> > 
> >> 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>
> >> ---
> >>  xen/common/Kconfig | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> >> index 6166327f4d..72e1d7ea97 100644
> >> --- a/xen/common/Kconfig
> >> +++ b/xen/common/Kconfig
> >> @@ -519,6 +519,15 @@ config TRACEBUFFER
> >>  	  to be collected at run time for debugging or performance analysis.
> >>  	  Memory and execution overhead when not active is minimal.
> >>  
> >> +menu "Supported hypercall interfaces"
> >> +	visible if EXPERT
> > 
> > Any particular reason for placing it in the middle of the common menu and not
> > at the end?
> > 
> >> +
> >> +config SYSCTL
> >> +	bool "Enable sysctl hypercall"
> > 
> > meganit: Arguably "sysctl" is a hypercall group rather than a hypercall, so
> > "Enable sysctl hypercalls" sounds (subjectively) more appropriate.
>
> I disagree. I view it as one hypercall with many sub-ops.

One could make the same argument for sysctl and domctl being a single hypercall 
with different ops.

Regardless, nit as it is, I don't think it matters much either way.

Cheers,
Alejandro

[PATCH v1 02/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL

[Penny Zheng]

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/include/xsm/dummy.h | 7 +++++++
 xen/include/xsm/xsm.h   | 9 +++++++++
 xen/xsm/dummy.c         | 2 ++
 xen/xsm/flask/hooks.c   | 4 ++++
 4 files changed, 22 insertions(+)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index a8d06de6b0..afc54a0b2f 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -180,11 +180,18 @@ static XSM_INLINE int cf_check xsm_domctl(
     }
 }
 
+#ifdef CONFIG_SYSCTL
 static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
 {
     XSM_ASSERT_ACTION(XSM_PRIV);
     return xsm_default_action(action, current->domain, NULL);
 }
+#else
+static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 
 static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 8c33b055fc..276507b515 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -60,7 +60,9 @@ struct xsm_ops {
     int (*sysctl_scheduler_op)(int op);
     int (*set_target)(struct domain *d, struct domain *e);
     int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
+#ifdef CONFIG_SYSCTL
     int (*sysctl)(int cmd);
+#endif
     int (*readconsole)(uint32_t clear);
 
     int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
@@ -259,10 +261,17 @@ static inline int xsm_domctl(xsm_default_t def, struct domain *d,
     return alternative_call(xsm_ops.domctl, d, cmd, ssidref);
 }
 
+#ifdef CONFIG_SYSCTL
 static inline int xsm_sysctl(xsm_default_t def, int cmd)
 {
     return alternative_call(xsm_ops.sysctl, cmd);
 }
+#else
+static inline int xsm_sysctl(xsm_default_t def, int cmd)
+{
+    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 ce6fbdc6c5..0a5fc06bbf 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 389707a164..7c5e7f5879 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

Re: [PATCH v1 02/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -180,11 +180,18 @@ static XSM_INLINE int cf_check xsm_domctl(
>      }
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>  {
>      XSM_ASSERT_ACTION(XSM_PRIV);
>      return xsm_default_action(action, current->domain, NULL);
>  }
> +#else
> +static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

Please can you avoid introducing unnecessary redundancy, by putting the
#ifdef inside the function body here and ...

> @@ -259,10 +261,17 @@ static inline int xsm_domctl(xsm_default_t def, struct domain *d,
>      return alternative_call(xsm_ops.domctl, d, cmd, ssidref);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return alternative_call(xsm_ops.sysctl, cmd);
>  }
> +#else
> +static inline int xsm_sysctl(xsm_default_t def, int cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

... here? Provided these #ifdef-s are actually needed in the first place.
The earlier one I can't really see a need for. If there was a need, at
least the assertion likely also would want to stay outside of the #ifdef.

Jan

Re: [PATCH v1 02/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL

[Alejandro Vallejo]

Hi,

On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
>  xen/include/xsm/dummy.h | 7 +++++++
>  xen/include/xsm/xsm.h   | 9 +++++++++
>  xen/xsm/dummy.c         | 2 ++
>  xen/xsm/flask/hooks.c   | 4 ++++
>  4 files changed, 22 insertions(+)
>
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index a8d06de6b0..afc54a0b2f 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -180,11 +180,18 @@ static XSM_INLINE int cf_check xsm_domctl(
>      }
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>  {
>      XSM_ASSERT_ACTION(XSM_PRIV);
>      return xsm_default_action(action, current->domain, NULL);
>  }
> +#else
> +static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

Doesn't this need to be -ENOSYS instead?

I'd put the ifdefs inside the function (making the signature common) and
then have the body ifdef-ed. But rather than that, I suspect the `else` branch
can just go away because...

>  
>  static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 8c33b055fc..276507b515 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -60,7 +60,9 @@ struct xsm_ops {
>      int (*sysctl_scheduler_op)(int op);
>      int (*set_target)(struct domain *d, struct domain *e);
>      int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
> +#ifdef CONFIG_SYSCTL
>      int (*sysctl)(int cmd);
> +#endif
>      int (*readconsole)(uint32_t clear);

... either you remove this field or make the dummy handler. Doing both seems
redundant.

The dummy handler would return -ENOTSUPP, so the field is benign (in which case
I don't really get why it must go). But if the field is gone, there's no need
for the handler to begin with.

All in all, removing the else branch in xsm_sysctl would make everything
consistent. Same in the files below.

Also, you may want to add the readconsole hook (and its handler) since that's
a specific sysctl that would also be disabled by !CONFIG_SYSCTL.

>  
>      int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
> @@ -259,10 +261,17 @@ static inline int xsm_domctl(xsm_default_t def, struct domain *d,
>      return alternative_call(xsm_ops.domctl, d, cmd, ssidref);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return alternative_call(xsm_ops.sysctl, cmd);
>  }
> +#else
> +static inline int xsm_sysctl(xsm_default_t def, int cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

Same as above

>  
>  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 ce6fbdc6c5..0a5fc06bbf 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c

Same remarks here as in the header.

> @@ -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 389707a164..7c5e7f5879 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,

readconsole ought to be included, imo. And its handler wiped out as well.

>  
>      .evtchn_unbound = flask_evtchn_unbound,

Cheers,
Alejandro

Re: [PATCH v1 02/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL

[Jan Beulich]

On 13.03.2025 12:43, Alejandro Vallejo wrote:
> On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -180,11 +180,18 @@ static XSM_INLINE int cf_check xsm_domctl(
>>      }
>>  }
>>  
>> +#ifdef CONFIG_SYSCTL
>>  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>>  {
>>      XSM_ASSERT_ACTION(XSM_PRIV);
>>      return xsm_default_action(action, current->domain, NULL);
>>  }
>> +#else
>> +static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +#endif
> 
> Doesn't this need to be -ENOSYS instead?

There shouldn't be any ENOSYS outside of the top-level hypercall handlers.
Granted we have many violations thereof, some of them not very reasonable
to fix (for guests looking for the specific but wrong error code).

Jan

Re: [PATCH v1 02/19] xen/xsm: wrap around xsm_sysctl with CONFIG_SYSCTL

[Alejandro Vallejo]

On Thu Mar 13, 2025 at 12:05 PM GMT, Jan Beulich wrote:
> On 13.03.2025 12:43, Alejandro Vallejo wrote:
> > On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> >> --- a/xen/include/xsm/dummy.h
> >> +++ b/xen/include/xsm/dummy.h
> >> @@ -180,11 +180,18 @@ static XSM_INLINE int cf_check xsm_domctl(
> >>      }
> >>  }
> >>  
> >> +#ifdef CONFIG_SYSCTL
> >>  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> >>  {
> >>      XSM_ASSERT_ACTION(XSM_PRIV);
> >>      return xsm_default_action(action, current->domain, NULL);
> >>  }
> >> +#else
> >> +static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> >> +{
> >> +    return -EOPNOTSUPP;
> >> +}
> >> +#endif
> > 
> > Doesn't this need to be -ENOSYS instead?
>
> There shouldn't be any ENOSYS outside of the top-level hypercall handlers.
> Granted we have many violations thereof, some of them not very reasonable
> to fix (for guests looking for the specific but wrong error code).
>
> Jan

That would be the case if the else branches were dropped. They have no use
AFAICS.

Cheers,
Alejandro

[PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

[Penny Zheng]

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>
---
 xen/drivers/char/console.c |  2 ++
 xen/include/xen/console.h  |  8 ++++++++
 xen/include/xsm/dummy.h    | 11 ++++++++---
 xen/include/xsm/xsm.h      | 11 ++++++++---
 xen/xsm/dummy.c            |  2 +-
 xen/xsm/flask/hooks.c      |  4 ++--
 6 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2f028c5d44..6e4f3c4659 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/xen/console.h b/xen/include/xen/console.h
index 83cbc9fbda..e7d5063d82 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -7,12 +7,20 @@
 #ifndef __CONSOLE_H__
 #define __CONSOLE_H__
 
+#include <xen/errno.h>
 #include <xen/inttypes.h>
 #include <xen/ctype.h>
 #include <public/xen.h>
 
 struct xen_sysctl_readconsole;
+#ifdef CONFIG_SYSCTL
 long read_console_ring(struct xen_sysctl_readconsole *op);
+#else
+static inline long read_console_ring(struct xen_sysctl_readconsole *op)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 
 void console_init_preirq(void);
 void console_init_ring(void);
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index afc54a0b2f..35d084aca7 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -186,18 +186,23 @@ static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
     XSM_ASSERT_ACTION(XSM_PRIV);
     return xsm_default_action(action, current->domain, NULL);
 }
+
+static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
+{
+    XSM_ASSERT_ACTION(XSM_HOOK);
+    return xsm_default_action(action, current->domain, NULL);
+}
 #else
 static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
 {
     return -EOPNOTSUPP;
 }
-#endif
 
 static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
 {
-    XSM_ASSERT_ACTION(XSM_HOOK);
-    return xsm_default_action(action, current->domain, NULL);
+    return -EOPNOTSUPP;
 }
+#endif /* CONFIG_SYSCTL */
 
 static XSM_INLINE int cf_check xsm_alloc_security_domain(struct domain *d)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 276507b515..d322740de1 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -62,8 +62,8 @@ struct xsm_ops {
     int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
 #ifdef CONFIG_SYSCTL
     int (*sysctl)(int cmd);
-#endif
     int (*readconsole)(uint32_t clear);
+#endif
 
     int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
     int (*evtchn_interdomain)(struct domain *d1, struct evtchn *chn1,
@@ -266,17 +266,22 @@ static inline int xsm_sysctl(xsm_default_t def, int cmd)
 {
     return alternative_call(xsm_ops.sysctl, cmd);
 }
+
+static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
+{
+    return alternative_call(xsm_ops.readconsole, clear);
+}
 #else
 static inline int xsm_sysctl(xsm_default_t def, int cmd)
 {
     return -EOPNOTSUPP;
 }
-#endif
 
 static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
 {
-    return alternative_call(xsm_ops.readconsole, clear);
+    return -EOPNOTSUPP;
 }
+#endif
 
 static inline int xsm_evtchn_unbound(
     xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t id2)
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 0a5fc06bbf..4c97db0c48 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 7c5e7f5879..7c46657d97 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

Re: [PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

[Alejandro Vallejo]

Hi,

Ok, so readconsole is done here. I see how if you're also removing the console
handler for the sysctl that's a bit unwiledly to do in one go.

I think my earlier remarks still hold in terms of removal of else branches of
ifdefs.

On Wed Mar 12, 2025 at 4:06 AM GMT, 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>
> ---
>  xen/drivers/char/console.c |  2 ++
>  xen/include/xen/console.h  |  8 ++++++++
>  xen/include/xsm/dummy.h    | 11 ++++++++---
>  xen/include/xsm/xsm.h      | 11 ++++++++---
>  xen/xsm/dummy.c            |  2 +-
>  xen/xsm/flask/hooks.c      |  4 ++--
>  6 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 2f028c5d44..6e4f3c4659 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/xen/console.h b/xen/include/xen/console.h
> index 83cbc9fbda..e7d5063d82 100644
> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -7,12 +7,20 @@
>  #ifndef __CONSOLE_H__
>  #define __CONSOLE_H__
>  
> +#include <xen/errno.h>
>  #include <xen/inttypes.h>
>  #include <xen/ctype.h>
>  #include <public/xen.h>
>  
>  struct xen_sysctl_readconsole;

That forward declaration should probably be inside the ifdef

> +#ifdef CONFIG_SYSCTL
>  long read_console_ring(struct xen_sysctl_readconsole *op);
> +#else
> +static inline long read_console_ring(struct xen_sysctl_readconsole *op)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

This is only called from sysctl.c, which will be compiled out. Why is the else
needed?

>  
>  void console_init_preirq(void);
>  void console_init_ring(void);
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index afc54a0b2f..35d084aca7 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -186,18 +186,23 @@ static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>      XSM_ASSERT_ACTION(XSM_PRIV);
>      return xsm_default_action(action, current->domain, NULL);
>  }
> +
> +static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
> +{
> +    XSM_ASSERT_ACTION(XSM_HOOK);
> +    return xsm_default_action(action, current->domain, NULL);
> +}
>  #else
>  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)
>  {
>      return -EOPNOTSUPP;
>  }
> -#endif
>  
>  static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
>  {
> -    XSM_ASSERT_ACTION(XSM_HOOK);
> -    return xsm_default_action(action, current->domain, NULL);
> +    return -EOPNOTSUPP;
>  }
> +#endif /* CONFIG_SYSCTL */
>  
>  static XSM_INLINE int cf_check xsm_alloc_security_domain(struct domain *d)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 276507b515..d322740de1 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -62,8 +62,8 @@ struct xsm_ops {
>      int (*domctl)(struct domain *d, unsigned int cmd, uint32_t ssidref);
>  #ifdef CONFIG_SYSCTL
>      int (*sysctl)(int cmd);
> -#endif
>      int (*readconsole)(uint32_t clear);
> +#endif
>  
>      int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
>      int (*evtchn_interdomain)(struct domain *d1, struct evtchn *chn1,
> @@ -266,17 +266,22 @@ static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return alternative_call(xsm_ops.sysctl, cmd);
>  }
> +
> +static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
> +{
> +    return alternative_call(xsm_ops.readconsole, clear);
> +}
>  #else
>  static inline int xsm_sysctl(xsm_default_t def, int cmd)
>  {
>      return -EOPNOTSUPP;
>  }
> -#endif
>  
>  static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
>  {
> -    return alternative_call(xsm_ops.readconsole, clear);
> +    return -EOPNOTSUPP;
>  }
> +#endif
>  
>  static inline int xsm_evtchn_unbound(
>      xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t id2)
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 0a5fc06bbf..4c97db0c48 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 7c5e7f5879..7c46657d97 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,

Otherwise, same remarks as in the sysctl hooks for xsm.

Cheers,
Alejandro

RE: [PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

[Penny, Zheng]

[Public]

Hi,

Sorry for the late reply, got side-tracked for a while

> -----Original Message-----
> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Sent: Thursday, March 13, 2025 8:03 PM
> To: Penny, Zheng <penny.zheng@amd.com>; xen-devel@lists.xenproject.org
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Daniel P. Smith
> <dpsmith@apertussolutions.com>
> Subject: Re: [PATCH v1 03/19] xen/sysctl: wrap around
> XEN_SYSCTL_readconsole
>
> Hi,
>
> Ok, so readconsole is done here. I see how if you're also removing the console
> handler for the sysctl that's a bit unwiledly to do in one go.
>
> I think my earlier remarks still hold in terms of removal of else branches of ifdefs.
>
> On Wed Mar 12, 2025 at 4:06 AM GMT, 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>
> > ---
> >  xen/drivers/char/console.c |  2 ++
> >  xen/include/xen/console.h  |  8 ++++++++
> >  xen/include/xsm/dummy.h    | 11 ++++++++---
> >  xen/include/xsm/xsm.h      | 11 ++++++++---
> >  xen/xsm/dummy.c            |  2 +-
> >  xen/xsm/flask/hooks.c      |  4 ++--
> >  6 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 2f028c5d44..6e4f3c4659 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/xen/console.h b/xen/include/xen/console.h
> > index 83cbc9fbda..e7d5063d82 100644
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -7,12 +7,20 @@
> >  #ifndef __CONSOLE_H__
> >  #define __CONSOLE_H__
> >
> > +#include <xen/errno.h>
> >  #include <xen/inttypes.h>
> >  #include <xen/ctype.h>
> >  #include <public/xen.h>
> >
> >  struct xen_sysctl_readconsole;
>
> That forward declaration should probably be inside the ifdef
>
> > +#ifdef CONFIG_SYSCTL
> >  long read_console_ring(struct xen_sysctl_readconsole *op);
> > +#else
> > +static inline long read_console_ring(struct xen_sysctl_readconsole
> > +*op) {
> > +    return -EOPNOTSUPP;
> > +}
> > +#endif
>
> This is only called from sysctl.c, which will be compiled out. Why is the else
> needed?
>

Because I wrapped the sysctl.c in the last commit.
If removing the else condition here, the compilation will fail on this commit.
So either I add #ifdef into read_console_ring function body, or in the last commit,
I draw back all these unnecessary else conditions, or combine all commits into one
Any preference? Or any other suggestion?

> >
> >  void console_init_preirq(void);
> >  void console_init_ring(void);
> > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index
> > afc54a0b2f..35d084aca7 100644
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -186,18 +186,23 @@ static XSM_INLINE int cf_check
> xsm_sysctl(XSM_DEFAULT_ARG int cmd)
> >      XSM_ASSERT_ACTION(XSM_PRIV);
> >      return xsm_default_action(action, current->domain, NULL);  }
> > +
> > +static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG
> > +uint32_t clear) {
> > +    XSM_ASSERT_ACTION(XSM_HOOK);
> > +    return xsm_default_action(action, current->domain, NULL); }
> >  #else
> >  static XSM_INLINE int cf_check xsm_sysctl(XSM_DEFAULT_ARG int cmd)  {
> >      return -EOPNOTSUPP;
> >  }
> > -#endif
> >
> >  static XSM_INLINE int cf_check xsm_readconsole(XSM_DEFAULT_ARG
> > uint32_t clear)  {
> > -    XSM_ASSERT_ACTION(XSM_HOOK);
> > -    return xsm_default_action(action, current->domain, NULL);
> > +    return -EOPNOTSUPP;
> >  }
> > +#endif /* CONFIG_SYSCTL */
> >
> >  static XSM_INLINE int cf_check xsm_alloc_security_domain(struct
> > domain *d)  { diff --git a/xen/include/xsm/xsm.h
> > b/xen/include/xsm/xsm.h index 276507b515..d322740de1 100644
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -62,8 +62,8 @@ struct xsm_ops {
> >      int (*domctl)(struct domain *d, unsigned int cmd, uint32_t
> > ssidref);  #ifdef CONFIG_SYSCTL
> >      int (*sysctl)(int cmd);
> > -#endif
> >      int (*readconsole)(uint32_t clear);
> > +#endif
> >
> >      int (*evtchn_unbound)(struct domain *d, struct evtchn *chn, domid_t id2);
> >      int (*evtchn_interdomain)(struct domain *d1, struct evtchn *chn1,
> > @@ -266,17 +266,22 @@ static inline int xsm_sysctl(xsm_default_t def,
> > int cmd)  {
> >      return alternative_call(xsm_ops.sysctl, cmd);  }
> > +
> > +static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
> > +{
> > +    return alternative_call(xsm_ops.readconsole, clear); }
> >  #else
> >  static inline int xsm_sysctl(xsm_default_t def, int cmd)  {
> >      return -EOPNOTSUPP;
> >  }
> > -#endif
> >
> >  static inline int xsm_readconsole(xsm_default_t def, uint32_t clear)
> > {
> > -    return alternative_call(xsm_ops.readconsole, clear);
> > +    return -EOPNOTSUPP;
> >  }
> > +#endif
> >
> >  static inline int xsm_evtchn_unbound(
> >      xsm_default_t def, struct domain *d1, struct evtchn *chn, domid_t
> > id2) diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index
> > 0a5fc06bbf..4c97db0c48 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
> > 7c5e7f5879..7c46657d97 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,
>
> Otherwise, same remarks as in the sysctl hooks for xsm.
>
> Cheers,
> Alejandro

Re: [PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

[Jan Beulich]

On 20.03.2025 09:02, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>> Sent: Thursday, March 13, 2025 8:03 PM
>>
>> Ok, so readconsole is done here. I see how if you're also removing the console
>> handler for the sysctl that's a bit unwiledly to do in one go.
>>
>> I think my earlier remarks still hold in terms of removal of else branches of ifdefs.
>>
>> On Wed Mar 12, 2025 at 4:06 AM GMT, 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>
>>> ---
>>>  xen/drivers/char/console.c |  2 ++
>>>  xen/include/xen/console.h  |  8 ++++++++
>>>  xen/include/xsm/dummy.h    | 11 ++++++++---
>>>  xen/include/xsm/xsm.h      | 11 ++++++++---
>>>  xen/xsm/dummy.c            |  2 +-
>>>  xen/xsm/flask/hooks.c      |  4 ++--
>>>  6 files changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>>> index 2f028c5d44..6e4f3c4659 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/xen/console.h b/xen/include/xen/console.h
>>> index 83cbc9fbda..e7d5063d82 100644
>>> --- a/xen/include/xen/console.h
>>> +++ b/xen/include/xen/console.h
>>> @@ -7,12 +7,20 @@
>>>  #ifndef __CONSOLE_H__
>>>  #define __CONSOLE_H__
>>>
>>> +#include <xen/errno.h>
>>>  #include <xen/inttypes.h>
>>>  #include <xen/ctype.h>
>>>  #include <public/xen.h>
>>>
>>>  struct xen_sysctl_readconsole;
>>
>> That forward declaration should probably be inside the ifdef
>>
>>> +#ifdef CONFIG_SYSCTL
>>>  long read_console_ring(struct xen_sysctl_readconsole *op);
>>> +#else
>>> +static inline long read_console_ring(struct xen_sysctl_readconsole
>>> +*op) {
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +#endif
>>
>> This is only called from sysctl.c, which will be compiled out. Why is the else
>> needed?
>>
> 
> Because I wrapped the sysctl.c in the last commit.
> If removing the else condition here, the compilation will fail on this commit.
> So either I add #ifdef into read_console_ring function body, or in the last commit,
> I draw back all these unnecessary else conditions, or combine all commits into one
> Any preference? Or any other suggestion?

Munging everything in a single commit may yield unwieldily big a result. Transiently
adding #ifdef in sysctl.c would seem like the cleanest approach to me. In the final
commit it'll (hopefully) be obvious enough then why all of the #ifdef-s are dropped
again.

Jan

RE: [PATCH v1 03/19] xen/sysctl: wrap around XEN_SYSCTL_readconsole

[Penny, Zheng]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, March 20, 2025 4:47 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Daniel
> P. Smith <dpsmith@apertussolutions.com>; Alejandro Vallejo
> <alejandro.vallejo@cloud.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1 03/19] xen/sysctl: wrap around
> XEN_SYSCTL_readconsole
>
> On 20.03.2025 09:02, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >> Sent: Thursday, March 13, 2025 8:03 PM
> >>
> >> Ok, so readconsole is done here. I see how if you're also removing
> >> the console handler for the sysctl that's a bit unwiledly to do in one go.
> >>
> >> I think my earlier remarks still hold in terms of removal of else branches of ifdefs.
> >>
> >> On Wed Mar 12, 2025 at 4:06 AM GMT, 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>
> >>> ---
> >>>  xen/drivers/char/console.c |  2 ++
> >>>  xen/include/xen/console.h  |  8 ++++++++
> >>>  xen/include/xsm/dummy.h    | 11 ++++++++---
> >>>  xen/include/xsm/xsm.h      | 11 ++++++++---
> >>>  xen/xsm/dummy.c            |  2 +-
> >>>  xen/xsm/flask/hooks.c      |  4 ++--
> >>>  6 files changed, 29 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> >>> index 2f028c5d44..6e4f3c4659 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/xen/console.h b/xen/include/xen/console.h
> >>> index 83cbc9fbda..e7d5063d82 100644
> >>> --- a/xen/include/xen/console.h
> >>> +++ b/xen/include/xen/console.h
> >>> @@ -7,12 +7,20 @@
> >>>  #ifndef __CONSOLE_H__
> >>>  #define __CONSOLE_H__
> >>>
> >>> +#include <xen/errno.h>
> >>>  #include <xen/inttypes.h>
> >>>  #include <xen/ctype.h>
> >>>  #include <public/xen.h>
> >>>
> >>>  struct xen_sysctl_readconsole;
> >>
> >> That forward declaration should probably be inside the ifdef
> >>
> >>> +#ifdef CONFIG_SYSCTL
> >>>  long read_console_ring(struct xen_sysctl_readconsole *op);
> >>> +#else
> >>> +static inline long read_console_ring(struct xen_sysctl_readconsole
> >>> +*op) {
> >>> +    return -EOPNOTSUPP;
> >>> +}
> >>> +#endif
> >>
> >> This is only called from sysctl.c, which will be compiled out. Why is
> >> the else needed?
> >>
> >
> > Because I wrapped the sysctl.c in the last commit.
> > If removing the else condition here, the compilation will fail on this commit.
> > So either I add #ifdef into read_console_ring function body, or in the
> > last commit, I draw back all these unnecessary else conditions, or
> > combine all commits into one Any preference? Or any other suggestion?
>
> Munging everything in a single commit may yield unwieldily big a result. Transiently
> adding #ifdef in sysctl.c would seem like the cleanest approach to me. In the final
> commit it'll (hopefully) be obvious enough then why all of the #ifdef-s are dropped
> again.
>

Understood, I'll add #ifdef in sysctl.c and remove them all in the last, and also add
description why we drop them in the commit message

> Jan

[PATCH v1 04/19] xen/sysctl: make CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL

[Penny Zheng]

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>
---
 xen/common/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 72e1d7ea97..c5f4192b60 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

Re: [PATCH v1 04/19] xen/sysctl: make CONFIG_TRACEBUFFER depend on CONFIG_SYSCTL

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> 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 72e1d7ea97..c5f4192b60 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
> 

[PATCH v1 05/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id

[Penny Zheng]

The following function shall be wrapped:
- scheduler_id

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/common/sched/core.c | 2 ++
 xen/include/xen/sched.h | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d6296d99fd..ea452d8b3e 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2052,11 +2052,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/include/xen/sched.h b/xen/include/xen/sched.h
index 559d201e0c..9bdeb85aa4 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -849,7 +849,14 @@ int  sched_init_domain(struct domain *d, unsigned int poolid);
 void sched_destroy_domain(struct domain *d);
 long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op);
 long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
+#ifdef CONFIG_SYSCTL
 int  scheduler_id(void);
+#else
+static inline int scheduler_id(void)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 
 /*
  * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
-- 
2.34.1

Re: [PATCH v1 05/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> 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/include/xen/sched.h | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index d6296d99fd..ea452d8b3e 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -2052,11 +2052,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/include/xen/sched.h b/xen/include/xen/sched.h
> index 559d201e0c..9bdeb85aa4 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -849,7 +849,14 @@ int  sched_init_domain(struct domain *d, unsigned int poolid);
>  void sched_destroy_domain(struct domain *d);
>  long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op);
>  long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
> +#ifdef CONFIG_SYSCTL
>  int  scheduler_id(void);
> +#else
> +static inline int scheduler_id(void)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
>  
>  /*
>   * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
> -- 
> 2.34.1
> 

Re: [PATCH v1 05/19] xen/sysctl: wrap around XEN_SYSCTL_sched_id

[Alejandro Vallejo]

On Wed Mar 12, 2025 at 4:06 AM GMT, Penny Zheng wrote:
> The following function shall be wrapped:
> - scheduler_id
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
>  xen/common/sched/core.c | 2 ++
>  xen/include/xen/sched.h | 7 +++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index d6296d99fd..ea452d8b3e 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -2052,11 +2052,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/include/xen/sched.h b/xen/include/xen/sched.h
> index 559d201e0c..9bdeb85aa4 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -849,7 +849,14 @@ int  sched_init_domain(struct domain *d, unsigned int poolid);
>  void sched_destroy_domain(struct domain *d);
>  long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op);
>  long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
> +#ifdef CONFIG_SYSCTL
>  int  scheduler_id(void);
> +#else
> +static inline int scheduler_id(void)
> +{
> +    return -EOPNOTSUPP;
> +}

Is this ever used when sysctl.c is compiled out?

> +#endif
>  
>  /*
>   * sched_get_id_by_name - retrieves a scheduler id given a scheduler name

Cheers,
Alejandro

[PATCH v1 06/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op

[Penny Zheng]

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>
---
 xen/common/perfc.c      | 2 ++
 xen/include/xen/perfc.h | 8 ++++++++
 2 files changed, 10 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/include/xen/perfc.h b/xen/include/xen/perfc.h
index bf0eb032f7..6cc8af12d5 100644
--- a/xen/include/xen/perfc.h
+++ b/xen/include/xen/perfc.h
@@ -5,6 +5,7 @@
 
 #include <xen/macros.h>
 #include <xen/percpu.h>
+#include <xen/errno.h>
 
 /*
  * NOTE: new counters must be defined in perfc_defn.h
@@ -92,7 +93,14 @@ DECLARE_PER_CPU(perfc_t[NUM_PERFCOUNTERS], perfcounters);
 #endif
 
 struct xen_sysctl_perfc_op;
+#ifdef CONFIG_SYSCTL
 int perfc_control(struct xen_sysctl_perfc_op *pc);
+#else
+static inline int perfc_control(struct xen_sysctl_perfc_op *pc)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 
 extern void cf_check perfc_printall(unsigned char key);
 extern void cf_check perfc_reset(unsigned char key);
-- 
2.34.1

Re: [PATCH v1 06/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op

[Stefano Stabellini]

On Wed, 12 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>
> ---
>  xen/common/perfc.c      | 2 ++
>  xen/include/xen/perfc.h | 8 ++++++++
>  2 files changed, 10 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

I think in this case it would be best to make CONFIG_PERF_COUNTERS
depending on CONFIG_SYSCTL. The consequence is that without
CONFIG_SYSCTL, xen/common/perfc.c will be disabled in the build.



>  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/include/xen/perfc.h b/xen/include/xen/perfc.h
> index bf0eb032f7..6cc8af12d5 100644
> --- a/xen/include/xen/perfc.h
> +++ b/xen/include/xen/perfc.h
> @@ -5,6 +5,7 @@
>  
>  #include <xen/macros.h>
>  #include <xen/percpu.h>
> +#include <xen/errno.h>
>  
>  /*
>   * NOTE: new counters must be defined in perfc_defn.h
> @@ -92,7 +93,14 @@ DECLARE_PER_CPU(perfc_t[NUM_PERFCOUNTERS], perfcounters);
>  #endif
>  
>  struct xen_sysctl_perfc_op;
> +#ifdef CONFIG_SYSCTL
>  int perfc_control(struct xen_sysctl_perfc_op *pc);
> +#else
> +static inline int perfc_control(struct xen_sysctl_perfc_op *pc)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
>  
>  extern void cf_check perfc_printall(unsigned char key);
>  extern void cf_check perfc_reset(unsigned char key);
> -- 
> 2.34.1
> 

Re: [PATCH v1 06/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op

[Jan Beulich]

On 13.03.2025 00:12, Stefano Stabellini wrote:
> On Wed, 12 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>
>> ---
>>  xen/common/perfc.c      | 2 ++
>>  xen/include/xen/perfc.h | 8 ++++++++
>>  2 files changed, 10 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
> 
> I think in this case it would be best to make CONFIG_PERF_COUNTERS
> depending on CONFIG_SYSCTL. The consequence is that without
> CONFIG_SYSCTL, xen/common/perfc.c will be disabled in the build.

But perf counters can be used without sysctl, via the dedicated debug
keys.

Jan

Re: [PATCH v1 06/19] xen/sysctl: wrap around XEN_SYSCTL_perfc_op

[Stefano Stabellini]

On Thu, 13 Mar 2025, Jan Beulich wrote:
> On 13.03.2025 00:12, Stefano Stabellini wrote:
> > On Wed, 12 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>
> >> ---
> >>  xen/common/perfc.c      | 2 ++
> >>  xen/include/xen/perfc.h | 8 ++++++++
> >>  2 files changed, 10 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
> > 
> > I think in this case it would be best to make CONFIG_PERF_COUNTERS
> > depending on CONFIG_SYSCTL. The consequence is that without
> > CONFIG_SYSCTL, xen/common/perfc.c will be disabled in the build.
> 
> But perf counters can be used without sysctl, via the dedicated debug
> keys.

I checked the code, and you are right, please ignore my comment here,
and also the similar comment to the next patch

[PATCH v1 07/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op

[Penny Zheng]

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>
---
 xen/common/spinlock.c      | 2 ++
 xen/include/xen/spinlock.h | 7 +++++++
 2 files changed, 9 insertions(+)

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/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index ca9d8c7ec0..7f2a814ee0 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -164,7 +164,14 @@ void _lock_profile_deregister_struct(int32_t type,
 #define lock_profile_deregister_struct(type, ptr)                             \
     _lock_profile_deregister_struct(type, &((ptr)->profile_head))
 
+#ifdef CONFIG_SYSCTL
 extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
+#else
+static inline int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 extern void cf_check spinlock_profile_printall(unsigned char key);
 extern void cf_check spinlock_profile_reset(unsigned char key);
 
-- 
2.34.1

Re: [PATCH v1 07/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op

[Stefano Stabellini]

On Wed, 12 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>


Similarly to the previous patch, could be make CONFIG_DEBUG_LOCK_PROFILE
depend on CONFIG_SYSCTL? 


> ---
>  xen/common/spinlock.c      | 2 ++
>  xen/include/xen/spinlock.h | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> 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/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index ca9d8c7ec0..7f2a814ee0 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -164,7 +164,14 @@ void _lock_profile_deregister_struct(int32_t type,
>  #define lock_profile_deregister_struct(type, ptr)                             \
>      _lock_profile_deregister_struct(type, &((ptr)->profile_head))
>  
> +#ifdef CONFIG_SYSCTL
>  extern int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc);
> +#else
> +static inline int spinlock_profile_control(struct xen_sysctl_lockprof_op *pc)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
>  extern void cf_check spinlock_profile_printall(unsigned char key);
>  extern void cf_check spinlock_profile_reset(unsigned char key);
>  
> -- 
> 2.34.1
> 

Re: [PATCH v1 07/19] xen/sysctl: wrap around XEN_SYSCTL_lockprof_op

[Jan Beulich]

On 13.03.2025 00:13, Stefano Stabellini wrote:
> On Wed, 12 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>
> 
> Similarly to the previous patch, could be make CONFIG_DEBUG_LOCK_PROFILE
> depend on CONFIG_SYSCTL? 

And like there this also has dedicated debug keys.

Jan

[PATCH v1 08/19] xen/sysctl: introduce CONFIG_PM_STATISTIC

[Penny Zheng]

This commit introduces CONFIG_PM_STATISTIC for wrapping all operations
regarding performance management statistic operations.
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().

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 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/acpi/pmstat.c                    | 192 ++++++++++++++++---
 xen/drivers/cpufreq/cpufreq.c                |  31 +++
 xen/drivers/cpufreq/cpufreq_misc_governors.c |   2 +
 xen/drivers/cpufreq/cpufreq_ondemand.c       |   2 +
 xen/drivers/cpufreq/utility.c                | 164 +---------------
 xen/include/acpi/cpufreq/cpufreq.h           |   6 +
 xen/include/acpi/cpufreq/processor_perf.h    |  12 +-
 xen/include/xen/acpi.h                       |   2 +
 xen/include/xen/pmstat.h                     |   2 +
 15 files changed, 238 insertions(+), 198 deletions(-)

diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 420198406d..9ea52def42 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_STATISTIC
 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_STATISTIC */
 
 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..d1309e3e0a 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_STATISTIC
 /*
  * 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_STATISTIC */
 
 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_STATISTIC
     .update = hwp_cpufreq_update,
+#endif
 };
 
+#ifdef CONFIG_PM_STATISTIC
 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_STATISTIC */
 
 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..0cdeeef883 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_STATISTIC
 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_STATISTIC */
 
 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_STATISTIC
     .update = powernow_cpufreq_update
+#endif
 };
 
 unsigned int __init powernow_register_driver(void)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c5f4192b60..ffc6b9d4df 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -107,6 +107,11 @@ config NEEDS_LIBELF
 config NUMA
 	bool
 
+config PM_STATISTIC
+        bool "Enable Performance Management Statistic Operations"
+        depends on ACPI && HAS_CPUFREQ
+        default y
+
 config STATIC_MEMORY
 	bool "Static Allocation Support (UNSUPPORTED)" if UNSUPPORTED
 	depends on DOM0LESS_BOOT
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c2d99ae12e..3d605633cc 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -170,7 +170,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_STATISTIC
     case XEN_SYSCTL_get_pmstat:
         ret = do_get_pm_info(&op->u.get_pmstat);
         break;
@@ -180,7 +180,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         if ( ret == -EAGAIN )
             copyback = 1;
         break;
-#endif
+#endif /* CONFIG_PM_STATISTIC */
 
     case XEN_SYSCTL_page_offline_op:
     {
diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
index 2fc5230253..70156ee0a4 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_STATISTIC) += pmstat.o
 
 obj-$(CONFIG_X86) += hwregs.o
 obj-$(CONFIG_X86) += reboot.o
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index df309e27b4..58cccd589b 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -43,6 +43,167 @@
 
 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;
+    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 +683,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/cpufreq_misc_governors.c b/xen/drivers/cpufreq/cpufreq_misc_governors.c
index 0327fad23b..efdc954a4e 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_STATISTIC
 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_STATISTIC */
 
 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..ec1e605a59 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_STATISTIC
 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_STATISTIC */
 
 static void dbs_check_cpu(struct cpu_dbs_info_s *this_dbs_info)
 {
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index e690a484f1..2eb3314694 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                         *
  *********************************************************************/
@@ -386,6 +224,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag)
     return policy->cur;
 }
 
+#ifdef CONFIG_PM_STATISTIC
 int cpufreq_update_turbo(unsigned int cpu, int new_state)
 {
     struct cpufreq_policy *policy;
@@ -417,6 +256,7 @@ int cpufreq_update_turbo(unsigned int cpu, int new_state)
 
     return ret;
 }
+#endif /* CONFIG_PM_STATISTIC */
 
 
 int cpufreq_get_turbo_status(unsigned int cpu)
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index a3c84143af..bfef9bb523 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -167,7 +167,9 @@ struct cpufreq_driver {
     int    (*init)(struct cpufreq_policy *policy);
     int    (*verify)(struct cpufreq_policy *policy);
     int    (*setpolicy)(struct cpufreq_policy *policy);
+#ifdef CONFIG_PM_STATISTIC
     int    (*update)(unsigned int cpu, struct cpufreq_policy *policy);
+#endif
     int    (*target)(struct cpufreq_policy *policy,
                      unsigned int target_freq,
                      unsigned int relation);
@@ -238,6 +240,7 @@ struct cpu_dbs_info_s {
     int8_t stoppable;
 };
 
+#ifdef CONFIG_PM_STATISTIC
 int get_cpufreq_ondemand_para(uint32_t *sampling_rate_max,
                               uint32_t *sampling_rate_min,
                               uint32_t *sampling_rate,
@@ -246,6 +249,7 @@ int write_ondemand_sampling_rate(unsigned int sampling_rate);
 int write_ondemand_up_threshold(unsigned int up_threshold);
 
 int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq);
+#endif /* CONFIG_PM_STATISTIC */
 
 void cpufreq_dbs_timer_suspend(void);
 void cpufreq_dbs_timer_resume(void);
@@ -270,10 +274,12 @@ bool hwp_active(void);
 static inline bool hwp_active(void) { return false; }
 #endif
 
+#ifdef CONFIG_PM_STATISTIC
 int get_hwp_para(unsigned int cpu,
                  struct xen_cppc_para *cppc_para);
 int set_hwp_para(struct cpufreq_policy *policy,
                  struct xen_set_cppc_para *set_cppc);
+#endif /* CONFIG_PM_STATISTIC */
 
 int acpi_cpufreq_register(void);
 
diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/acpi/cpufreq/processor_perf.h
index 301104e16f..2f5a4d2b7b 100644
--- a/xen/include/acpi/cpufreq/processor_perf.h
+++ b/xen/include/acpi/cpufreq/processor_perf.h
@@ -9,11 +9,19 @@
 
 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);
+#ifdef CONFIG_PM_STATISTIC
 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);
+#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_STATISTIC */
 
 int  cpufreq_limit_change(unsigned int cpu);
 
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index bc4818c943..0bbbc3c15d 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -158,6 +158,7 @@ int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
 extern unsigned int max_cstate;
 extern unsigned int max_csubstate;
 
+#ifdef CONFIG_PM_STATISTIC
 static inline unsigned int acpi_get_cstate_limit(void)
 {
 	return max_cstate;
@@ -177,6 +178,7 @@ static inline void acpi_set_csubstate_limit(unsigned int new_limit)
 {
 	max_csubstate = new_limit;
 }
+#endif /* CONFIG_PM_STATISTIC */
 
 #else
 static inline unsigned int acpi_get_cstate_limit(void) { return 0; }
diff --git a/xen/include/xen/pmstat.h b/xen/include/xen/pmstat.h
index 8350403e95..ad72665025 100644
--- a/xen/include/xen/pmstat.h
+++ b/xen/include/xen/pmstat.h
@@ -15,11 +15,13 @@ struct compat_processor_power;
 long compat_set_cx_pminfo(uint32_t acpi_id, struct compat_processor_power *power);
 #endif
 
+#ifdef CONFIG_PM_STATISTIC
 uint32_t pmstat_get_cx_nr(unsigned int cpu);
 int pmstat_get_cx_stat(unsigned int cpu, struct pm_cx_stat *stat);
 int pmstat_reset_cx_stat(unsigned int cpu);
 
 int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
 int do_pm_op(struct xen_sysctl_pm_op *op);
+#endif /* CONFIG_PM_STATISTIC */
 
 #endif /* __XEN_PMSTAT_H_ */
-- 
2.34.1

Re: [PATCH v1 08/19] xen/sysctl: introduce CONFIG_PM_STATISTIC

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> This commit introduces CONFIG_PM_STATISTIC for wrapping all operations
> regarding performance management statistic operations.

Please can descriptions not use "This patch ..." or "This commit ..." or
anyhting like these?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -107,6 +107,11 @@ config NEEDS_LIBELF
>  config NUMA
>  	bool
>  
> +config PM_STATISTIC

I think this wants to use plural, i.e. PM_STATISTICS or PM_STATS.

> +        bool "Enable Performance Management Statistic Operations"

"Enable Performance Management Statistics" ?

> +        depends on ACPI && HAS_CPUFREQ
> +        default y

Nit: Indentation.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -170,7 +170,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_STATISTIC
>      case XEN_SYSCTL_get_pmstat:
>          ret = do_get_pm_info(&op->u.get_pmstat);
>          break;
> @@ -180,7 +180,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>          if ( ret == -EAGAIN )
>              copyback = 1;
>          break;
> -#endif
> +#endif /* CONFIG_PM_STATISTIC */
>  
>      case XEN_SYSCTL_page_offline_op:
>      {
> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
> index 2fc5230253..70156ee0a4 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_STATISTIC) += pmstat.o
>  
>  obj-$(CONFIG_X86) += hwregs.o
>  obj-$(CONFIG_X86) += reboot.o
> diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
> index df309e27b4..58cccd589b 100644
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -43,6 +43,167 @@
>  
>  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;
> +    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);
> +}

The want/need for this code movement wants mentioning in the description imo.
It may even be desirable to split this out. The more that while you move it,
it would be quite nice if various style corrections could be applied.

> @@ -522,34 +683,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;
> -}
> --- 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;
> +}

Same here - looks pretty independent.

> --- a/xen/include/acpi/cpufreq/processor_perf.h
> +++ b/xen/include/acpi/cpufreq/processor_perf.h
> @@ -9,11 +9,19 @@
>  
>  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);
> +#ifdef CONFIG_PM_STATISTIC
>  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);
> +#else
> +static inline  void cpufreq_statistic_update(unsigned int cpu, uint8_t from,
> +                                             uint8_t to) {};

Nit: Stray semicolon. I'm also uncertain whether we like this kind of
formatting (i.e. if already things don't fit on a single line, I'm unsure
whether we like then having the braces not on their own line).

> +static inline int cpufreq_statistic_init(unsigned int cpu)
> +{
> +    return 0;
> +}
> +static inline void cpufreq_statistic_exit(unsigned int cpu) {};

Stray semicolon again.

> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -158,6 +158,7 @@ int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
>  extern unsigned int max_cstate;
>  extern unsigned int max_csubstate;
>  
> +#ifdef CONFIG_PM_STATISTIC
>  static inline unsigned int acpi_get_cstate_limit(void)
>  {
>  	return max_cstate;
> @@ -177,6 +178,7 @@ static inline void acpi_set_csubstate_limit(unsigned int new_limit)
>  {
>  	max_csubstate = new_limit;
>  }
> +#endif /* CONFIG_PM_STATISTIC */

Is this really necessary? Afaict these inline functions would still
compile fine; they'd merely end up without any user for now. (Not sure
what Misra's take is on unused inline functions.)

> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -15,11 +15,13 @@ struct compat_processor_power;
>  long compat_set_cx_pminfo(uint32_t acpi_id, struct compat_processor_power *power);
>  #endif
>  
> +#ifdef CONFIG_PM_STATISTIC
>  uint32_t pmstat_get_cx_nr(unsigned int cpu);
>  int pmstat_get_cx_stat(unsigned int cpu, struct pm_cx_stat *stat);
>  int pmstat_reset_cx_stat(unsigned int cpu);
>  
>  int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
>  int do_pm_op(struct xen_sysctl_pm_op *op);
> +#endif /* CONFIG_PM_STATISTIC */

Similarly leaving these declarations visible isn't going to be a problem.
We do so quite frequently elsewhere.

Jan

[PATCH v1 09/19] xen/sysctl: make CONFIG_PM_STATISTIC depend on CONFIG_SYSCTL

[Penny Zheng]

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/common/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index ffc6b9d4df..fbaca097ff 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -109,7 +109,7 @@ config NUMA
 
 config PM_STATISTIC
         bool "Enable Performance Management Statistic Operations"
-        depends on ACPI && HAS_CPUFREQ
+        depends on ACPI && HAS_CPUFREQ && SYSCTL
         default y
 
 config STATIC_MEMORY
-- 
2.34.1

Re: [PATCH v1 09/19] xen/sysctl: make CONFIG_PM_STATISTIC depend on CONFIG_SYSCTL

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
>  xen/common/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index ffc6b9d4df..fbaca097ff 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -109,7 +109,7 @@ config NUMA
>  
>  config PM_STATISTIC
>          bool "Enable Performance Management Statistic Operations"
> -        depends on ACPI && HAS_CPUFREQ
> +        depends on ACPI && HAS_CPUFREQ && SYSCTL
>          default y
>  
>  config STATIC_MEMORY

Better fold into the previous patch?

Jan

[PATCH v1 10/19] xen/page: fix return type of online_page()

[Penny Zheng]

This commit fixes return type of online_page(), which shall be int
to include correct error value.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/common/page_alloc.c | 2 +-
 xen/include/xen/mm.h    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 1bf070c8c5..b5ec4bda7d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1764,7 +1764,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
  *   The caller should make sure end_pfn <= max_page,
  *   if not, expand_pages() should be called prior to online_page().
  */
-unsigned int online_page(mfn_t mfn, uint32_t *status)
+int online_page(mfn_t mfn, uint32_t *status)
 {
     unsigned long x, nx, y;
     struct page_info *pg;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 16f733281a..ae1c48a615 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -144,7 +144,7 @@ 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))
-unsigned int online_page(mfn_t mfn, uint32_t *status);
+int online_page(mfn_t mfn, uint32_t *status);
 int offline_page(mfn_t mfn, int broken, uint32_t *status);
 int query_page_offline(mfn_t mfn, uint32_t *status);
 
-- 
2.34.1

Re: [PATCH v1 10/19] xen/page: fix return type of online_page()

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> This commit fixes return type of online_page(), which shall be int
> to include correct error value.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

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


> ---
>  xen/common/page_alloc.c | 2 +-
>  xen/include/xen/mm.h    | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 1bf070c8c5..b5ec4bda7d 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1764,7 +1764,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>   *   The caller should make sure end_pfn <= max_page,
>   *   if not, expand_pages() should be called prior to online_page().
>   */
> -unsigned int online_page(mfn_t mfn, uint32_t *status)
> +int online_page(mfn_t mfn, uint32_t *status)
>  {
>      unsigned long x, nx, y;
>      struct page_info *pg;
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 16f733281a..ae1c48a615 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -144,7 +144,7 @@ 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))
> -unsigned int online_page(mfn_t mfn, uint32_t *status);
> +int online_page(mfn_t mfn, uint32_t *status);
>  int offline_page(mfn_t mfn, int broken, uint32_t *status);
>  int query_page_offline(mfn_t mfn, uint32_t *status);
>  
> -- 
> 2.34.1
> 

Re: [PATCH v1 10/19] xen/page: fix return type of online_page()

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> This commit fixes return type of online_page(), which shall be int
> to include correct error value.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

While the patch can certainly be taken as-is, ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1764,7 +1764,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>   *   The caller should make sure end_pfn <= max_page,
>   *   if not, expand_pages() should be called prior to online_page().
>   */
> -unsigned int online_page(mfn_t mfn, uint32_t *status)
> +int online_page(mfn_t mfn, uint32_t *status)
>  {
>      unsigned long x, nx, y;
>      struct page_info *pg;

... below here we have

        ret = *status = 0;

which aiui will need splitting for Misra anyway. Perhaps a good opportunity
to do so right here? (I wouldn't mind doing the extra change while committing.
Yet I can also see that this may be deemed too orthogonal to the main purpose
of the change here.)

Jan

Re: [PATCH v1 10/19] xen/page: fix return type of online_page()

[Jan Beulich]

On 13.03.2025 09:30, Jan Beulich wrote:
> On 12.03.2025 05:06, Penny Zheng wrote:
>> This commit fixes return type of online_page(), which shall be int
>> to include correct error value.
>>
>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> 
> While the patch can certainly be taken as-is, ...
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1764,7 +1764,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>>   *   The caller should make sure end_pfn <= max_page,
>>   *   if not, expand_pages() should be called prior to online_page().
>>   */
>> -unsigned int online_page(mfn_t mfn, uint32_t *status)
>> +int online_page(mfn_t mfn, uint32_t *status)
>>  {
>>      unsigned long x, nx, y;
>>      struct page_info *pg;
> 
> ... below here we have
> 
>         ret = *status = 0;
> 
> which aiui will need splitting for Misra anyway. Perhaps a good opportunity
> to do so right here? (I wouldn't mind doing the extra change while committing.
> Yet I can also see that this may be deemed too orthogonal to the main purpose
> of the change here.)

Actually, having thought about this some more, let's just put it in as is.

Jan

[PATCH v1 11/19] xen/sysctl: wrap around XEN_SYSCTL_page_offline_op

[Penny Zheng]

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>
---
 xen/common/page_alloc.c |  2 ++
 xen/include/xen/mm.h    | 13 ++++++++++++-
 xen/include/xsm/dummy.h |  7 +++++++
 xen/include/xsm/xsm.h   |  9 +++++++++
 xen/xsm/dummy.c         |  2 ++
 xen/xsm/flask/hooks.c   |  6 ++++++
 6 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index b5ec4bda7d..7476d37238 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1759,6 +1759,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,
@@ -1843,6 +1844,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/include/xen/mm.h b/xen/include/xen/mm.h
index ae1c48a615..86bbb15890 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -144,9 +144,20 @@ 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))
-int online_page(mfn_t mfn, uint32_t *status);
 int offline_page(mfn_t mfn, int broken, uint32_t *status);
+#ifdef CONFIG_SYSCTL
+int online_page(mfn_t mfn, uint32_t *status);
 int query_page_offline(mfn_t mfn, uint32_t *status);
+#else
+static inline int online_page(mfn_t mfn, uint32_t *status)
+{
+    return -EOPNOTSUPP;
+}
+static inline int query_page_offline(mfn_t mfn, uint32_t *status)
+{
+    return -EOPNOTSUPP;
+}
+#endif /* CONFIG_SYSCTL */
 
 void heap_init_late(void);
 
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 35d084aca7..5f89fc98fc 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -497,11 +497,18 @@ static XSM_INLINE int cf_check xsm_resource_setup_misc(XSM_DEFAULT_VOID)
     return xsm_default_action(action, current->domain, NULL);
 }
 
+#ifdef CONFIG_SYSCTL
 static XSM_INLINE int cf_check xsm_page_offline(XSM_DEFAULT_ARG uint32_t cmd)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
     return xsm_default_action(action, current->domain, NULL);
 }
+#else
+static XSM_INLINE int cf_check xsm_page_offline(XSM_DEFAULT_ARG uint32_t cmd)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 
 static XSM_INLINE int cf_check xsm_hypfs_op(XSM_DEFAULT_VOID)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index d322740de1..d6f27aa977 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -140,7 +140,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);
@@ -601,10 +603,17 @@ static inline int xsm_resource_setup_misc(xsm_default_t def)
     return alternative_call(xsm_ops.resource_setup_misc);
 }
 
+#ifdef CONFIG_SYSCTL
 static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
 {
     return alternative_call(xsm_ops.page_offline, cmd);
 }
+#else
+static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
+{
+    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 4c97db0c48..aa89ee5331 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 7c46657d97..9819cd407d 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

Re: [PATCH v1 11/19] xen/sysctl: wrap around XEN_SYSCTL_page_offline_op

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> 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>


> ---
>  xen/common/page_alloc.c |  2 ++
>  xen/include/xen/mm.h    | 13 ++++++++++++-
>  xen/include/xsm/dummy.h |  7 +++++++
>  xen/include/xsm/xsm.h   |  9 +++++++++
>  xen/xsm/dummy.c         |  2 ++
>  xen/xsm/flask/hooks.c   |  6 ++++++
>  6 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index b5ec4bda7d..7476d37238 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1759,6 +1759,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,
> @@ -1843,6 +1844,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/include/xen/mm.h b/xen/include/xen/mm.h
> index ae1c48a615..86bbb15890 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -144,9 +144,20 @@ 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))
> -int online_page(mfn_t mfn, uint32_t *status);
>  int offline_page(mfn_t mfn, int broken, uint32_t *status);
> +#ifdef CONFIG_SYSCTL
> +int online_page(mfn_t mfn, uint32_t *status);
>  int query_page_offline(mfn_t mfn, uint32_t *status);
> +#else
> +static inline int online_page(mfn_t mfn, uint32_t *status)
> +{
> +    return -EOPNOTSUPP;
> +}
> +static inline int query_page_offline(mfn_t mfn, uint32_t *status)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif /* CONFIG_SYSCTL */
>  
>  void heap_init_late(void);
>  
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 35d084aca7..5f89fc98fc 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -497,11 +497,18 @@ static XSM_INLINE int cf_check xsm_resource_setup_misc(XSM_DEFAULT_VOID)
>      return xsm_default_action(action, current->domain, NULL);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static XSM_INLINE int cf_check xsm_page_offline(XSM_DEFAULT_ARG uint32_t cmd)
>  {
>      XSM_ASSERT_ACTION(XSM_HOOK);
>      return xsm_default_action(action, current->domain, NULL);
>  }
> +#else
> +static XSM_INLINE int cf_check xsm_page_offline(XSM_DEFAULT_ARG uint32_t cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
>  
>  static XSM_INLINE int cf_check xsm_hypfs_op(XSM_DEFAULT_VOID)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index d322740de1..d6f27aa977 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -140,7 +140,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);
> @@ -601,10 +603,17 @@ static inline int xsm_resource_setup_misc(xsm_default_t def)
>      return alternative_call(xsm_ops.resource_setup_misc);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
>  {
>      return alternative_call(xsm_ops.page_offline, cmd);
>  }
> +#else
> +static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
> +{
> +    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 4c97db0c48..aa89ee5331 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 7c46657d97..9819cd407d 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
> 

Re: [PATCH v1 11/19] xen/sysctl: wrap around XEN_SYSCTL_page_offline_op

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> @@ -601,10 +603,17 @@ static inline int xsm_resource_setup_misc(xsm_default_t def)
>      return alternative_call(xsm_ops.resource_setup_misc);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
>  {
>      return alternative_call(xsm_ops.page_offline, cmd);
>  }
> +#else
> +static inline int xsm_page_offline(xsm_default_t def, uint32_t cmd)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif

As before: Less redundancy would result by placing the #ifdef inside the
function body.

Jan

[PATCH v1 12/19] xen/sysctl: wrap around XEN_SYSCTL_cpupool_op

[Penny Zheng]

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>
---
 xen/common/sched/cpupool.c | 9 +++++++++
 xen/include/xen/sched.h    | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 3d02c7b706..0efd8a4591 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,8 @@ 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 +383,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 +573,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 +639,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 +817,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 +983,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/include/xen/sched.h b/xen/include/xen/sched.h
index 9bdeb85aa4..ea81832c50 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1259,7 +1259,14 @@ static always_inline bool is_cpufreq_controller(const struct domain *d)
 }
 
 int cpupool_move_domain(struct domain *d, struct cpupool *c);
+#ifdef CONFIG_SYSCTL
 int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
+#else
+static inline int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 unsigned int cpupool_get_id(const struct domain *d);
 const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
 
-- 
2.34.1

Re: [PATCH v1 12/19] xen/sysctl: wrap around XEN_SYSCTL_cpupool_op

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> 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>


> ---
>  xen/common/sched/cpupool.c | 9 +++++++++
>  xen/include/xen/sched.h    | 7 +++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
> index 3d02c7b706..0efd8a4591 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,8 @@ 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 +383,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 +573,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 +639,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 +817,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 +983,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/include/xen/sched.h b/xen/include/xen/sched.h
> index 9bdeb85aa4..ea81832c50 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1259,7 +1259,14 @@ static always_inline bool is_cpufreq_controller(const struct domain *d)
>  }
>  
>  int cpupool_move_domain(struct domain *d, struct cpupool *c);
> +#ifdef CONFIG_SYSCTL
>  int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
> +#else
> +static inline int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
>  unsigned int cpupool_get_id(const struct domain *d);
>  const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
>  
> -- 
> 2.34.1
> 

[PATCH v1 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op

[Penny Zheng]

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>
---
 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/include/xen/sched.h     | 6 +++++-
 xen/include/xsm/dummy.h     | 2 ++
 xen/include/xsm/xsm.h       | 4 ++++
 xen/xsm/dummy.c             | 2 ++
 xen/xsm/flask/hooks.c       | 4 ++++
 10 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
index a82c0d7314..3a2c349d38 100644
--- a/xen/common/sched/arinc653.c
+++ b/xen/common/sched/arinc653.c
@@ -214,6 +214,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.
@@ -328,6 +329,7 @@ arinc653_sched_get(
 
     return 0;
 }
+#endif /* CONFIG_SYSCTL */
 
 /**************************************************************************
  * Scheduler callback functions                                           *
@@ -648,6 +650,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
@@ -687,6 +690,7 @@ a653sched_adjust_global(const struct scheduler *ops,
 
     return rc;
 }
+#endif /* CONFIG_SYSCTL */
 
 /**
  * This structure defines our scheduler for Xen.
@@ -721,7 +725,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 ea452d8b3e..e528909e5d 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2095,6 +2095,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;
@@ -2123,6 +2124,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/include/xen/sched.h b/xen/include/xen/sched.h
index ea81832c50..5a065b3624 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -848,10 +848,14 @@ void sched_destroy_vcpu(struct vcpu *v);
 int  sched_init_domain(struct domain *d, unsigned int poolid);
 void sched_destroy_domain(struct domain *d);
 long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op);
-long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
 #ifdef CONFIG_SYSCTL
+long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
 int  scheduler_id(void);
 #else
+static inline long sched_adjust_global(struct xen_sysctl_scheduler_op *op)
+{
+    return -EOPNOTSUPP;
+}
 static inline int scheduler_id(void)
 {
     return -EOPNOTSUPP;
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 5f89fc98fc..826db1fc69 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -148,11 +148,13 @@ static XSM_INLINE int cf_check xsm_domctl_scheduler_op(
     return xsm_default_action(action, current->domain, d);
 }
 
+#ifdef CONFIG_SYSCTL
 static XSM_INLINE int cf_check xsm_sysctl_scheduler_op(XSM_DEFAULT_ARG int cmd)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
     return xsm_default_action(action, current->domain, NULL);
 }
+#endif
 
 static XSM_INLINE int cf_check xsm_set_target(
     XSM_DEFAULT_ARG struct domain *d, struct domain *e)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index d6f27aa977..d413a12897 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);
 #ifdef CONFIG_SYSCTL
@@ -246,10 +248,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 aa89ee5331..6270bef308 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 9819cd407d..aefb7681cb 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

Re: [PATCH v1 13/19] xen/sysctl: wrap around XEN_SYSCTL_scheduler_op

[Stefano Stabellini]

On Wed, 12 Mar 2025, 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>


> ---
>  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/include/xen/sched.h     | 6 +++++-
>  xen/include/xsm/dummy.h     | 2 ++
>  xen/include/xsm/xsm.h       | 4 ++++
>  xen/xsm/dummy.c             | 2 ++
>  xen/xsm/flask/hooks.c       | 4 ++++
>  10 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
> index a82c0d7314..3a2c349d38 100644
> --- a/xen/common/sched/arinc653.c
> +++ b/xen/common/sched/arinc653.c
> @@ -214,6 +214,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.
> @@ -328,6 +329,7 @@ arinc653_sched_get(
>  
>      return 0;
>  }
> +#endif /* CONFIG_SYSCTL */
>  
>  /**************************************************************************
>   * Scheduler callback functions                                           *
> @@ -648,6 +650,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
> @@ -687,6 +690,7 @@ a653sched_adjust_global(const struct scheduler *ops,
>  
>      return rc;
>  }
> +#endif /* CONFIG_SYSCTL */
>  
>  /**
>   * This structure defines our scheduler for Xen.
> @@ -721,7 +725,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 ea452d8b3e..e528909e5d 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -2095,6 +2095,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;
> @@ -2123,6 +2124,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/include/xen/sched.h b/xen/include/xen/sched.h
> index ea81832c50..5a065b3624 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -848,10 +848,14 @@ void sched_destroy_vcpu(struct vcpu *v);
>  int  sched_init_domain(struct domain *d, unsigned int poolid);
>  void sched_destroy_domain(struct domain *d);
>  long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op);
> -long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
>  #ifdef CONFIG_SYSCTL
> +long sched_adjust_global(struct xen_sysctl_scheduler_op *op);
>  int  scheduler_id(void);
>  #else
> +static inline long sched_adjust_global(struct xen_sysctl_scheduler_op *op)
> +{
> +    return -EOPNOTSUPP;
> +}
>  static inline int scheduler_id(void)
>  {
>      return -EOPNOTSUPP;
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 5f89fc98fc..826db1fc69 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -148,11 +148,13 @@ static XSM_INLINE int cf_check xsm_domctl_scheduler_op(
>      return xsm_default_action(action, current->domain, d);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static XSM_INLINE int cf_check xsm_sysctl_scheduler_op(XSM_DEFAULT_ARG int cmd)
>  {
>      XSM_ASSERT_ACTION(XSM_HOOK);
>      return xsm_default_action(action, current->domain, NULL);
>  }
> +#endif
>  
>  static XSM_INLINE int cf_check xsm_set_target(
>      XSM_DEFAULT_ARG struct domain *d, struct domain *e)
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index d6f27aa977..d413a12897 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);
>  #ifdef CONFIG_SYSCTL
> @@ -246,10 +248,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 aa89ee5331..6270bef308 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 9819cd407d..aefb7681cb 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
> 

[PATCH v1 14/19] xen: make avail_domheap_pages() static

[Penny Zheng]

Function avail_domheap_pages() is only invoked by get_outstanding_claims(),
so it shall be static, no need to extern.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/common/page_alloc.c | 2 +-
 xen/include/xen/mm.h    | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 7476d37238..5e710cc9a1 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2796,7 +2796,7 @@ unsigned long avail_domheap_pages_region(
     return avail_heap_pages(zone_lo, zone_hi, node);
 }
 
-unsigned long avail_domheap_pages(void)
+static unsigned long avail_domheap_pages(void)
 {
     return avail_heap_pages(MEMZONE_XEN + 1,
                             NR_ZONES - 1,
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 86bbb15890..cbb9f2dfdb 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

Re: [PATCH v1 14/19] xen: make avail_domheap_pages() static

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> Function avail_domheap_pages() is only invoked by get_outstanding_claims(),
> so it shall be static, no need to extern.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

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


> ---
>  xen/common/page_alloc.c | 2 +-
>  xen/include/xen/mm.h    | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 7476d37238..5e710cc9a1 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2796,7 +2796,7 @@ unsigned long avail_domheap_pages_region(
>      return avail_heap_pages(zone_lo, zone_hi, node);
>  }
>  
> -unsigned long avail_domheap_pages(void)
> +static unsigned long avail_domheap_pages(void)
>  {
>      return avail_heap_pages(MEMZONE_XEN + 1,
>                              NR_ZONES - 1,
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 86bbb15890..cbb9f2dfdb 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
> 

Re: [PATCH v1 14/19] xen: make avail_domheap_pages() static

[Stefano Stabellini]

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

On Wed, 12 Mar 2025, Stefano Stabellini wrote:
> On Wed, 12 Mar 2025, Penny Zheng wrote:
> > Function avail_domheap_pages() is only invoked by get_outstanding_claims(),
> > so it shall be static, no need to extern.
> > 
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I spoke too soon: this patch breaks the build:

common/page_alloc.c: In function ‘get_outstanding_claims’:
common/page_alloc.c:587:20: error: implicit declaration of function ‘avail_domheap_pages’; did you mean ‘avail_node_heap_pages’? [-Werror=implicit-function-declaration]
  587 |     *free_pages =  avail_domheap_pages();
      |                    ^~~~~~~~~~~~~~~~~~~
      |                    avail_node_heap_pages
common/page_alloc.c:587:20: error: nested extern declaration of ‘avail_domheap_pages’ [-Werror=nested-externs]
common/page_alloc.c: At top level:
common/page_alloc.c:2798:22: error: conflicting types for ‘avail_domheap_pages’
 2798 | static unsigned long avail_domheap_pages(void)
      |                      ^~~~~~~~~~~~~~~~~~~
common/page_alloc.c:587:20: note: previous implicit declaration of ‘avail_domheap_pages’ was here
  587 |     *free_pages =  avail_domheap_pages();
      |                    ^~~~~~~~~~~~~~~~~~~
common/page_alloc.c:2798:22: error: ‘avail_domheap_pages’ defined but not used [-Werror=unused-function]
 2798 | static unsigned long avail_domheap_pages(void)
      |                      ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1
make[1]: *** [build.mk:72: common] Error 2
make: *** [Makefile:615: xen] Error 2


I think you fixed it in the next patch. Still, it is better not to break
bisectability.

> > ---
> >  xen/common/page_alloc.c | 2 +-
> >  xen/include/xen/mm.h    | 1 -
> >  2 files changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 7476d37238..5e710cc9a1 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2796,7 +2796,7 @@ unsigned long avail_domheap_pages_region(
> >      return avail_heap_pages(zone_lo, zone_hi, node);
> >  }
> >  
> > -unsigned long avail_domheap_pages(void)
> > +static unsigned long avail_domheap_pages(void)
> >  {
> >      return avail_heap_pages(MEMZONE_XEN + 1,
> >                              NR_ZONES - 1,
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> > index 86bbb15890..cbb9f2dfdb 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
> > 
> 

[PATCH v1 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo

[Penny Zheng]

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>
---
 xen/arch/arm/sysctl.c   | 2 ++
 xen/arch/riscv/stubs.c  | 2 ++
 xen/arch/x86/sysctl.c   | 2 ++
 xen/common/page_alloc.c | 5 +++++
 xen/include/xen/mm.h    | 5 +++++
 xen/include/xen/sched.h | 4 ++++
 6 files changed, 20 insertions(+)

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..0321ad57f0 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
 
 /* 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 5e710cc9a1..d1c4db57a5 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -581,6 +581,8 @@ out:
     return ret;
 }
 
+#ifdef CONFIG_SYSCTL
+static unsigned long avail_domheap_pages(void);
 void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
 {
     spin_lock(&heap_lock);
@@ -588,6 +590,7 @@ void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
     *free_pages =  avail_domheap_pages();
     spin_unlock(&heap_lock);
 }
+#endif
 
 static bool __read_mostly first_node_initialised;
 #ifndef CONFIG_SEPARATE_XENHEAP
@@ -2796,12 +2799,14 @@ unsigned long avail_domheap_pages_region(
     return avail_heap_pages(zone_lo, zone_hi, node);
 }
 
+#ifdef CONFIG_SYSCTL
 static unsigned long avail_domheap_pages(void)
 {
     return avail_heap_pages(MEMZONE_XEN + 1,
                             NR_ZONES - 1,
                             -1);
 }
+#endif
 
 unsigned long avail_node_heap_pages(unsigned int nodeid)
 {
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index cbb9f2dfdb..a63e063a46 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -131,7 +131,12 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
 unsigned long __must_check domain_adjust_tot_pages(struct domain *d,
     long pages);
 int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
+#ifdef CONFIG_SYSCTL
 void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
+#else
+static inline void get_outstanding_claims(uint64_t *free_pages,
+                                          uint64_t *outstanding_pages) {}
+#endif /* CONFIG_SYSCTL */
 
 /* Domain suballocator. These functions are *not* interrupt-safe.*/
 void init_domheap_pages(paddr_t ps, paddr_t pe);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 5a065b3624..df39c0465a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1291,7 +1291,11 @@ struct cpupool *cpupool_create_pool(unsigned int pool_id, int sched_id);
 
 extern void cf_check dump_runq(unsigned char key);
 
+#ifdef CONFIG_SYSCTL
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
+#else
+static inline void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {};
+#endif /* CONFIG_SYSCTL */
 
 #ifdef CONFIG_BOOT_TIME_CPUPOOLS
 void btcpupools_allocate_pools(void);
-- 
2.34.1

Re: [PATCH v1 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo

[Stefano Stabellini]

On Wed, 12 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>
> ---
>  xen/arch/arm/sysctl.c   | 2 ++
>  xen/arch/riscv/stubs.c  | 2 ++
>  xen/arch/x86/sysctl.c   | 2 ++
>  xen/common/page_alloc.c | 5 +++++
>  xen/include/xen/mm.h    | 5 +++++
>  xen/include/xen/sched.h | 4 ++++
>  6 files changed, 20 insertions(+)
> 
> 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..0321ad57f0 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
>  
>  /* 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 5e710cc9a1..d1c4db57a5 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -581,6 +581,8 @@ out:
>      return ret;
>  }
>  
> +#ifdef CONFIG_SYSCTL
> +static unsigned long avail_domheap_pages(void);

This part here should be in the previous patch. I would add it at the
top of the page_alloc.c file, ideally without the #ifdef, I am guessing
it is not required for a forward declaration.


>  void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
>  {
>      spin_lock(&heap_lock);
> @@ -588,6 +590,7 @@ void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
>      *free_pages =  avail_domheap_pages();
>      spin_unlock(&heap_lock);
>  }
> +#endif
>  
>  static bool __read_mostly first_node_initialised;
>  #ifndef CONFIG_SEPARATE_XENHEAP
> @@ -2796,12 +2799,14 @@ unsigned long avail_domheap_pages_region(
>      return avail_heap_pages(zone_lo, zone_hi, node);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static unsigned long avail_domheap_pages(void)
>  {
>      return avail_heap_pages(MEMZONE_XEN + 1,
>                              NR_ZONES - 1,
>                              -1);
>  }
> +#endif
>  
>  unsigned long avail_node_heap_pages(unsigned int nodeid)
>  {
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index cbb9f2dfdb..a63e063a46 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -131,7 +131,12 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
>  unsigned long __must_check domain_adjust_tot_pages(struct domain *d,
>      long pages);
>  int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
> +#ifdef CONFIG_SYSCTL
>  void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
> +#else
> +static inline void get_outstanding_claims(uint64_t *free_pages,
> +                                          uint64_t *outstanding_pages) {}
> +#endif /* CONFIG_SYSCTL */
>  
>  /* Domain suballocator. These functions are *not* interrupt-safe.*/
>  void init_domheap_pages(paddr_t ps, paddr_t pe);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 5a065b3624..df39c0465a 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1291,7 +1291,11 @@ struct cpupool *cpupool_create_pool(unsigned int pool_id, int sched_id);
>  
>  extern void cf_check dump_runq(unsigned char key);
>  
> +#ifdef CONFIG_SYSCTL
>  void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
> +#else
> +static inline void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {};
> +#endif /* CONFIG_SYSCTL */
>  
>  #ifdef CONFIG_BOOT_TIME_CPUPOOLS
>  void btcpupools_allocate_pools(void);
> -- 
> 2.34.1
> 

Re: [PATCH v1 15/19] xen/sysctl: wrap around XEN_SYSCTL_physinfo

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> --- 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

This is being removed again in patch 18. Can we try to avoid such back and forth,
if at all possible?

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -581,6 +581,8 @@ out:
>      return ret;
>  }
>  
> +#ifdef CONFIG_SYSCTL
> +static unsigned long avail_domheap_pages(void);

Instead of this, ...

> @@ -2796,12 +2799,14 @@ unsigned long avail_domheap_pages_region(
>      return avail_heap_pages(zone_lo, zone_hi, node);
>  }
>  
> +#ifdef CONFIG_SYSCTL
>  static unsigned long avail_domheap_pages(void)
>  {
>      return avail_heap_pages(MEMZONE_XEN + 1,
>                              NR_ZONES - 1,
>                              -1);
>  }
> +#endif

... can this be moved up please? Then also reducing the number of #ifdef-s?
Arguably avail_domheap_pages() could be inlined into its sole caller. That
might then be a legitimate replacement for patch 14, without breaking the
build.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -131,7 +131,12 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
>  unsigned long __must_check domain_adjust_tot_pages(struct domain *d,
>      long pages);
>  int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
> +#ifdef CONFIG_SYSCTL
>  void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
> +#else
> +static inline void get_outstanding_claims(uint64_t *free_pages,
> +                                          uint64_t *outstanding_pages) {}

This isn't a valid stub: Both arguments point to outputs. As it stands the
function now requires the caller to clear respective fields/variables up
front.

Jan

[PATCH v1 16/19] xen/sysctl: wrap around XEN_SYSCTL_coverage_op

[Penny Zheng]

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>
---
 xen/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index c4a8d86912..b226ca9b6e 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

Re: [PATCH v1 16/19] xen/sysctl: wrap around XEN_SYSCTL_coverage_op

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> 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>


> ---
>  xen/Kconfig.debug | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> index c4a8d86912..b226ca9b6e 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
> 

[PATCH v1 17/19] xen/sysctl: wrap around XEN_SYSCTL_livepatch_op

[Penny Zheng]

LIVEPATCH mechanism relies on LIVEPATCH_SYSCTL hypercall, so CONFIG_LIVEPATCH
shall depend on CONFIG_SYSCTL

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/common/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index fbaca097ff..583972f7e3 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -426,7 +426,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

Re: [PATCH v1 17/19] xen/sysctl: wrap around XEN_SYSCTL_livepatch_op

[Stefano Stabellini]

On Wed, 12 Mar 2025, Penny Zheng wrote:
> 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>


> ---
>  xen/common/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index fbaca097ff..583972f7e3 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -426,7 +426,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
> 

Re: [PATCH v1 17/19] xen/sysctl: wrap around XEN_SYSCTL_livepatch_op

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> LIVEPATCH mechanism relies on LIVEPATCH_SYSCTL hypercall, so CONFIG_LIVEPATCH
> shall depend on CONFIG_SYSCTL
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
>  xen/common/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index fbaca097ff..583972f7e3 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -426,7 +426,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

Here and in the previous patch the title doesn't really describe what's being
done. Earlier in the series such changes are titled "xen/...: make
CONFIG_... depend on CONFIG_SYSCTL". Whereas "wrap around ..." patches add
#ifdef-ary.

Jan

[PATCH v1 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl

[Penny Zheng]

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

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>
---
 xen/arch/arm/Kconfig           |  2 +-
 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/include/asm/psr.h |  2 ++
 xen/arch/x86/psr.c             | 18 ++++++++++++++++++
 xen/arch/x86/sysctl.c          |  2 --
 xen/include/xen/hypercall.h    |  8 ++++++++
 9 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index a26d3e1182..001e65b336 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -140,7 +140,7 @@ config HAS_ITS
         depends on GICV3 && !NEW_VGIC && !ARM_32
 
 config OVERLAY_DTB
-	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
+	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED && 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 fb0948f067..5819c7e4d3 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 0321ad57f0..28590efb65 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 c763f80b0b..1949c4276e 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/include/asm/psr.h b/xen/arch/x86/include/asm/psr.h
index d21a59d98f..e466da7428 100644
--- a/xen/arch/x86/include/asm/psr.h
+++ b/xen/arch/x86/include/asm/psr.h
@@ -75,8 +75,10 @@ static inline bool psr_cmt_enabled(void)
 int psr_alloc_rmid(struct domain *d);
 void psr_free_rmid(struct domain *d);
 
+#ifdef CONFIG_SYSCTL
 int psr_get_info(unsigned int socket, enum psr_type type,
                  uint32_t data[], unsigned int array_len);
+#endif
 int psr_get_val(struct domain *d, unsigned int socket,
                 uint32_t *val, enum psr_type type);
 int psr_set_val(struct domain *d, unsigned int socket,
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 5815a35335..34d5de8d2c 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
 
 /* 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
 
 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
 
 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/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 15b6be6ec8..d0ea64b332 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -29,10 +29,18 @@ arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 
+#ifdef CONFIG_SYSCTL
 extern long
 arch_do_sysctl(
     struct xen_sysctl *sysctl,
     XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl);
+#else
+static inline long arch_do_sysctl(struct xen_sysctl *sysctl,
+                                  XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
+{
+    return -EOPNOTSUPP;
+}
+#endif
 
 extern long
 pci_physdev_op(
-- 
2.34.1

Re: [PATCH v1 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl

[Stefano Stabellini]

On Wed, 12 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
> 
> 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>
> ---
>  xen/arch/arm/Kconfig           |  2 +-
>  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/include/asm/psr.h |  2 ++
>  xen/arch/x86/psr.c             | 18 ++++++++++++++++++
>  xen/arch/x86/sysctl.c          |  2 --
>  xen/include/xen/hypercall.h    |  8 ++++++++
>  9 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index a26d3e1182..001e65b336 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -140,7 +140,7 @@ config HAS_ITS
>          depends on GICV3 && !NEW_VGIC && !ARM_32
>  
>  config OVERLAY_DTB
> -	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> +	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED && 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 fb0948f067..5819c7e4d3 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

Why remove CONFIG_SYSCTL here?


>  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 0321ad57f0..28590efb65 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 c763f80b0b..1949c4276e 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/include/asm/psr.h b/xen/arch/x86/include/asm/psr.h
> index d21a59d98f..e466da7428 100644
> --- a/xen/arch/x86/include/asm/psr.h
> +++ b/xen/arch/x86/include/asm/psr.h
> @@ -75,8 +75,10 @@ static inline bool psr_cmt_enabled(void)
>  int psr_alloc_rmid(struct domain *d);
>  void psr_free_rmid(struct domain *d);
>  
> +#ifdef CONFIG_SYSCTL
>  int psr_get_info(unsigned int socket, enum psr_type type,
>                   uint32_t data[], unsigned int array_len);
> +#endif
>  int psr_get_val(struct domain *d, unsigned int socket,
>                  uint32_t *val, enum psr_type type);
>  int psr_set_val(struct domain *d, unsigned int socket,
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 5815a35335..34d5de8d2c 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
>  
>  /* 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
>  
>  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
>  
>  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

and also here?


>  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/include/xen/hypercall.h b/xen/include/xen/hypercall.h
> index 15b6be6ec8..d0ea64b332 100644
> --- a/xen/include/xen/hypercall.h
> +++ b/xen/include/xen/hypercall.h
> @@ -29,10 +29,18 @@ arch_do_domctl(
>      struct xen_domctl *domctl, struct domain *d,
>      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  
> +#ifdef CONFIG_SYSCTL
>  extern long
>  arch_do_sysctl(
>      struct xen_sysctl *sysctl,
>      XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl);
> +#else
> +static inline long arch_do_sysctl(struct xen_sysctl *sysctl,
> +                                  XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
>  
>  extern long
>  pci_physdev_op(
> -- 
> 2.34.1
> 

Re: [PATCH v1 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl

[Jan Beulich]

On 13.03.2025 00:44, Stefano Stabellini wrote:
> On Wed, 12 Mar 2025, Penny Zheng wrote:
>> --- 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

This is ...

>> --- 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
> 
> Why remove CONFIG_SYSCTL here?

... the answer to this question.

Jan

Re: [PATCH v1 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl

[Jan Beulich]

On 12.03.2025 05:06, Penny Zheng wrote:
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -140,7 +140,7 @@ config HAS_ITS
>          depends on GICV3 && !NEW_VGIC && !ARM_32
>  
>  config OVERLAY_DTB
> -	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> +	bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED && SYSCTL
>  	help
>  	  Dynamic addition/removal of Xen device tree nodes using a dtbo.

This wants to be "depends on", not an extension to the prompt's visibility.
Else a "select OVERLAY_DTB" somewhere would be permitted by kconfig without
issuing a warning.

> --- 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");

This kind of a change would preferrably have the #endif annotated with a comment
mentioning the #ifdef's condition. Such comments may best be added as the #ifdef-s
are introduced. They can certainly continue to be omitted when the range covered
is only very few lines.

> --- a/xen/arch/x86/include/asm/psr.h
> +++ b/xen/arch/x86/include/asm/psr.h
> @@ -75,8 +75,10 @@ static inline bool psr_cmt_enabled(void)
>  int psr_alloc_rmid(struct domain *d);
>  void psr_free_rmid(struct domain *d);
>  
> +#ifdef CONFIG_SYSCTL
>  int psr_get_info(unsigned int socket, enum psr_type type,
>                   uint32_t data[], unsigned int array_len);
> +#endif

As before declarations alone may not need #ifdef-ing out.

> --- 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
>  
>  /* 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
>  
>  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
>  
>  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)

That's quite a lot of #ifdef-ary here. I wonder if we can't do any better.

Jan

RE: [PATCH v1 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl

[Penny, Zheng]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, March 14, 2025 12:33 AM
> 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 v1 18/19] xen/sysctl: wrap around arch-specific
> arch_do_sysctl
>
> On 12.03.2025 05:06, Penny Zheng wrote:
> > --- 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
> >
> >  /* 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
> >
> >  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
> >
> >  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)
>
> That's quite a lot of #ifdef-ary here. I wonder if we can't do any better.
>

xl-psr half relies on sysctl op, and half relies on domctl. So I'm not sure for
CONFIG_X86_PSR, whether we shall make it dependent on CONFIG_SYSCTL

> Jan

Re: [PATCH v1 18/19] xen/sysctl: wrap around arch-specific arch_do_sysctl

[Jan Beulich]

On 24.03.2025 08:59, Penny, Zheng wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, March 14, 2025 12:33 AM
>> 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 v1 18/19] xen/sysctl: wrap around arch-specific
>> arch_do_sysctl
>>
>> On 12.03.2025 05:06, Penny Zheng wrote:
>>> --- 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
>>>
>>>  /* 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
>>>
>>>  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
>>>
>>>  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)
>>
>> That's quite a lot of #ifdef-ary here. I wonder if we can't do any better.
>>
> 
> xl-psr half relies on sysctl op, and half relies on domctl. So I'm not sure for
> CONFIG_X86_PSR, whether we shall make it dependent on CONFIG_SYSCTL

And indeed I don't think you can. That also wasn't what I had in mind (or else I
would have said so explicitly); I kept my reply vague because I didn't have any
concrete suggestion (yet).

Jan

[PATCH v1 19/19] xen/sysctl: wrap around sysctl hypercall

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@amd.com>

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>
---
 xen/common/Makefile          | 2 +-
 xen/include/hypercall-defs.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index cba3b32733..31c6fb1b71 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-y += monitor.o
-obj-y += sysctl.o
+obj-$(CONFIG_SYSCTL) += sysctl.o
 endif
 
 extra-y := symbols-dummy.o
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 7720a29ade..b2118e9adb 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -195,7 +195,9 @@ 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
+#ifdef CONFIG_SYSCTL
 sysctl(xen_sysctl_t *u_sysctl)
+#endif
 domctl(xen_domctl_t *u_domctl)
 paging_domctl_cont(xen_domctl_t *u_domctl)
 platform_op(xen_platform_op_t *u_xenpf_op)
@@ -274,7 +276,9 @@ physdev_op                         compat   do       hvm      hvm      do_arm
 hvm_op                             do       do       do       do       do
 #endif
 #ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#ifdef CONFIG_SYSCTL
 sysctl                             do       do       do       do       do
+#endif
 domctl                             do       do       do       do       do
 #endif
 #ifdef CONFIG_KEXEC
-- 
2.34.1

Re: [PATCH v1 19/19] xen/sysctl: wrap around sysctl hypercall

[Jan Beulich]

On 12.03.2025 05:06, 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-y += monitor.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o

This wants to move back up then, into the main (alphabetically sorted)
list of objects.

> --- a/xen/include/hypercall-defs.c
> +++ b/xen/include/hypercall-defs.c
> @@ -195,7 +195,9 @@ 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
> +#ifdef CONFIG_SYSCTL
>  sysctl(xen_sysctl_t *u_sysctl)
> +#endif
>  domctl(xen_domctl_t *u_domctl)
>  paging_domctl_cont(xen_domctl_t *u_domctl)
>  platform_op(xen_platform_op_t *u_xenpf_op)
> @@ -274,7 +276,9 @@ physdev_op                         compat   do       hvm      hvm      do_arm
>  hvm_op                             do       do       do       do       do
>  #endif
>  #ifndef CONFIG_PV_SHIM_EXCLUSIVE
> +#ifdef CONFIG_SYSCTL
>  sysctl                             do       do       do       do       do
> +#endif
>  domctl                             do       do       do       do       do
>  #endif
>  #ifdef CONFIG_KEXEC

As indicated earlier on, PV_SHIM_EXCLUSIVE likely wants / needs sorting as
a prereq anyway. Otherwise I think the new #ifdef-s better wouldn't end up
inside the PV_SHIM_EXCLUSIVE ones.

Jan

RE: [PATCH v1 19/19] xen/sysctl: wrap around sysctl hypercall

[Penny, Zheng]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, March 14, 2025 5:27 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Stabellini, Stefano
> <stefano.stabellini@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Sergiy Kibrik
> <Sergiy_Kibrik@epam.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1 19/19] xen/sysctl: wrap around sysctl hypercall
>
> On 12.03.2025 05:06, 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-y += monitor.o -obj-y += sysctl.o
> > +obj-$(CONFIG_SYSCTL) += sysctl.o
>
> This wants to move back up then, into the main (alphabetically sorted) list of
> objects.
>
> > --- a/xen/include/hypercall-defs.c
> > +++ b/xen/include/hypercall-defs.c
> > @@ -195,7 +195,9 @@ 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
> > +#ifdef CONFIG_SYSCTL
> >  sysctl(xen_sysctl_t *u_sysctl)
> > +#endif
> >  domctl(xen_domctl_t *u_domctl)
> >  paging_domctl_cont(xen_domctl_t *u_domctl)
> > platform_op(xen_platform_op_t *u_xenpf_op)
> > @@ -274,7 +276,9 @@ physdev_op                         compat   do       hvm      hvm
> do_arm
> >  hvm_op                             do       do       do       do       do
> >  #endif
> >  #ifndef CONFIG_PV_SHIM_EXCLUSIVE
> > +#ifdef CONFIG_SYSCTL
> >  sysctl                             do       do       do       do       do
> > +#endif
> >  domctl                             do       do       do       do       do
> >  #endif
> >  #ifdef CONFIG_KEXEC
>
> As indicated earlier on, PV_SHIM_EXCLUSIVE likely wants / needs sorting as a
> prereq anyway. Otherwise I think the new #ifdef-s better wouldn't end up inside the
> PV_SHIM_EXCLUSIVE ones.
>

May I ask, if we dropped the earlier commit, not replacing all the !PV_SHIM_EXCLUSIVE with UNRESTRICTED,
whathat is the next plan for it ?

> Jan

Re: [PATCH v1 19/19] xen/sysctl: wrap around sysctl hypercall

[Jan Beulich]

On 24.03.2025 07:11, Penny, Zheng wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, March 14, 2025 5:27 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Stabellini, Stefano
>> <stefano.stabellini@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> Anthony PERARD <anthony.perard@vates.tech>; Orzel, Michal
>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau Monné
>> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Sergiy Kibrik
>> <Sergiy_Kibrik@epam.com>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v1 19/19] xen/sysctl: wrap around sysctl hypercall
>>
>> On 12.03.2025 05:06, 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-y += monitor.o -obj-y += sysctl.o
>>> +obj-$(CONFIG_SYSCTL) += sysctl.o
>>
>> This wants to move back up then, into the main (alphabetically sorted) list of
>> objects.
>>
>>> --- a/xen/include/hypercall-defs.c
>>> +++ b/xen/include/hypercall-defs.c
>>> @@ -195,7 +195,9 @@ 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
>>> +#ifdef CONFIG_SYSCTL
>>>  sysctl(xen_sysctl_t *u_sysctl)
>>> +#endif
>>>  domctl(xen_domctl_t *u_domctl)
>>>  paging_domctl_cont(xen_domctl_t *u_domctl)
>>> platform_op(xen_platform_op_t *u_xenpf_op)
>>> @@ -274,7 +276,9 @@ physdev_op                         compat   do       hvm      hvm
>> do_arm
>>>  hvm_op                             do       do       do       do       do
>>>  #endif
>>>  #ifndef CONFIG_PV_SHIM_EXCLUSIVE
>>> +#ifdef CONFIG_SYSCTL
>>>  sysctl                             do       do       do       do       do
>>> +#endif
>>>  domctl                             do       do       do       do       do
>>>  #endif
>>>  #ifdef CONFIG_KEXEC
>>
>> As indicated earlier on, PV_SHIM_EXCLUSIVE likely wants / needs sorting as a
>> prereq anyway. Otherwise I think the new #ifdef-s better wouldn't end up inside the
>> PV_SHIM_EXCLUSIVE ones.
> 
> May I ask, if we dropped the earlier commit, not replacing all the !PV_SHIM_EXCLUSIVE with UNRESTRICTED,
> whathat is the next plan for it ?

Didn't I mention this in enough detail in [1]? Stefano said he'd have someone
in mind to carry out that work. Stefano - any more concrete indications?

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2025-03/msg00783.html