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
}
in hvm_hap_nested_page_fault().
Also - you responded only to a secondary remark here. What about the
more basic points further up?
Jan
