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
