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
vm_event_is_enabled() condition check 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.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v3:
- a comment next to the excessive condition
- use vm_event_is_enabled() instead
- avoid heavy churn by using the inverted condition plus break
---
xen/arch/x86/hvm/hvm.c | 25 ++++++++++++++++++++++++-
xen/arch/x86/include/asm/mem_access.h | 10 ++++++++++
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index aa14101241..8509b53585 100644
--- 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>
@@ -2080,7 +2081,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
#endif
}
- if ( req_ptr )
+ /*
+ * Excessive condition is to avoid runtime undefined error only
+ * when CONFIG_USBAN=y
+ */
+ if ( req_ptr && vm_event_is_enabled(curr) )
{
if ( monitor_traps(curr, sync, req_ptr) < 0 )
rc = 0;
@@ -4802,6 +4807,12 @@ static int do_altp2m_op(
break;
case HVMOP_altp2m_set_mem_access:
+ if ( !vm_event_is_enabled(current) )
+ {
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
if ( a.u.mem_access.pad )
rc = -EINVAL;
else
@@ -4811,6 +4822,12 @@ static int do_altp2m_op(
break;
case HVMOP_altp2m_set_mem_access_multi:
+ if ( !vm_event_is_enabled(current) )
+ {
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
if ( a.u.set_mem_access_multi.pad ||
a.u.set_mem_access_multi.opaque > a.u.set_mem_access_multi.nr )
{
@@ -4842,6 +4859,12 @@ static int do_altp2m_op(
break;
case HVMOP_altp2m_get_mem_access:
+ if ( !vm_event_is_enabled(current) )
+ {
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
if ( a.u.mem_access.pad )
rc = -EINVAL;
else
diff --git a/xen/arch/x86/include/asm/mem_access.h b/xen/arch/x86/include/asm/mem_access.h
index 257ed33de1..790bed81e8 100644
--- a/xen/arch/x86/include/asm/mem_access.h
+++ b/xen/arch/x86/include/asm/mem_access.h
@@ -14,6 +14,7 @@
#ifndef __ASM_X86_MEM_ACCESS_H__
#define __ASM_X86_MEM_ACCESS_H__
+#ifdef CONFIG_VM_EVENT
/*
* Setup vm_event request based on the access (gla is -1ull if not available).
* Handles the rw2rx conversion. Boolean return value indicates if event type
@@ -25,6 +26,15 @@
bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
struct npfec npfec,
struct vm_event_st **req_ptr);
+#else
+static inline bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
+ struct npfec npfec,
+ struct vm_event_st **req_ptr)
+{
+ *req_ptr = NULL;
+ return false;
+}
+#endif /* CONFIG_VM_EVENT */
/* Check for emulation and mark vcpu for skipping one instruction
* upon rescheduling if required. */
--
2.34.1On 21.11.2025 10:15, Penny Zheng wrote:
> @@ -2080,7 +2081,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> #endif
> }
>
> - if ( req_ptr )
> + /*
> + * Excessive condition is to avoid runtime undefined error only
> + * when CONFIG_USBAN=y
> + */
> + if ( req_ptr && vm_event_is_enabled(curr) )
> {
I fear the comment isn't really helpful this way. What's "excessive" here may
be clear from patch context, but it won't be clear when looking at the code
later. Nor would it then be immediately clear why the vm_event_is_enabled()
check is (seemingly) unnecessary. How about this:
"req_ptr being constant NULL when !CONFIG_VM_EVENT, CONFIG_UBSAN=y builds
have been observed to still hit undefined-ness at runtime. Hence do a
seemingly redundant vm_event_is_enabled() check here."
With this or any other suitable improvement to the comment:
Acked-by: Jan Beulich <jbeulich@suse.com>
If we want to go with the suggestion above, I'd be happy to do the replacement
while committing. But of course first the necessary 2nd ack will want
collecting.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, November 24, 2025 10:10 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; grygorii_strashko@epam.com; Andrew
> Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Tamas K Lengyel <tamas@tklengyel.com>; Alexandru Isaila
> <aisaila@bitdefender.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v3 6/7] xen/mem_access: wrap memory access when
> VM_EVENT=n
>
> On 21.11.2025 10:15, Penny Zheng wrote:
> > @@ -2080,7 +2081,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> > unsigned long gla, #endif
> > }
> >
> > - if ( req_ptr )
> > + /*
> > + * Excessive condition is to avoid runtime undefined error only
> > + * when CONFIG_USBAN=y
> > + */
> > + if ( req_ptr && vm_event_is_enabled(curr) )
> > {
>
> I fear the comment isn't really helpful this way. What's "excessive" here may be
> clear from patch context, but it won't be clear when looking at the code later. Nor
> would it then be immediately clear why the vm_event_is_enabled() check is
> (seemingly) unnecessary. How about this:
>
> "req_ptr being constant NULL when !CONFIG_VM_EVENT, CONFIG_UBSAN=y
> builds have been observed to still hit undefined-ness at runtime. Hence do a
> seemingly redundant vm_event_is_enabled() check here."
>
> With this or any other suitable improvement to the comment:
> Acked-by: Jan Beulich <jbeulich@suse.com> If we want to go with the suggestion
> above, I'd be happy to do the replacement while committing. But of course first the
> necessary 2nd ack will want collecting.
Thx!
>
> Jan
© 2016 - 2025 Red Hat, Inc.