[PATCH v2 0/4] make build of vm_event/mem_access/monitor optional

[PATCH v2 0/4] make build of vm_event/mem_access/monitor optional

[Sergiy Kibrik]

This series aims to provide a possibility to build Xen without mem_access and
related features. It is already largely covered by corresponding
CONFIG_MEM_ACCESS option, yet there're still some parts remaining.
Hopefully this would help to reduce dead code a bit.

As coverage of MEM_ACCESS config option begins to extend beyond actual
mem_access code it has been suggested to rename it into VM_EVENT, as a more
general option controlling mem_access, vm_event and monitor code.

v1 patch here:
https://lore.kernel.org/xen-devel/20241230063051.3332332-1-Sergiy_Kibrik@epam.com/

  -Sergiy

Sergiy Kibrik (3):
  xen: kconfig: rename MEM_ACCESS -> VM_EVENT
  x86:monitor: control monitor.c build with CONFIG_VM_EVENT option
  automation: rename CONFIG_MEM_ACCESS -> CONFIG_VM_EVENT

Stefano Stabellini (1):
  xen: mem_access: conditionally compile vm_event.c & monitor.c

 automation/eclair_analysis/xen_arm_config |  2 +-
 automation/eclair_analysis/xen_x86_config |  2 +-
 automation/gitlab-ci/build.yaml           |  2 +-
 xen/arch/arm/Makefile                     |  6 +++---
 xen/arch/arm/configs/tiny64_defconfig     |  2 +-
 xen/arch/arm/include/asm/mem_access.h     |  4 ++--
 xen/arch/arm/vsmc.c                       |  3 ++-
 xen/arch/ppc/configs/ppc64_defconfig      |  2 +-
 xen/arch/riscv/configs/tiny64_defconfig   |  2 +-
 xen/arch/x86/Makefile                     |  2 +-
 xen/arch/x86/mm/Makefile                  |  2 +-
 xen/common/Kconfig                        |  2 +-
 xen/common/Makefile                       |  6 +++---
 xen/common/domctl.c                       |  2 +-
 xen/include/xen/mem_access.h              |  6 +++---
 xen/include/xen/monitor.h                 |  9 +++++++++
 xen/include/xen/vm_event.h                | 14 +++++++++++---
 xen/include/xsm/dummy.h                   |  2 +-
 xen/include/xsm/xsm.h                     |  4 ++--
 xen/xsm/dummy.c                           |  2 +-
 xen/xsm/flask/hooks.c                     |  4 ++--
 21 files changed, 49 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Sergiy Kibrik]

Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
feature, with mem_access & monitor depending on it.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
option renaming brought up as part of v1 review discussion:
   https://lore.kernel.org/xen-devel/c8684340-33f9-41d3-94e4-77ee3bc18306@suse.com/
---
 xen/arch/arm/Makefile                   | 2 +-
 xen/arch/arm/configs/tiny64_defconfig   | 2 +-
 xen/arch/arm/include/asm/mem_access.h   | 4 ++--
 xen/arch/ppc/configs/ppc64_defconfig    | 2 +-
 xen/arch/riscv/configs/tiny64_defconfig | 2 +-
 xen/arch/x86/mm/Makefile                | 2 +-
 xen/common/Kconfig                      | 2 +-
 xen/common/Makefile                     | 2 +-
 xen/common/domctl.c                     | 2 +-
 xen/include/xen/mem_access.h            | 6 +++---
 xen/include/xsm/dummy.h                 | 2 +-
 xen/include/xsm/xsm.h                   | 4 ++--
 xen/xsm/dummy.c                         | 2 +-
 xen/xsm/flask/hooks.c                   | 4 ++--
 14 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 43ab5e8f25..ad29316df1 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -37,7 +37,7 @@ obj-y += irq.o
 obj-y += kernel.init.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
-obj-$(CONFIG_MEM_ACCESS) += mem_access.o
+obj-$(CONFIG_VM_EVENT) += mem_access.o
 obj-y += mm.o
 obj-y += monitor.o
 obj-y += p2m.o
diff --git a/xen/arch/arm/configs/tiny64_defconfig b/xen/arch/arm/configs/tiny64_defconfig
index cc6d93f2f8..469a1eb9f9 100644
--- a/xen/arch/arm/configs/tiny64_defconfig
+++ b/xen/arch/arm/configs/tiny64_defconfig
@@ -5,7 +5,7 @@ CONFIG_ARM=y
 # Architecture Features
 #
 # CONFIG_GICV3 is not set
