[Public]
Hi，
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, April 1, 2025 10:47 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Julien Grall <jul...@xen.org>; Bertrand Marquis
> <bertrand.marq...@arm.com>; Orzel, Michal <michal.or...@amd.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
> Roger Pau Monné <roger....@citrix.com>; Alistair Francis
> <alistair.fran...@wdc.com>; Bob Eshleman <bobbyeshle...@gmail.com>;
> Connor Davis <connojda...@gmail.com>; Oleksii Kurochko
> <oleksii.kuroc...@gmail.com>; Stabellini, Stefano
> <stefano.stabell...@amd.com>; Sergiy Kibrik <sergiy_kib...@epam.com>; xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific
> arch_do_sysctl
>
> On 26.03.2025 06:50, Penny Zheng wrote:
> > Function arch_do_sysctl is to perform arch-specific sysctl op.
> > Some functions, like psr_get_info for x86, DTB overlay support for
> > arm, are solely available through sysctl op, then they all shall be
> > wrapped with CONFIG_SYSCTL Also, remove all #ifdef CONFIG_SYSCTL-s in
> > arch-specific sysctl.c, as we put the guardian in Makefile for the
> > whole file.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabell...@amd.com>
> > Signed-off-by: Sergiy Kibrik <sergiy_kib...@epam.com>
> > Signed-off-by: Penny Zheng <penny.zh...@amd.com>
> > ---
> > - use "depends on" for config OVERLAY_DTB
> > - no need to wrap declaration
> > - add transient #ifdef in sysctl.c for correct compilation
> > ---
> > xen/arch/arm/Kconfig | 1 +
> > xen/arch/arm/Makefile | 2 +-
> > xen/arch/arm/sysctl.c | 2 --
> > xen/arch/riscv/stubs.c | 2 +-
> > xen/arch/x86/Makefile | 2 +-
> > xen/arch/x86/psr.c | 18 ++++++++++++++++++
> > xen/arch/x86/sysctl.c | 2 --
> > xen/common/sysctl.c | 2 ++
> > 8 files changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > ffdff1f0a3..aa1b4a6e6b 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -141,6 +141,7 @@ config HAS_ITS
> >
> > config OVERLAY_DTB
> > bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> > + depends on SYSCTL
> > help
> > Dynamic addition/removal of Xen device tree nodes using a dtbo.
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index
> > 4837ad467a..7c6015b84d 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -54,7 +54,7 @@ obj-y += smpboot.o
> > obj-$(CONFIG_STATIC_EVTCHN) += static-evtchn.init.o
> > obj-$(CONFIG_STATIC_MEMORY) += static-memory.init.o
> > obj-$(CONFIG_STATIC_SHM) += static-shmem.init.o -obj-y += sysctl.o
> > +obj-$(CONFIG_SYSCTL) += sysctl.o
> > obj-y += time.o
> > obj-y += traps.o
> > obj-y += vcpreg.o
> > diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c index
> > 2d350b700a..32cab4feff 100644
> > --- a/xen/arch/arm/sysctl.c
> > +++ b/xen/arch/arm/sysctl.c
> > @@ -15,7 +15,6 @@
> > #include <asm/arm64/sve.h>
> > #include <public/sysctl.h>
> >
> > -#ifdef CONFIG_SYSCTL
> > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {
> > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm |
> > XEN_SYSCTL_PHYSCAP_hap; @@ -23,7 +22,6 @@ void
> arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> > pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
> >
> > XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); } -#endif
> >
> > long arch_do_sysctl(struct xen_sysctl *sysctl,
> > XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index
> > 7b3f748886..ae865e1972 100644
> > --- a/xen/arch/riscv/stubs.c
> > +++ b/xen/arch/riscv/stubs.c
> > @@ -322,13 +322,13 @@ unsigned long raw_copy_from_guest(void *to,
> > const void __user *from,
> >
> > /* sysctl.c */
> >
> > +#ifdef CONFIG_SYSCTL
> > long arch_do_sysctl(struct xen_sysctl *sysctl,
> > XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) {
> > BUG_ON("unimplemented");
> > }
> >
> > -#ifdef CONFIG_SYSCTL
> > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) {
> > BUG_ON("unimplemented");
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index
> > f59c9665fd..837eafcbc0 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -79,7 +79,7 @@ ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) obj-y +=
> > domctl.o obj-y += platform_hypercall.o
> > obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o -obj-y +=
> > sysctl.o
> > +obj-$(CONFIG_SYSCTL) += sysctl.o
> > endif
>
> I think I had indicated before that this shouldn't stay inside the
> conditional, but
> move back up. Whether that is to happen here or while addressing my respective
> comment on patch 01 I can't easily tell.
>
We want that "PV_SHIM_EXCLUSIVE likely wants / needs sorting as
a prereq anyway", does the prereq here mean that prereq in kconfig,
something like
```
config SYSCTL
depends on xxx
```
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -490,8 +490,10 @@ long
> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > break;
> >
> > default:
> > +#ifdef CONFIG_SYSCTL
> > ret = arch_do_sysctl(op, u_sysctl);
> > copyback = 0;
> > +#endif
> > break;
> > }
>
> This isn't enough. "ret" is 0 when reaching the default: label, but may not
> stay 0 for
> the return from the function. I understand (expect) this is going to be
> dropped
> again in the next patch, but even if only transiently needed this should be
> kept
> correct imo. Things might be different if patch 02 introduced the option
> without a
> prompt, i.e. always enabled. Then all the #ifdef-ary added up to here would be
> merely syntactic sugar. In fact in that case you could omit all the transient
> #ifdef
> that the last patch is going to remove again. Please consider going that
> route.
>
> Otherwise I think the #endif also needs moving up, for copyback to still be
> cleared
> here.
>
I'll change it to as follows to complement case for CONFIG_SYSCTL==n, plz
correct me if I understand wrongly here:
```
default:
+#ifdef CONFIG_SYSCTL
ret = arch_do_sysctl(op, u_sysctl);
+#else
+ ret = -EOPNOTSUPP;
+#endif
copyback = 0;
break;
```
> Jan
