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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan