1. Patchew
  2. Xen
  3. [PATCH v2 0/4] make build of vm_event/mem_access/monitor optional
  4. View patch

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

Sergiy Kibrik posted 4 patches 9 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 4/4] xen: mem_access: conditionally compile vm_event.c & monitor.c
Posted by Sergiy Kibrik 9 months, 2 weeks ago 
From: Stefano Stabellini <stefano.stabellini@amd.com>

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@suse.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
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();
 
     if ( rc == 1 )
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b71d4b3efa..ac23120d7d 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -54,7 +54,7 @@ obj-y += timer.o
 obj-$(CONFIG_TRACEBUFFER) += trace.o
 obj-y += version.o
 obj-y += virtual_region.o
-obj-y += vm_event.o
+obj-$(CONFIG_VM_EVENT) += vm_event.o
 obj-$(CONFIG_HAS_VMAP) += vmap.o
 obj-y += vsprintf.o
 obj-y += wait.o
@@ -68,7 +68,7 @@ obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o xlat.o
 
 ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
 obj-y += domctl.o
-obj-y += monitor.o
+obj-$(CONFIG_VM_EVENT) += monitor.o
 obj-y += sysctl.o
 endif
 
diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h
index 713d54f7c1..afb582bc26 100644
--- a/xen/include/xen/monitor.h
+++ b/xen/include/xen/monitor.h
@@ -27,8 +27,17 @@
 struct domain;
 struct xen_domctl_monitor_op;
 
+#ifdef CONFIG_VM_EVENT
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop);
 void monitor_guest_request(void);
+#else
+static inline int monitor_domctl(struct domain *d,
+                                 struct xen_domctl_monitor_op *mop)
+{
+    return -EINVAL;
+}
+static inline void monitor_guest_request(void) {}
+#endif
 
 int monitor_traps(struct vcpu *v, bool sync, vm_event_request_t *req);
 
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index 9a86358b42..268c85fc4f 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -50,9 +50,6 @@ struct vm_event_domain
     unsigned int last_vcpu_wake_up;
 };
 
-/* Clean up on domain destruction */
-void vm_event_cleanup(struct domain *d);
-
 /* Returns whether a ring has been set up */
 bool vm_event_check_ring(struct vm_event_domain *ved);
 
@@ -88,7 +85,18 @@ void vm_event_cancel_slot(struct domain *d, struct vm_event_domain *ved);
 void vm_event_put_request(struct domain *d, struct vm_event_domain *ved,
                           vm_event_request_t *req);
 
+#ifdef CONFIG_VM_EVENT
+/* Clean up on domain destruction */
+void vm_event_cleanup(struct domain *d);
 int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec);
+#else
+static inline void vm_event_cleanup(struct domain *d) {}
+static inline int vm_event_domctl(struct domain *d,
+                                  struct xen_domctl_vm_event_op *vec)
+{
+    return -EINVAL;
+}
+#endif
 
 void vm_event_vcpu_pause(struct vcpu *v);
 void vm_event_vcpu_unpause(struct vcpu *v);
-- 
2.25.1
Re: [PATCH v2 4/4] xen: mem_access: conditionally compile vm_event.c & monitor.c
Posted by Tamas K Lengyel 9 months ago 
On Tue, Jan 21, 2025 at 5:25 AM Sergiy Kibrik <Sergiy_Kibrik@epam.com> wrote:
>
> From: Stefano Stabellini <stefano.stabellini@amd.com>
>
> 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@suse.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
> 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
Re: [PATCH v2 4/4] xen: mem_access: conditionally compile vm_event.c & monitor.c
Posted by Jan Beulich 9 months ago 
On 31.01.2025 01:33, Tamas K Lengyel wrote:
> On Tue, Jan 21, 2025 at 5:25 AM Sergiy Kibrik <Sergiy_Kibrik@epam.com> wrote:
>> --- 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.

Well - we generally prefer things being written this way, where possible.
This is to keep as much code as possible exposed to the compiler no
matter what configuration. This way the risk of bit-rotting is a little
lower (e.g. when making changes affecting such a piece of code, but not
noticing the need for a change because things compile fine in whatever
configuration(s) the person tests).

Jan

> The rest of the patch looks fine to me.
> 
> Tamas
Re: [PATCH v2 4/4] xen: mem_access: conditionally compile vm_event.c & monitor.c
Posted by Jan Beulich 9 months ago 
On 21.01.2025 11:25, Sergiy Kibrik wrote:
> --- a/xen/include/xen/monitor.h
> +++ b/xen/include/xen/monitor.h
> @@ -27,8 +27,17 @@
>  struct domain;
>  struct xen_domctl_monitor_op;
>  
> +#ifdef CONFIG_VM_EVENT
>  int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop);
>  void monitor_guest_request(void);
> +#else
> +static inline int monitor_domctl(struct domain *d,
> +                                 struct xen_domctl_monitor_op *mop)
> +{
> +    return -EINVAL;

EOPNOTSUPP perhaps?

Otherwise looks okay to me, but first and foremost requires Arm side approval.

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.