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

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

Sergiy Kibrik posted 4 patches 9 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT
Posted by Sergiy Kibrik 9 months, 2 weeks ago 
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
Posted by Tamas K Lengyel 9 months ago 
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>
Re: [PATCH v2 1/4] xen: kconfig: rename MEM_ACCESS -> VM_EVENT
Posted by Jan Beulich 9 months ago 
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
Posted by Jan Beulich 9 months ago 
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
Posted by Tamas K Lengyel 9 months ago 
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
Posted by Jan Beulich 9 months ago 
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
Posted by Tamas K Lengyel 9 months ago 
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
Posted by Jan Beulich 9 months ago 
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
Posted by Tamas K Lengyel 9 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.