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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Feb 3, 2025 at 2:36 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 01.02.2025 00:36, Tamas K Lengyel wrote:
> > 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.
>
> 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