-# CONFIG_MEM_ACCESS is not set
+# CONFIG_VM_EVENT is not set
 # CONFIG_SBSA_VUART_CONSOLE is not set
 
 #
diff --git a/xen/arch/arm/include/asm/mem_access.h b/xen/arch/arm/include/asm/mem_access.h
index abac8032fc..43f73f7e38 100644
--- a/xen/arch/arm/include/asm/mem_access.h
+++ b/xen/arch/arm/include/asm/mem_access.h
@@ -37,7 +37,7 @@ static inline bool p2m_mem_access_sanity_check(struct domain *d)
  * Send mem event based on the access. Boolean return value indicates if trap
  * needs to be injected into guest.
  */
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
 bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec);
 
 struct page_info*
@@ -58,7 +58,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
     return NULL;
 }
 
-#endif /*CONFIG_MEM_ACCESS*/
+#endif /*CONFIG_VM_EVENT*/
 #endif /* _ASM_ARM_MEM_ACCESS_H */
 
 /*
diff --git a/xen/arch/ppc/configs/ppc64_defconfig b/xen/arch/ppc/configs/ppc64_defconfig
index 4924d881a2..d6aaf772e7 100644
--- a/xen/arch/ppc/configs/ppc64_defconfig
+++ b/xen/arch/ppc/configs/ppc64_defconfig
@@ -1,6 +1,6 @@
 # CONFIG_GRANT_TABLE is not set
 # CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
-# CONFIG_MEM_ACCESS is not set
+# CONFIG_VM_EVENT is not set
 
 CONFIG_PPC64=y
 CONFIG_DEBUG=y
diff --git a/xen/arch/riscv/configs/tiny64_defconfig b/xen/arch/riscv/configs/tiny64_defconfig
index bb3ae26a44..2399f7b918 100644
--- a/xen/arch/riscv/configs/tiny64_defconfig
+++ b/xen/arch/riscv/configs/tiny64_defconfig
@@ -1,6 +1,6 @@
 # CONFIG_BOOT_TIME_CPUPOOLS is not set
 # CONFIG_GRANT_TABLE is not set
-# CONFIG_MEM_ACCESS is not set
+# CONFIG_VM_EVENT is not set
 # CONFIG_COVERAGE is not set
 # CONFIG_LIVEPATCH is not set
 # CONFIG_XSM is not set
diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile
index 0345388359..960f6e8409 100644
--- a/xen/arch/x86/mm/Makefile
+++ b/xen/arch/x86/mm/Makefile
@@ -4,7 +4,7 @@ obj-$(CONFIG_HVM) += hap/
 obj-$(CONFIG_ALTP2M) += altp2m.o
 obj-$(CONFIG_HVM) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_SHADOW_PAGING) += guest_walk_4.o
-obj-$(CONFIG_MEM_ACCESS) += mem_access.o
+obj-$(CONFIG_VM_EVENT) += mem_access.o
 obj-$(CONFIG_MEM_PAGING) += mem_paging.o
 obj-$(CONFIG_MEM_SHARING) += mem_sharing.o
 obj-$(CONFIG_HVM) += nested.o
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d..a6aa2c5c14 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -92,7 +92,7 @@ config HAS_VMAP
 config MEM_ACCESS_ALWAYS_ON
 	bool
 
-config MEM_ACCESS
+config VM_EVENT
 	def_bool MEM_ACCESS_ALWAYS_ON
 	prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
 	depends on HVM
diff --git a/xen/common/Makefile b/xen/common/Makefile
index cba3b32733..b71d4b3efa 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -26,7 +26,7 @@ obj-$(CONFIG_KEXEC) += kexec.o
 obj-$(CONFIG_KEXEC) += kimage.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o livepatch_elf.o
 obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
-obj-$(CONFIG_MEM_ACCESS) += mem_access.o
+obj-$(CONFIG_VM_EVENT) += mem_access.o
 obj-y += memory.o
 obj-y += multicall.o
 obj-y += notifier.o
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 05abb581a0..ffe896d8b3 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -802,7 +802,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = true;
         break;
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
     case XEN_DOMCTL_set_access_required:
         if ( unlikely(current->domain == d) ) /* no domain_pause() */
             ret = -EPERM;
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 2231341b5d..4de651038d 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -33,7 +33,7 @@
  */
 struct vm_event_st;
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
 #include <asm/mem_access.h>
 #endif
 
