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