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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Jan 31, 2025 at 1:30 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 31.01.2025 01:26, Tamas K Lengyel wrote:
> > On Thu, Jan 30, 2025 at 8:24 AM Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxx>
> >>
> >> I don't think this is applicable; my suggestion went in a different 
> >> direction.
> >>
> >>> Suggested-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> >>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> >>
> >> 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