Re: [PATCH v1 6/7] xen/mem_access: wrap memory access when VM_EVENT=n

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2025 04:16, Penny Zheng wrote:
> Feature memory access is based on vm event subsystem, and it could be disabled
> in the future. So a few switch-blocks in do_altp2m_op() need
> IS_ENABLED(CONFIG_VM_EVENT) wrapping to pass compilation when ALTP2M=y and
> VM_EVENT=n(, hence MEM_ACCESS=n), like HVMOP_altp2m_set_mem_access, etc.
> Function p2m_mem_access_check() still needs stub when VM_EVENT=n to
> pass compilation.
> Although local variable "req_ptr" still remains NULL throughout its lifetime,
> with the change of NULL assignment, we will face runtime undefined error only
> when CONFIG_USBAN is on. So we strengthen the condition check via adding
> vm_event_is_enabled() for the special case.

As to this, didn't I ask for ...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -52,6 +52,7 @@
>  #include <asm/i387.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/mce.h>
> +#include <asm/mem_access.h>
>  #include <asm/monitor.h>
>  #include <asm/msr.h>
>  #include <asm/mtrr.h>
> @@ -2081,7 +2082,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>  #endif
>      }
>  
> -    if ( req_ptr )
> +    if ( req_ptr && vm_event_is_enabled(curr) )
>      {
>          if ( monitor_traps(curr, sync, req_ptr) < 0 )
>              rc = 0;

... a comment next to the (now seemingly excessive) condition?

> @@ -4862,58 +4863,70 @@ static int do_altp2m_op(
>          break;
>  
>      case HVMOP_altp2m_set_mem_access:
> -        if ( a.u.mem_access.pad )
> -            rc = -EINVAL;
> -        else
> -            rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
> -                                    a.u.mem_access.access,
> -                                    a.u.mem_access.view);
> +        if ( IS_ENABLED(CONFIG_VM_EVENT) )

Wouldn't this then better be vm_event_is_enabled()? Or can this (and the ones 
below)
be carried out ahead of enabling? Tamas?

In any event, here and even more so ...

> +        {
> +            if ( a.u.mem_access.pad )
> +                rc = -EINVAL;
> +            else
> +                rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
> +                                        a.u.mem_access.access,
> +                                        a.u.mem_access.view);
> +        }
>          break;
>  
>      case HVMOP_altp2m_set_mem_access_multi:
> -        if ( a.u.set_mem_access_multi.pad ||
> -             a.u.set_mem_access_multi.opaque > a.u.set_mem_access_multi.nr )
> +        if ( IS_ENABLED(CONFIG_VM_EVENT) )

... here please avoid this heavy churn by using the inverted condition plus 
break;
In all cases you fiddle with rc also needs setting to a suitable error 
indicator,
e.g. -EOPNOTSUPP, I think.

Jan