@@ -99,7 +99,7 @@ long p2m_set_mem_access_multi(struct domain *d,
 int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
                        unsigned int altp2m_idx);
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
 int mem_access_memop(unsigned long cmd,
                      XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
 #else
@@ -109,7 +109,7 @@ int mem_access_memop(unsigned long cmd,
 {
     return -ENOSYS;
 }
-#endif /* CONFIG_MEM_ACCESS */
+#endif /* CONFIG_VM_EVENT */
 
 #endif /* _XEN_MEM_ACCESS_H */
 
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 6a2fc33c3b..c728da9016 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -646,7 +646,7 @@ static XSM_INLINE int cf_check xsm_vm_event_control(
     return xsm_default_action(action, current->domain, d);
 }
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
 static XSM_INLINE int cf_check xsm_mem_access(XSM_DEFAULT_ARG struct domain *d)
 {
     XSM_ASSERT_ACTION(XSM_DM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 4dbff9d866..6ac1627b7b 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -153,7 +153,7 @@ struct xsm_ops {
 
     int (*vm_event_control)(struct domain *d, int mode, int op);
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
     int (*mem_access)(struct domain *d);
 #endif
 
@@ -631,7 +631,7 @@ static inline int xsm_vm_event_control(
     return alternative_call(xsm_ops.vm_event_control, d, mode, op);
 }
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
 static inline int xsm_mem_access(xsm_default_t def, struct domain *d)
 {
     return alternative_call(xsm_ops.mem_access, d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index e6ffa948f7..a6d2ec2f8b 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -111,7 +111,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
 
     .vm_event_control              = xsm_vm_event_control,
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
     .mem_access                    = xsm_mem_access,
 #endif
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 14d84df9ca..acca89e123 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1361,7 +1361,7 @@ static int cf_check flask_vm_event_control(struct domain *d, int mode, int op)
     return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__VM_EVENT);
 }
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
 static int cf_check flask_mem_access(struct domain *d)
 {
     return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__MEM_ACCESS);
@@ -1949,7 +1949,7 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
 
     .vm_event_control = flask_vm_event_control,
 
-#ifdef CONFIG_MEM_ACCESS
+#ifdef CONFIG_VM_EVENT
     .mem_access = flask_mem_access,
 #endif
 
-- 
2.25.1

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Jan Beulich]

On 21.01.2025 11:19, Sergiy Kibrik wrote:
> Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
> CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
> feature, with mem_access & monitor depending on it.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>

I don't think this is applicable; my suggestion went in a different direction.

> Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>

Before considering to ack this, I'd like you, Tamas, to confirm this is really
what you had thought of. In particular ...

> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -37,7 +37,7 @@ obj-y += irq.o
>  obj-y += kernel.init.o
>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
> -obj-$(CONFIG_MEM_ACCESS) += mem_access.o
> +obj-$(CONFIG_VM_EVENT) += mem_access.o

... changes like this one look somewhat odd to me.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -92,7 +92,7 @@ config HAS_VMAP
>  config MEM_ACCESS_ALWAYS_ON
>  	bool
>  
> -config MEM_ACCESS
> +config VM_EVENT
>  	def_bool MEM_ACCESS_ALWAYS_ON
>  	prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
>  	depends on HVM

What about MEM_ACCESS_ALWAYS_ON (visible in patch context)? Shouldn't that
become VM_EVENT_ALWAYS_ON then, too?

Further, what about MEM_PAGING and MEM_SHARING? Shouldn't those, at least
documentation purposes, then also gain a dependency on VM_EVENT?

Jan

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Tamas K Lengyel]

On Thu, Jan 30, 2025 at 8:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 21.01.2025 11:19, Sergiy Kibrik wrote:
> > Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
> > CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
> > feature, with mem_access & monitor depending on it.
> >
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
>
> I don't think this is applicable; my suggestion went in a different direction.
>
> > Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
> > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
>
> Before considering to ack this, I'd like you, Tamas, to confirm this is really
> what you had thought of. In particular ...
>
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -37,7 +37,7 @@ obj-y += irq.o
> >  obj-y += kernel.init.o
> >  obj-$(CONFIG_LIVEPATCH) += livepatch.o
> >  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
> > -obj-$(CONFIG_MEM_ACCESS) += mem_access.o
> > +obj-$(CONFIG_VM_EVENT) += mem_access.o
>
> ... changes like this one look somewhat odd to me.
>
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -92,7 +92,7 @@ config HAS_VMAP
> >  config MEM_ACCESS_ALWAYS_ON
> >       bool
> >
> > -config MEM_ACCESS
> > +config VM_EVENT
> >       def_bool MEM_ACCESS_ALWAYS_ON
> >       prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
> >       depends on HVM
>
> What about MEM_ACCESS_ALWAYS_ON (visible in patch context)? Shouldn't that
> become VM_EVENT_ALWAYS_ON then, too?
>
> Further, what about MEM_PAGING and MEM_SHARING? Shouldn't those, at least
> documentation purposes, then also gain a dependency on VM_EVENT?

MEM_PAGING, yes. MEM_SHARING, definitely not. MEM_SHARING is perfectly
functional without vm_event.

Tamas

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Jan Beulich]

On 31.01.2025 01:26, Tamas K Lengyel wrote:
> On Thu, Jan 30, 2025 at 8:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 21.01.2025 11:19, Sergiy Kibrik wrote:
>>> Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
>>> CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
>>> feature, with mem_access & monitor depending on it.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>
>> I don't think this is applicable; my suggestion went in a different direction.
>>
>>> Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
>>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
>>
>> Before considering to ack this, I'd like you, Tamas, to confirm this is really
>> what you had thought of. In particular ...
>>
>>> --- a/xen/arch/arm/Makefile
>>> +++ b/xen/arch/arm/Makefile
>>> @@ -37,7 +37,7 @@ obj-y += irq.o
>>>  obj-y += kernel.init.o
>>>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
>>>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
>>> -obj-$(CONFIG_MEM_ACCESS) += mem_access.o
>>> +obj-$(CONFIG_VM_EVENT) += mem_access.o
>>
>> ... changes like this one look somewhat odd to me.
>>
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -92,7 +92,7 @@ config HAS_VMAP
>>>  config MEM_ACCESS_ALWAYS_ON
>>>       bool
>>>
>>> -config MEM_ACCESS
>>> +config VM_EVENT
>>>       def_bool MEM_ACCESS_ALWAYS_ON
>>>       prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
>>>       depends on HVM
>>
>> What about MEM_ACCESS_ALWAYS_ON (visible in patch context)? Shouldn't that
>> become VM_EVENT_ALWAYS_ON then, too?
>>
>> Further, what about MEM_PAGING and MEM_SHARING? Shouldn't those, at least
>> documentation purposes, then also gain a dependency on VM_EVENT?
> 
> MEM_PAGING, yes. MEM_SHARING, definitely not. MEM_SHARING is perfectly
> functional without vm_event.

Is it? I see e.g.

    if ( sharing_enomem )
    {
#ifdef CONFIG_MEM_SHARING
        if ( !vm_event_check_ring(currd->vm_event_share) )
        {
            gprintk(XENLOG_ERR, "Domain %pd attempt to unshare "
                    "gfn %lx, ENOMEM and no helper\n",
                    currd, gfn);
            /* Crash the domain */
            rc = 0;
        }
#endif
    }

in hvm_hap_nested_page_fault().

Also - you responded only to a secondary remark here. What about the
more basic points further up?

Jan

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Tamas K Lengyel]

On Fri, Jan 31, 2025 at 1:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 31.01.2025 01:26, Tamas K Lengyel wrote:
> > On Thu, Jan 30, 2025 at 8:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 21.01.2025 11:19, Sergiy Kibrik wrote:
> >>> Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
> >>> CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
> >>> feature, with mem_access & monitor depending on it.
> >>>
> >>> Suggested-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> I don't think this is applicable; my suggestion went in a different direction.
> >>
> >>> Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
> >>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> >>
> >> Before considering to ack this, I'd like you, Tamas, to confirm this is really
> >> what you had thought of. In particular ...
> >>
> >>> --- a/xen/arch/arm/Makefile
> >>> +++ b/xen/arch/arm/Makefile
> >>> @@ -37,7 +37,7 @@ obj-y += irq.o
> >>>  obj-y += kernel.init.o
> >>>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
> >>>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
> >>> -obj-$(CONFIG_MEM_ACCESS) += mem_access.o
> >>> +obj-$(CONFIG_VM_EVENT) += mem_access.o
> >>
> >> ... changes like this one look somewhat odd to me.
> >>
> >>> --- a/xen/common/Kconfig
> >>> +++ b/xen/common/Kconfig
> >>> @@ -92,7 +92,7 @@ config HAS_VMAP
> >>>  config MEM_ACCESS_ALWAYS_ON
> >>>       bool
> >>>
> >>> -config MEM_ACCESS
> >>> +config VM_EVENT
> >>>       def_bool MEM_ACCESS_ALWAYS_ON
> >>>       prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
> >>>       depends on HVM
> >>
> >> What about MEM_ACCESS_ALWAYS_ON (visible in patch context)? Shouldn't that
> >> become VM_EVENT_ALWAYS_ON then, too?
> >>
> >> Further, what about MEM_PAGING and MEM_SHARING? Shouldn't those, at least
> >> documentation purposes, then also gain a dependency on VM_EVENT?
> >
> > MEM_PAGING, yes. MEM_SHARING, definitely not. MEM_SHARING is perfectly
> > functional without vm_event.
>
> Is it? I see e.g.
>
>     if ( sharing_enomem )
>     {
> #ifdef CONFIG_MEM_SHARING
>         if ( !vm_event_check_ring(currd->vm_event_share) )
>         {
>             gprintk(XENLOG_ERR, "Domain %pd attempt to unshare "
>                     "gfn %lx, ENOMEM and no helper\n",
>                     currd, gfn);
>             /* Crash the domain */
>             rc = 0;
>         }
> #endif
>     }

On x86 vm_event is always compiled in as per current setup. If we were
to make that dependent on the now renamed config option this here
should be converted to CONFIG_MEM_SHARING && CONFIG_VM_EVENT. The rest
of the mem_sharing codebase does not require vm_event to function,
this here is used only if there is a subscriber to the enomem
corner-case. It isn't normally used.

> in hvm_hap_nested_page_fault().
>
> Also - you responded only to a secondary remark here. What about the
> more basic points further up?

My recommendation to use CONFIG_VM_EVENT for the
vm_event/mem_access/monitor subsystems strictly only applies to ARM
where these three subsystems have a 1:1:1 dependency. On x86 the
dependency between the three can be more complex, I would not change
the x86 side of things unless we want to get the three subsystems
their own kconfig options.


Tamas

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Jan Beulich]

