Re: [PATCH v2 4/4] xen: mem_access: conditionally compile vm_event.c & monitor.c

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Jan 21, 2025 at 5:25 AM Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx> wrote:
>
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
>
> Extend coverage of CONFIG_VM_EVENT option and make the build of VM events
> and monitoring support optional.
> This is to reduce code size on Arm when this option isn't enabled.
>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> ---
> changes in v2:
>  - rename CONFIG_MEM_ACCESS -> CONFIG_VM_EVENT
>  - tags
> ---
>  xen/arch/arm/Makefile      |  4 ++--
>  xen/arch/arm/vsmc.c        |  3 ++-
>  xen/common/Makefile        |  4 ++--
>  xen/include/xen/monitor.h  |  9 +++++++++
>  xen/include/xen/vm_event.h | 14 +++++++++++---
>  5 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index ad29316df1..e61238c4d0 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,7 +39,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o
>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
>  obj-$(CONFIG_VM_EVENT) += mem_access.o
>  obj-y += mm.o
> -obj-y += monitor.o
> +obj-$(CONFIG_VM_EVENT) += monitor.o
>  obj-y += p2m.o
>  obj-y += platform.o
>  obj-y += platform_hypercall.o
> @@ -65,7 +65,7 @@ obj-$(CONFIG_VGICV2) += vgic-v2.o
>  obj-$(CONFIG_GICV3) += vgic-v3.o
>  obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
>  endif
> -obj-y += vm_event.o
> +obj-$(CONFIG_VM_EVENT) += vm_event.o
>  obj-y += vtimer.o
>  obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o
>  obj-y += vsmc.o
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 62d8117a12..1ea75cd7f1 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -330,7 +330,8 @@ void do_trap_smc(struct cpu_user_regs *regs, const union 
> hsr hsr)
>      }
>
>      /* If monitor is enabled, let it handle the call. */
> -    if ( current->domain->arch.monitor.privileged_call_enabled )
> +    if ( IS_ENABLED(CONFIG_VM_EVENT) &&
> +         current->domain->arch.monitor.privileged_call_enabled )
>          rc = monitor_smc();

Why not wrap this entire if block above in an #ifdef CONFIG_VM_EVENT?
I think it would be more explicit what code is being compiled that way
instead of just relying on the compiler optimization to take care of
removing it. The rest of the patch looks fine to me.

Tamas