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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
    }

in hvm_hap_nested_page_fault().

Also - you responded only to a secondary remark here. What about the
more basic points further up?

Jan