On 01.02.2025 00:36, Tamas K Lengyel wrote:
> On Fri, Jan 31, 2025 at 1:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 31.01.2025 01:26, Tamas K Lengyel wrote:
>>> On Thu, Jan 30, 2025 at 8:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 21.01.2025 11:19, Sergiy Kibrik wrote:
>>>>> Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
>>>>> CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
>>>>> feature, with mem_access & monitor depending on it.
>>>>>
>>>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I don't think this is applicable; my suggestion went in a different direction.
>>>>
>>>>> Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
>>>>
>>>> Before considering to ack this, I'd like you, Tamas, to confirm this is really
>>>> what you had thought of. In particular ...
>>>>
>>>>> --- a/xen/arch/arm/Makefile
>>>>> +++ b/xen/arch/arm/Makefile
>>>>> @@ -37,7 +37,7 @@ obj-y += irq.o
>>>>>  obj-y += kernel.init.o
>>>>>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
>>>>>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
>>>>> -obj-$(CONFIG_MEM_ACCESS) += mem_access.o
>>>>> +obj-$(CONFIG_VM_EVENT) += mem_access.o
>>>>
>>>> ... changes like this one look somewhat odd to me.
>>>>
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -92,7 +92,7 @@ config HAS_VMAP
>>>>>  config MEM_ACCESS_ALWAYS_ON
>>>>>       bool
>>>>>
>>>>> -config MEM_ACCESS
>>>>> +config VM_EVENT
>>>>>       def_bool MEM_ACCESS_ALWAYS_ON
>>>>>       prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
>>>>>       depends on HVM
>>>>
>>>> What about MEM_ACCESS_ALWAYS_ON (visible in patch context)? Shouldn't that
>>>> become VM_EVENT_ALWAYS_ON then, too?
>>>>
>>>> Further, what about MEM_PAGING and MEM_SHARING? Shouldn't those, at least
>>>> documentation purposes, then also gain a dependency on VM_EVENT?
>>>
>>> MEM_PAGING, yes. MEM_SHARING, definitely not. MEM_SHARING is perfectly
>>> functional without vm_event.
>>
>> Is it? I see e.g.
>>
>>     if ( sharing_enomem )
>>     {
>> #ifdef CONFIG_MEM_SHARING
>>         if ( !vm_event_check_ring(currd->vm_event_share) )
>>         {
>>             gprintk(XENLOG_ERR, "Domain %pd attempt to unshare "
>>                     "gfn %lx, ENOMEM and no helper\n",
>>                     currd, gfn);
>>             /* Crash the domain */
>>             rc = 0;
>>         }
>> #endif
>>     }
> 
> On x86 vm_event is always compiled in as per current setup. If we were
> to make that dependent on the now renamed config option this here
> should be converted to CONFIG_MEM_SHARING && CONFIG_VM_EVENT. The rest
> of the mem_sharing codebase does not require vm_event to function,
> this here is used only if there is a subscriber to the enomem
> corner-case. It isn't normally used.

