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