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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2025 06:50, Penny Zheng wrote:
> Function arch_do_sysctl is to perform arch-specific sysctl op.
> Some functions, like psr_get_info for x86, DTB overlay support for arm,
> are solely available through sysctl op, then they all shall be wrapped
> with CONFIG_SYSCTL
> Also, remove all #ifdef CONFIG_SYSCTL-s in arch-specific sysctl.c, as
> we put the guardian in Makefile for the whole file.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> - use "depends on" for config OVERLAY_DTB
> - no need to wrap declaration
> - add transient #ifdef in sysctl.c for correct compilation
> ---
>  xen/arch/arm/Kconfig   |  1 +
>  xen/arch/arm/Makefile  |  2 +-
>  xen/arch/arm/sysctl.c  |  2 --
>  xen/arch/riscv/stubs.c |  2 +-
>  xen/arch/x86/Makefile  |  2 +-
>  xen/arch/x86/psr.c     | 18 ++++++++++++++++++
>  xen/arch/x86/sysctl.c  |  2 --
>  xen/common/sysctl.c    |  2 ++
>  8 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ffdff1f0a3..aa1b4a6e6b 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -141,6 +141,7 @@ config HAS_ITS
>  
>  config OVERLAY_DTB
>       bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> +     depends on SYSCTL
>       help
>         Dynamic addition/removal of Xen device tree nodes using a dtbo.
>  
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 4837ad467a..7c6015b84d 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -54,7 +54,7 @@ obj-y += smpboot.o
>  obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
>  obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
>  obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
>  obj-y += time.o
>  obj-y += traps.o
>  obj-y += vcpreg.o
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index 2d350b700a..32cab4feff 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -15,7 +15,6 @@
>  #include <asm/arm64/sve.h>
>  #include <public/sysctl.h>
>  
> -#ifdef CONFIG_SYSCTL
>  void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>  {
>      pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
> @@ -23,7 +22,6 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>      pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>                                         XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>  }
> -#endif
>  
>  long arch_do_sysctl(struct xen_sysctl *sysctl,
>                      XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 7b3f748886..ae865e1972 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to, const void 
> __user *from,
>  
>  /* sysctl.c */
>  
> +#ifdef CONFIG_SYSCTL
>  long arch_do_sysctl(struct xen_sysctl *sysctl,
>                      XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  {
>      BUG_ON("unimplemented");
>  }
>  
> -#ifdef CONFIG_SYSCTL
>  void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>  {
>      BUG_ON("unimplemented");
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f59c9665fd..837eafcbc0 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
>  obj-y += domctl.o
>  obj-y += platform_hypercall.o
>  obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
> -obj-y += sysctl.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
>  endif

I think I had indicated before that this shouldn't stay inside the conditional,
but move back up. Whether that is to happen here or while addressing my
respective comment on patch 01 I can't easily tell.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -490,8 +490,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          break;
>  
>      default:
> +#ifdef CONFIG_SYSCTL
>          ret = arch_do_sysctl(op, u_sysctl);
>          copyback = 0;
> +#endif
>          break;
>      }

This isn't enough. "ret" is 0 when reaching the default: label, but may not
stay 0 for the return from the function. I understand (expect) this is going to
be dropped again in the next patch, but even if only transiently needed this
should be kept correct imo. Things might be different if patch 02 introduced
the option without a prompt, i.e. always enabled. Then all the #ifdef-ary added
up to here would be merely syntactic sugar. In fact in that case you could omit
all the transient #ifdef that the last patch is going to remove again. Please
consider going that route.

Otherwise I think the #endif also needs moving up, for copyback to still be
cleared here.

Jan