I see.

>> in hvm_hap_nested_page_fault().
>>
>> Also - you responded only to a secondary remark here. What about the
>> more basic points further up?
> 
> My recommendation to use CONFIG_VM_EVENT for the
> vm_event/mem_access/monitor subsystems strictly only applies to ARM
> where these three subsystems have a 1:1:1 dependency. On x86 the
> dependency between the three can be more complex, I would not change
> the x86 side of things unless we want to get the three subsystems
> their own kconfig options.

Then why did you ack the patch, which clearly extends things to x86 as
well? Iirc my suggestion was to indeed go with separate options (hence
why I think the Suggested-by: here is wrong; see context near the top).

Jan

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Tamas K Lengyel]

On Mon, Feb 3, 2025 at 2:36 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.02.2025 00:36, Tamas K Lengyel wrote:
> > On Fri, Jan 31, 2025 at 1:30 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 31.01.2025 01:26, Tamas K Lengyel wrote:
> >>> On Thu, Jan 30, 2025 at 8:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 21.01.2025 11:19, Sergiy Kibrik wrote:
> >>>>> Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
> >>>>> CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
> >>>>> feature, with mem_access & monitor depending on it.
> >>>>>
> >>>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> I don't think this is applicable; my suggestion went in a different direction.
> >>>>
> >>>>> Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
> >>>>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> >>>>
> >>>> Before considering to ack this, I'd like you, Tamas, to confirm this is really
> >>>> what you had thought of. In particular ...
> >>>>
> >>>>> --- a/xen/arch/arm/Makefile
> >>>>> +++ b/xen/arch/arm/Makefile
> >>>>> @@ -37,7 +37,7 @@ obj-y += irq.o
> >>>>>  obj-y += kernel.init.o
> >>>>>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
> >>>>>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
> >>>>> -obj-$(CONFIG_MEM_ACCESS) += mem_access.o
> >>>>> +obj-$(CONFIG_VM_EVENT) += mem_access.o
> >>>>
> >>>> ... changes like this one look somewhat odd to me.
> >>>>
> >>>>> --- a/xen/common/Kconfig
> >>>>> +++ b/xen/common/Kconfig
> >>>>> @@ -92,7 +92,7 @@ config HAS_VMAP
> >>>>>  config MEM_ACCESS_ALWAYS_ON
> >>>>>       bool
> >>>>>
> >>>>> -config MEM_ACCESS
> >>>>> +config VM_EVENT
> >>>>>       def_bool MEM_ACCESS_ALWAYS_ON
> >>>>>       prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
> >>>>>       depends on HVM
> >>>>
> >>>> What about MEM_ACCESS_ALWAYS_ON (visible in patch context)? Shouldn't that
> >>>> become VM_EVENT_ALWAYS_ON then, too?
> >>>>
> >>>> Further, what about MEM_PAGING and MEM_SHARING? Shouldn't those, at least
> >>>> documentation purposes, then also gain a dependency on VM_EVENT?
> >>>
> >>> MEM_PAGING, yes. MEM_SHARING, definitely not. MEM_SHARING is perfectly
> >>> functional without vm_event.
> >>
> >> Is it? I see e.g.
> >>
> >>     if ( sharing_enomem )
> >>     {
> >> #ifdef CONFIG_MEM_SHARING
> >>         if ( !vm_event_check_ring(currd->vm_event_share) )
> >>         {
> >>             gprintk(XENLOG_ERR, "Domain %pd attempt to unshare "
> >>                     "gfn %lx, ENOMEM and no helper\n",
> >>                     currd, gfn);
> >>             /* Crash the domain */
> >>             rc = 0;
> >>         }
> >> #endif
> >>     }
> >
> > On x86 vm_event is always compiled in as per current setup. If we were
> > to make that dependent on the now renamed config option this here
> > should be converted to CONFIG_MEM_SHARING && CONFIG_VM_EVENT. The rest
> > of the mem_sharing codebase does not require vm_event to function,
> > this here is used only if there is a subscriber to the enomem
> > corner-case. It isn't normally used.
>
> I see.
>
> >> in hvm_hap_nested_page_fault().
> >>
> >> Also - you responded only to a secondary remark here. What about the
> >> more basic points further up?
> >
> > My recommendation to use CONFIG_VM_EVENT for the
> > vm_event/mem_access/monitor subsystems strictly only applies to ARM
> > where these three subsystems have a 1:1:1 dependency. On x86 the
> > dependency between the three can be more complex, I would not change
> > the x86 side of things unless we want to get the three subsystems
> > their own kconfig options.
>
> Then why did you ack the patch, which clearly extends things to x86 as
> well? Iirc my suggestion was to indeed go with separate options (hence
> why I think the Suggested-by: here is wrong; see context near the top).

