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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2025 11:46, Penny, Zheng wrote:
> [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
> ```

I'm sorry, but I fear I can't interpret what you're saying (possibly asking).

>>> --- a/xen/common/sysctl.c
>>> +++ b/xen/common/sysctl.c
>>> @@ -490,8 +490,10 @@ long
>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>>          break;
>>>
>>>      default:
>>> +#ifdef CONFIG_SYSCTL
>>>          ret = arch_do_sysctl(op, u_sysctl);
>>>          copyback = 0;
>>> +#endif
>>>          break;
>>>      }
>>
>> This isn't enough. "ret" is 0 when reaching the default: label, but may not 
>> stay 0 for
>> the return from the function. I understand (expect) this is going to be 
>> dropped
>> again in the next patch, but even if only transiently needed this should be 
>> kept
>> correct imo. Things might be different if patch 02 introduced the option 
>> without a
>> prompt, i.e. always enabled. Then all the #ifdef-ary added up to here would 
>> be
>> merely syntactic sugar. In fact in that case you could omit all the 
>> transient #ifdef
>> that the last patch is going to remove again. Please consider going that 
>> route.
>>
>> Otherwise I think the #endif also needs moving up, for copyback to still be 
>> cleared
>> here.
>>
> 
> I'll change it to as follows to complement case for CONFIG_SYSCTL==n, plz 
> correct me if I understand wrongly here:
> ```
>       default:
> +#ifdef CONFIG_SYSCTL
>          ret = arch_do_sysctl(op, u_sysctl);
> +#else
> +        ret = -EOPNOTSUPP;
> +#endif
>          copyback = 0;
>          break;
> ```

This is an option, yes, yet I'd like my other outline to be taken into 
consideration,
too (for imo resulting in less churn overall).

Jan