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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi，

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, April 1, 2025 10:47 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <bertrand.marquis@xxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>; Alistair Francis
> <alistair.francis@xxxxxxx>; Bob Eshleman <bobbyeshleman@xxxxxxxxx>;
> Connor Davis <connojdavis@xxxxxxxxx>; Oleksii Kurochko
> <oleksii.kurochko@xxxxxxxxx>; Stabellini, Stefano
> <stefano.stabellini@xxxxxxx>; Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 18/19] xen/sysctl: wrap around arch-specific
> arch_do_sysctl
>
> On 26.03.2025 06:50, Penny Zheng wrote:
> > Function arch_do_sysctl is to perform arch-specific sysctl op.
> > Some functions, like psr_get_info for x86, DTB overlay support for
> > arm, are solely available through sysctl op, then they all shall be
> > wrapped with CONFIG_SYSCTL Also, remove all #ifdef CONFIG_SYSCTL-s in
> > arch-specific sysctl.c, as we put the guardian in Makefile for the
> > whole file.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@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.
>

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