Because I'm fine with the level of impact this single renaming has on
the x86 codebase. I just don't want to start renaming other x86
specific config options or combining them into a single one because
the interactions between the sharing/paging/access/monitor/vm_event is
fairly tangled and would require a bit more careful consideration.

Tamas

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Jan Beulich]

On 21.01.2025 11:19, Sergiy Kibrik wrote:
> @@ -58,7 +58,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
>      return NULL;
>  }
>  
> -#endif /*CONFIG_MEM_ACCESS*/
> +#endif /*CONFIG_VM_EVENT*/

Oh, also - as you touch this anyway: Would you mind adding the mising blanks,
just like we have them ...

>  #endif /* _ASM_ARM_MEM_ACCESS_H */

... on the immediately following line?

Jan

Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT

[Tamas K Lengyel]

On Tue, Jan 21, 2025 at 5:19 AM Sergiy Kibrik <Sergiy_Kibrik@epam.com> wrote:
>
> Use more generic CONFIG_VM_EVENT name throughout Xen code instead of
> CONFIG_MEM_ACCESS. This reflects the fact that vm_event is a higher level
> feature, with mem_access & monitor depending on it.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Suggested-by: Tamas K Lengyel <tamas@tklengyel.com>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

[PATCH v2 2/4] x86:monitor: control monitor.c build with CONFIG_VM_EVENT option

[Sergiy Kibrik]

Replace more general CONFIG_HVM option with CONFIG_VM_EVENT which is more
relevant and specific to monitoring. This is only to clarify at build level
to which subsystem this file belongs.

No functional change here, as VM_EVENT depends on HVM.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/x86/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b35fd5196c..8b3c17d689 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -49,7 +49,7 @@ obj-$(CONFIG_PV) += ioport_emulate.o
 obj-y += irq.o
 obj-$(CONFIG_KEXEC) += machine_kexec.o
 obj-y += mm.o x86_64/mm.o
-obj-$(CONFIG_HVM) += monitor.o
+obj-$(CONFIG_VM_EVENT) += monitor.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
-- 
2.25.1

Re: [PATCH v2 2/4] x86:monitor: control monitor.c build with CONFIG_VM_EVENT option

[Jan Beulich]

On 21.01.2025 11:21, Sergiy Kibrik wrote:
> Replace more general CONFIG_HVM option with CONFIG_VM_EVENT which is more
> relevant and specific to monitoring. This is only to clarify at build level
> to which subsystem this file belongs.
> 
> No functional change here, as VM_EVENT depends on HVM.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>

As long as patch 1 stays roughly as it is right now:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[PATCH v2 3/4] automation: rename CONFIG_MEM_ACCESS -> CONFIG_VM_EVENT

[Sergiy Kibrik]

Following the renaming of Xen build option.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 automation/eclair_analysis/xen_arm_config | 2 +-
 automation/eclair_analysis/xen_x86_config | 2 +-
 automation/gitlab-ci/build.yaml           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/automation/eclair_analysis/xen_arm_config b/automation/eclair_analysis/xen_arm_config
index ef140ceb73..4b01ef51c5 100644
--- a/automation/eclair_analysis/xen_arm_config
+++ b/automation/eclair_analysis/xen_arm_config
@@ -63,7 +63,7 @@ CONFIG_HAS_DEVICE_TREE=y
 CONFIG_HAS_FAST_MULTIPLY=y
 CONFIG_HAS_PDX=y
 CONFIG_HAS_PMAP=y
-# CONFIG_MEM_ACCESS is not set
+# CONFIG_VM_EVENT is not set
 CONFIG_STATIC_MEMORY=y
 
 #
diff --git a/automation/eclair_analysis/xen_x86_config b/automation/eclair_analysis/xen_x86_config
index abc44d43e1..9da3264dd0 100644
--- a/automation/eclair_analysis/xen_x86_config
+++ b/automation/eclair_analysis/xen_x86_config
@@ -54,7 +54,7 @@ CONFIG_HAS_PDX=y
 CONFIG_HAS_SCHED_GRANULARITY=y
 CONFIG_HAS_UBSAN=y
 CONFIG_MEM_ACCESS_ALWAYS_ON=y
-CONFIG_MEM_ACCESS=y
+CONFIG_VM_EVENT=y
 CONFIG_NEEDS_LIBELF=y
 CONFIG_NUMA=y
 
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index bc4a8a5ad2..ed65e2edd7 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -741,7 +741,7 @@ debian-12-riscv64-gcc:
       CONFIG_EXPERT=y
       CONFIG_GRANT_TABLE=n
       CONFIG_LIVEPATCH=n
-      CONFIG_MEM_ACCESS=n
+      CONFIG_VM_EVENT=n
       CONFIG_QEMU_PLATFORM=y
       CONFIG_XSM=n
 
-- 
2.25.1

Re: [PATCH v2 3/4] automation: rename CONFIG_MEM_ACCESS -> CONFIG_VM_EVENT

[Nicola Vetrini]

On 2025-01-21 11:23, Sergiy Kibrik wrote:
> Following the renaming of Xen build option.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>

For the ECLAIR part:
Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

> ---
>  automation/eclair_analysis/xen_arm_config | 2 +-
>  automation/eclair_analysis/xen_x86_config | 2 +-
>  automation/gitlab-ci/build.yaml           | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/automation/eclair_analysis/xen_arm_config 
> b/automation/eclair_analysis/xen_arm_config
> index ef140ceb73..4b01ef51c5 100644
> --- a/automation/eclair_analysis/xen_arm_config
> +++ b/automation/eclair_analysis/xen_arm_config
> @@ -63,7 +63,7 @@ CONFIG_HAS_DEVICE_TREE=y
>  CONFIG_HAS_FAST_MULTIPLY=y
>  CONFIG_HAS_PDX=y
>  CONFIG_HAS_PMAP=y
> -# CONFIG_MEM_ACCESS is not set
> +# CONFIG_VM_EVENT is not set
>  CONFIG_STATIC_MEMORY=y
> 
>  #
> diff --git a/automation/eclair_analysis/xen_x86_config 
> b/automation/eclair_analysis/xen_x86_config
> index abc44d43e1..9da3264dd0 100644
> --- a/automation/eclair_analysis/xen_x86_config
> +++ b/automation/eclair_analysis/xen_x86_config
> @@ -54,7 +54,7 @@ CONFIG_HAS_PDX=y
>  CONFIG_HAS_SCHED_GRANULARITY=y
>  CONFIG_HAS_UBSAN=y
>  CONFIG_MEM_ACCESS_ALWAYS_ON=y
> -CONFIG_MEM_ACCESS=y
> +CONFIG_VM_EVENT=y
>  CONFIG_NEEDS_LIBELF=y
>  CONFIG_NUMA=y
> 
> diff --git a/automation/gitlab-ci/build.yaml 
> b/automation/gitlab-ci/build.yaml
> index bc4a8a5ad2..ed65e2edd7 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -741,7 +741,7 @@ debian-12-riscv64-gcc:
>        CONFIG_EXPERT=y
>        CONFIG_GRANT_TABLE=n
>        CONFIG_LIVEPATCH=n
> -      CONFIG_MEM_ACCESS=n
> +      CONFIG_VM_EVENT=n
>        CONFIG_QEMU_PLATFORM=y
>        CONFIG_XSM=n

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [PATCH v2 3/4] automation: rename CONFIG_MEM_ACCESS -> CONFIG_VM_EVENT

[Jan Beulich]

On 21.01.2025 11:23, Sergiy Kibrik wrote:
> Following the renaming of Xen build option.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
>  automation/eclair_analysis/xen_arm_config | 2 +-
>  automation/eclair_analysis/xen_x86_config | 2 +-
>  automation/gitlab-ci/build.yaml           | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

This can't really be separated from the changes doing the actual rename,
can it? Aiui the build (randconfig ones in particular) may break between
the two patches, or what is being tested may end up being different.

Jan

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

[Sergiy Kibrik]

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

[Jan Beulich]

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

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

[Tamas K Lengyel]

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

[Jan Beulich]

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