[PATCH v1 0/7] consolidate vm event subsystem

[PATCH v1 0/7] consolidate vm event subsystem

[Penny Zheng]

This patch serie originates from "Disable domctl-op via CONFIG_MGMT_HYPERCALLS"
[1], and focuses on consolidating vm event subsystem (i.e. VM_EVENT), and its
derived features, like memory paging, etc.

[1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg200843.html

Penny Zheng (7):
  xen/svm: limit the scope of "rc"
  xen/vm_event: introduce vm_event_is_enabled()
  xen/monitor: wrap monitor_op under CONFIG_VM_EVENT
  xen/p2m: move xenmem_access_to_p2m_access() to common p2m.c
  xen/x86: move declaration from mem_access.h to altp2m.h
  xen/mem_access: wrap memory access when VM_EVENT=n
  xen/vm_event: consolidate CONFIG_VM_EVENT

 xen/arch/x86/Makefile                 |   2 +-
 xen/arch/x86/hvm/Kconfig              |   1 -
 xen/arch/x86/hvm/Makefile             |   4 +-
 xen/arch/x86/hvm/emulate.c            |  67 +++++++-------
 xen/arch/x86/hvm/hvm.c                | 123 ++++++++++++++++----------
 xen/arch/x86/hvm/svm/intr.c           |   2 +-
 xen/arch/x86/hvm/svm/svm.c            |  82 +++++++++++------
 xen/arch/x86/hvm/vmx/intr.c           |   2 +-
 xen/arch/x86/hvm/vmx/vmx.c            |  73 +++++++++------
 xen/arch/x86/include/asm/altp2m.h     |  10 +++
 xen/arch/x86/include/asm/hvm/hvm.h    |  18 ++--
 xen/arch/x86/include/asm/mem_access.h |  20 ++---
 xen/arch/x86/include/asm/monitor.h    |   9 ++
 xen/arch/x86/include/asm/vm_event.h   |   8 ++
 xen/arch/x86/mm/mem_access.c          |  36 --------
 xen/arch/x86/mm/mem_sharing.c         |   3 +
 xen/arch/x86/mm/p2m.c                 |  38 ++++++++
 xen/common/Kconfig                    |   7 +-
 xen/include/xen/vm_event.h            |   7 ++
 19 files changed, 319 insertions(+), 193 deletions(-)

-- 
2.34.1

[PATCH v1 3/7] xen/monitor: wrap monitor_op under CONFIG_VM_EVENT

[Penny Zheng]

Feature monitor_op is based on vm event subsystem, so monitor.o shall be
wrapped under CONFIG_VM_EVENT.
The following functions are only invoked by monitor-op, so they all shall be
wrapped with CONFIG_VM_EVENT (otherwise they will become unreachable and
violate Misra rule 2.1 when VM_EVENT=n):
- hvm_enable_msr_interception
  - hvm_function_table.enable_msr_interception
- hvm_has_set_descriptor_access_existing
  - hvm_function_table.set_descriptor_access_existi
- arch_monitor_get_capabilities
Function monitored_msr() still needs a stub to pass compilation when
VM_EVENT=n.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v3 -> v4:
- a new commit split from previous "xen/vm_event: consolidate CONFIG_VM_EVENT"
- Another blank line ahead of the #ifdef
- Move hvm_enable_msr_interception() up into the earlier #ifdef
- only arch_monitor_get_capabilities() needs wrapping, as this static inline
function calls hvm_has_set_descriptor_access_exiting(), which is declared only
when VM_EVENT=y
---
 xen/arch/x86/hvm/Makefile          |  2 +-
 xen/arch/x86/hvm/svm/svm.c         |  8 +++++++-
 xen/arch/x86/hvm/vmx/vmx.c         | 10 ++++++++++
 xen/arch/x86/include/asm/hvm/hvm.h | 18 +++++++++++-------
 xen/arch/x86/include/asm/monitor.h |  9 +++++++++
 5 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 6ec2c8f2db..50e0b6e63b 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -16,7 +16,7 @@ obj-y += io.o
 obj-y += ioreq.o
 obj-y += irq.o
 obj-y += mmio.o
-obj-y += monitor.o
+obj-$(CONFIG_VM_EVENT) += monitor.o
 obj-y += mtrr.o
 obj-y += nestedhvm.o
 obj-y += pmtimer.o
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 9de2fd950e..06e4572d89 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -299,6 +299,7 @@ void svm_intercept_msr(struct vcpu *v, uint32_t msr, int flags)
         __clear_bit(msr * 2 + 1, msr_bit);
 }
 
+#ifdef CONFIG_VM_EVENT
 static void cf_check svm_enable_msr_interception(struct domain *d, uint32_t msr)
 {
     struct vcpu *v;
@@ -306,6 +307,7 @@ static void cf_check svm_enable_msr_interception(struct domain *d, uint32_t msr)
     for_each_vcpu ( d, v )
         svm_intercept_msr(v, msr, MSR_INTERCEPT_WRITE);
 }
+#endif /* CONFIG_VM_EVENT */
 
 static void svm_save_dr(struct vcpu *v)
 {
@@ -826,6 +828,7 @@ static void cf_check svm_set_rdtsc_exiting(struct vcpu *v, bool enable)
     vmcb_set_general2_intercepts(vmcb, general2_intercepts);
 }
 
+#ifdef CONFIG_VM_EVENT
 static void cf_check svm_set_descriptor_access_exiting(
     struct vcpu *v, bool enable)
 {
@@ -843,6 +846,7 @@ static void cf_check svm_set_descriptor_access_exiting(
 
     vmcb_set_general1_intercepts(vmcb, general1_intercepts);
 }
+#endif /* CONFIG_VM_EVENT */
 
 static unsigned int cf_check svm_get_insn_bytes(struct vcpu *v, uint8_t *buf)
 {
@@ -2457,9 +2461,11 @@ static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
     .fpu_dirty_intercept  = svm_fpu_dirty_intercept,
     .msr_read_intercept   = svm_msr_read_intercept,
     .msr_write_intercept  = svm_msr_write_intercept,
+#ifdef CONFIG_VM_EVENT
     .enable_msr_interception = svm_enable_msr_interception,
-    .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
     .set_descriptor_access_exiting = svm_set_descriptor_access_exiting,
+#endif
+    .set_rdtsc_exiting    = svm_set_rdtsc_exiting,
     .get_insn_bytes       = svm_get_insn_bytes,
 
     .nhvm_vcpu_initialise = nsvm_vcpu_initialise,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2e9da26016..d29c9a2ff2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1520,6 +1520,7 @@ static void cf_check vmx_set_rdtsc_exiting(struct vcpu *v, bool enable)
     vmx_vmcs_exit(v);
 }
 
+#ifdef CONFIG_VM_EVENT
 static void cf_check vmx_set_descriptor_access_exiting(
     struct vcpu *v, bool enable)
 {
@@ -1534,6 +1535,7 @@ static void cf_check vmx_set_descriptor_access_exiting(
     vmx_update_secondary_exec_control(v);
     vmx_vmcs_exit(v);
 }
+#endif /* CONFIG_VM_EVENT */
 
 static void cf_check vmx_init_hypercall_page(void *p)
 {
@@ -2413,6 +2415,7 @@ static void cf_check vmx_handle_eoi(uint8_t vector, int isr)
         printk_once(XENLOG_WARNING "EOI for %02x but SVI=%02x\n", vector, old_svi);
 }
 
+#ifdef CONFIG_VM_EVENT
 static void cf_check vmx_enable_msr_interception(struct domain *d, uint32_t msr)
 {
     struct vcpu *v;
@@ -2420,6 +2423,7 @@ static void cf_check vmx_enable_msr_interception(struct domain *d, uint32_t msr)
     for_each_vcpu ( d, v )
         vmx_set_msr_intercept(v, msr, VMX_MSR_W);
 }
+#endif /* CONFIG_VM_EVENT */
 
 #ifdef CONFIG_ALTP2M
 
@@ -2871,7 +2875,9 @@ static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
     .nhvm_domain_relinquish_resources = nvmx_domain_relinquish_resources,
     .update_vlapic_mode = vmx_vlapic_msr_changed,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
+#ifdef CONFIG_VM_EVENT
     .enable_msr_interception = vmx_enable_msr_interception,
+#endif
 #ifdef CONFIG_ALTP2M
     .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp,
     .altp2m_vcpu_update_vmfunc_ve = vmx_vcpu_update_vmfunc_ve,
@@ -3079,9 +3085,11 @@ const struct hvm_function_table * __init start_vmx(void)
 
     vmx_function_table.caps.singlestep = cpu_has_monitor_trap_flag;
 
+#ifdef CONFIG_VM_EVENT
     if ( cpu_has_vmx_dt_exiting )
         vmx_function_table.set_descriptor_access_exiting =
             vmx_set_descriptor_access_exiting;
+#endif
 
     /*
      * Do not enable EPT when (!cpu_has_vmx_pat), to prevent security hole
@@ -3152,8 +3160,10 @@ void __init vmx_fill_funcs(void)
     if ( !cpu_has_xen_ibt )
         return;
 
+#ifdef CONFIG_VM_EVENT
     vmx_function_table.set_descriptor_access_exiting =
         vmx_set_descriptor_access_exiting;
+#endif
 
     vmx_function_table.update_eoi_exit_bitmap = vmx_update_eoi_exit_bitmap;
     vmx_function_table.process_isr            = vmx_process_isr;
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h
index f02183691e..da00ed0694 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -192,7 +192,11 @@ struct hvm_function_table {
     void (*handle_cd)(struct vcpu *v, unsigned long value);
     void (*set_info_guest)(struct vcpu *v);
     void (*set_rdtsc_exiting)(struct vcpu *v, bool enable);
+
+#ifdef CONFIG_VM_EVENT
     void (*set_descriptor_access_exiting)(struct vcpu *v, bool enable);
+    void (*enable_msr_interception)(struct domain *d, uint32_t msr);
+#endif
 
     /* Nested HVM */
     int (*nhvm_vcpu_initialise)(struct vcpu *v);
@@ -224,8 +228,6 @@ struct hvm_function_table {
                                 paddr_t *L1_gpa, unsigned int *page_order,
                                 uint8_t *p2m_acc, struct npfec npfec);
 
-    void (*enable_msr_interception)(struct domain *d, uint32_t msr);
-
 #ifdef CONFIG_ALTP2M
     /* Alternate p2m */
     void (*altp2m_vcpu_update_p2m)(struct vcpu *v);
@@ -433,11 +435,18 @@ static inline bool using_svm(void)
 
 #define hvm_long_mode_active(v) (!!((v)->arch.hvm.guest_efer & EFER_LMA))
 
+#ifdef CONFIG_VM_EVENT
 static inline bool hvm_has_set_descriptor_access_exiting(void)
 {
     return hvm_funcs.set_descriptor_access_exiting;
 }
 
+static inline void hvm_enable_msr_interception(struct domain *d, uint32_t msr)
+{
+    alternative_vcall(hvm_funcs.enable_msr_interception, d, msr);
+}
+#endif /* CONFIG_VM_EVENT */
+
 static inline void hvm_domain_creation_finished(struct domain *d)
 {
     if ( hvm_funcs.domain_creation_finished )
@@ -679,11 +688,6 @@ static inline int nhvm_hap_walk_L1_p2m(
         v, L2_gpa, L1_gpa, page_order, p2m_acc, npfec);
 }
 
-static inline void hvm_enable_msr_interception(struct domain *d, uint32_t msr)
-{
-    alternative_vcall(hvm_funcs.enable_msr_interception, d, msr);
-}
-
 static inline bool hvm_is_singlestep_supported(void)
 {
     return hvm_funcs.caps.singlestep;
diff --git a/xen/arch/x86/include/asm/monitor.h b/xen/arch/x86/include/asm/monitor.h
index 3c64d8258f..9249324fd0 100644
--- a/xen/arch/x86/include/asm/monitor.h
+++ b/xen/arch/x86/include/asm/monitor.h
@@ -71,6 +71,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
     return rc;
 }
 
+#ifdef CONFIG_VM_EVENT
 static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
 {
     uint32_t capabilities = 0;
@@ -102,6 +103,7 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
 
     return capabilities;
 }
+#endif /* CONFIG_VM_EVENT */
 
 int arch_monitor_domctl_event(struct domain *d,
                               struct xen_domctl_monitor_op *mop);
@@ -123,7 +125,14 @@ static inline void arch_monitor_cleanup_domain(struct domain *d) {}
 
 #endif
 
+#ifdef CONFIG_VM_EVENT
 bool monitored_msr(const struct domain *d, u32 msr);
+#else
+static inline bool monitored_msr(const struct domain *d, u32 msr)
+{
+    return false;
+}
+#endif
 bool monitored_msr_onchangeonly(const struct domain *d, u32 msr);
 
 #endif /* __ASM_X86_MONITOR_H__ */
-- 
2.34.1

[PATCH v1 4/7] xen/p2m: move xenmem_access_to_p2m_access() to common p2m.c

[Penny Zheng]

Memory access and ALTP2M are two seperate features, while both depending on
helper xenmem_access_to_p2m_access(). So it betters lives in common p2m.c,
other than mem_access.c which will be compiled out when VM_EVENT=n && ALTP2M=y.
Coding style has been corrected at the same time.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v3 -> v4:
- new commit
---
 xen/arch/x86/mm/mem_access.c | 36 ----------------------------------
 xen/arch/x86/mm/p2m.c        | 38 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index e6b609064c..e55e53f44c 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -298,42 +298,6 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
     return rc;
 }
 
-bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
-                                 xenmem_access_t xaccess,
-                                 p2m_access_t *paccess)
-{
-    static const p2m_access_t memaccess[] = {
-#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
-        ACCESS(n),
-        ACCESS(r),
-        ACCESS(w),
-        ACCESS(rw),
-        ACCESS(x),
-        ACCESS(rx),
-        ACCESS(wx),
-        ACCESS(rwx),
-        ACCESS(rx2rw),
-        ACCESS(n2rwx),
-        ACCESS(r_pw),
-#undef ACCESS
-    };
-
-    switch ( xaccess )
-    {
-    case 0 ... ARRAY_SIZE(memaccess) - 1:
-        xaccess = array_index_nospec(xaccess, ARRAY_SIZE(memaccess));
-        *paccess = memaccess[xaccess];
-        break;
-    case XENMEM_access_default:
-        *paccess = p2m->default_access;
-        break;
-    default:
-        return false;
-    }
-
-    return true;
-}
-
 /*
  * Set access type for a region of gfns.
  * If gfn == INVALID_GFN, sets the default access type.
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e2a00a0efd..f2bf5ef8d3 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2189,6 +2189,44 @@ void p2m_log_dirty_range(struct domain *d, unsigned long begin_pfn,
     guest_flush_tlb_mask(d, d->dirty_cpumask);
 }
 
+bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
+                                 xenmem_access_t xaccess,
+                                 p2m_access_t *paccess)
+{
+    static const p2m_access_t memaccess[] = {
+#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
+        ACCESS(n),
+        ACCESS(r),
+        ACCESS(w),
+        ACCESS(rw),
+        ACCESS(x),
+        ACCESS(rx),
+        ACCESS(wx),
+        ACCESS(rwx),
+        ACCESS(rx2rw),
+        ACCESS(n2rwx),
+        ACCESS(r_pw),
+#undef ACCESS
+    };
+
+    switch ( xaccess )
+    {
+    case 0 ... ARRAY_SIZE(memaccess) - 1:
+        xaccess = array_index_nospec(xaccess, ARRAY_SIZE(memaccess));
+        *paccess = memaccess[xaccess];
+        break;
+
+    case XENMEM_access_default:
+        *paccess = p2m->default_access;
+        break;
+
+    default:
+        return false;
+    }
+
+    return true;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

[PATCH v1 5/7] xen/x86: move declaration from mem_access.h to altp2m.h

[Penny Zheng]

Memory access and ALTP2M are two seperate features, and each could be
controlled via VM_EVENT or ALTP2M. In order to avoid implicit declaration
when ALTP2M=y and VM_EVENT=n on compiling hvm.o/altp2m.o, we move declaration
of the following functions from <asm/mem_access.h> to <asm/altp2m.h>:
- p2m_set_suppress_ve
- p2m_set_suppress_ve_multi
- p2m_get_suppress_ve
Potential error on altp2m.c also breaks Misra Rule 8.4.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v3 -> v4:
- new commit
---
 xen/arch/x86/include/asm/altp2m.h     | 10 ++++++++++
 xen/arch/x86/include/asm/mem_access.h | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h
index 8ecd74f363..9c1ac3cc26 100644
--- a/xen/arch/x86/include/asm/altp2m.h
+++ b/xen/arch/x86/include/asm/altp2m.h
@@ -46,6 +46,16 @@ void altp2m_vcpu_destroy(struct vcpu *v);
 int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn);
 void altp2m_vcpu_disable_ve(struct vcpu *v);
 
+int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
+                        unsigned int altp2m_idx);
+
+struct xen_hvm_altp2m_suppress_ve_multi;
+int p2m_set_suppress_ve_multi(struct domain *d,
+                              struct xen_hvm_altp2m_suppress_ve_multi *sve);
+
+int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
+                        unsigned int altp2m_idx);
+
 #else
 
 static inline bool altp2m_is_eptp_valid(const struct domain *d,
diff --git a/xen/arch/x86/include/asm/mem_access.h b/xen/arch/x86/include/asm/mem_access.h
index 1a52a10322..257ed33de1 100644
--- a/xen/arch/x86/include/asm/mem_access.h
+++ b/xen/arch/x86/include/asm/mem_access.h
@@ -34,16 +34,6 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
 /* Sanity check for mem_access hardware support */
 bool p2m_mem_access_sanity_check(const struct domain *d);
 
-int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
-                        unsigned int altp2m_idx);
-
-struct xen_hvm_altp2m_suppress_ve_multi;
-int p2m_set_suppress_ve_multi(struct domain *d,
-                              struct xen_hvm_altp2m_suppress_ve_multi *sve);
-
-int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
-                        unsigned int altp2m_idx);
-
 #endif /*__ASM_X86_MEM_ACCESS_H__ */
 
 /*
-- 
2.34.1

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

[Penny Zheng]

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.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v3 -> v4:
- a new commit split from previous "xen/vm_event: consolidate CONFIG_VM_EVENT"
- use IS_ENABLED() to replace #ifdef
- remove unnecessary changes in compat_altp2m_op()
- strengthen the condition check to fix runtime undefined error when
CONFIG_USBAN=y
---
 xen/arch/x86/hvm/hvm.c                | 97 +++++++++++++++------------
 xen/arch/x86/include/asm/mem_access.h | 10 +++
 2 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ebadcf2289..c3417b0593 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>
@@ -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;
@@ -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) )
+        {
+            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) )
         {
-            rc = -EINVAL;
-            break;
-        }
+            if ( a.u.set_mem_access_multi.pad ||
+                 a.u.set_mem_access_multi.opaque >
+                    a.u.set_mem_access_multi.nr )
+            {
+                rc = -EINVAL;
+                break;
+            }
 
-        /*
-         * Unlike XENMEM_access_op_set_access_multi, we don't need any bits of
-         * the 'continuation' counter to be zero (to stash a command in).
-         * However, 0x40 is a good 'stride' to make sure that we make
-         * a reasonable amount of forward progress before yielding,
-         * so use a mask of 0x3F here.
-         */
-        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
-                                      a.u.set_mem_access_multi.access_list,
-                                      a.u.set_mem_access_multi.nr,
-                                      a.u.set_mem_access_multi.opaque,
-                                      0x3F,
-                                      a.u.set_mem_access_multi.view);
-        if ( rc > 0 )
-        {
-            a.u.set_mem_access_multi.opaque = rc;
-            rc = -ERESTART;
-            if ( __copy_field_to_guest(guest_handle_cast(arg, xen_hvm_altp2m_op_t),
-                                       &a, u.set_mem_access_multi.opaque) )
-                rc = -EFAULT;
+            /*
+             * Unlike XENMEM_access_op_set_access_multi, we don't need any
+             * bits of the 'continuation' counter to be zero (to stash a
+             * command in).
+             * However, 0x40 is a good 'stride' to make sure that we make
+             * a reasonable amount of forward progress before yielding,
+             * so use a mask of 0x3F here.
+             */
+            rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
+                                          a.u.set_mem_access_multi.access_list,
+                                          a.u.set_mem_access_multi.nr,
+                                          a.u.set_mem_access_multi.opaque,
+                                          0x3F,
+                                          a.u.set_mem_access_multi.view);
+            if ( rc > 0 )
+            {
+                a.u.set_mem_access_multi.opaque = rc;
+                rc = -ERESTART;
+                if ( __copy_field_to_guest(
+                        guest_handle_cast(arg, xen_hvm_altp2m_op_t),
+                        &a, u.set_mem_access_multi.opaque) )
+                    rc = -EFAULT;
+            }
         }
         break;
 
     case HVMOP_altp2m_get_mem_access:
-        if ( a.u.mem_access.pad )
-            rc = -EINVAL;
-        else
+        if ( IS_ENABLED(CONFIG_VM_EVENT) )
         {
-            xenmem_access_t access;
-
-            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
-                                    a.u.mem_access.view);
-            if ( !rc )
+            if ( a.u.mem_access.pad )
+                rc = -EINVAL;
+            else
             {
-                a.u.mem_access.access = access;
-                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+                xenmem_access_t access;
+
+                rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
+                                        a.u.mem_access.view);
+                if ( !rc )
+                {
+                    a.u.mem_access.access = access;
+                    rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+                }
             }
         }
         break;
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.1

[PATCH v1 7/7] xen/vm_event: consolidate CONFIG_VM_EVENT

[Penny Zheng]

File hvm/vm_event.c and x86/vm_event.c are the extend to vm_event handling
routines, and its compilation shall be guarded by CONFIG_VM_EVENT too.

Although CONFIG_VM_EVENT is right now forcibly enabled on x86 via
MEM_ACCESS_ALWAYS_ON, we could disable it through disabling
CONFIG_MGMT_HYPERCALLS later. So we remove MEM_ACCESS_ALWAYS_ON and
make VM_EVENT=y on default only on x86 to retain the same.

The following functions are developed on the basis of vm event framework, or
only invoked by vm_event.c, so they all shall be wrapped with CONFIG_VM_EVENT
(otherwise they will become unreachable and
violate Misra rule 2.1 when VM_EVENT=n):
- hvm_toggle_singlestep
- hvm_fast_singlestep
- hvm_emulate_one_vm_event
    - hvmemul_write{,cmpxchg,rep_ins,rep_outs,rep_movs,rep_stos,read_io,write_io}_discard
And Function vm_event_check_ring() needs stub to pass compilation when
VM_EVENT=n.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- split out XSM changes
- remove unnecessary stubs
- move "struct p2m_domain" declaration ahead of the #ifdef
---
v2 -> v3:
- move .enable_msr_interception and .set_descriptor_access_exiting together
- with the introduction of "vm_event_is_enabled()", all hvm_monitor_xxx()
stubs are no longer needed
- change to use in-place stubs in do_altp2m_op()
- no need to add stub for monitor_traps(), __vm_event_claim_slot(),
vm_event_put_request() and vm_event_vcpu_pause()
- remove MEM_ACCESS_ALWAYS_ON
- return default p2m_access_rwx for xenmem_access_to_p2m_access() when
VM_EVENT=n
- add wrapping for hvm_emulate_one_vm_event/
hvmemul_write{,cmpxchg,rep_ins,rep_outs,rep_movs,rep_stos,read_io,write_io}_discard
---
 xen/arch/x86/Makefile      |  2 +-
 xen/arch/x86/hvm/Kconfig   |  1 -
 xen/arch/x86/hvm/Makefile  |  2 +-
 xen/arch/x86/hvm/emulate.c | 58 ++++++++++++++++++++------------------
 xen/arch/x86/hvm/hvm.c     |  2 ++
 xen/common/Kconfig         |  7 ++---
 xen/include/xen/vm_event.h |  7 +++++
 7 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 407571c510..f71ca9f18b 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -75,7 +75,7 @@ obj-y += usercopy.o
 obj-y += x86_emulate.o
 obj-$(CONFIG_TBOOT) += tboot.o
 obj-y += hpet.o
-obj-y += vm_event.o
+obj-$(CONFIG_VM_EVENT) += vm_event.o
 obj-y += xstate.o
 
 ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
diff --git a/xen/arch/x86/hvm/Kconfig b/xen/arch/x86/hvm/Kconfig
index f10a2b3744..ea957363a9 100644
--- a/xen/arch/x86/hvm/Kconfig
+++ b/xen/arch/x86/hvm/Kconfig
@@ -4,7 +4,6 @@ menuconfig HVM
 	default !PV_SHIM
 	select COMPAT
 	select IOREQ_SERVER
-	select MEM_ACCESS_ALWAYS_ON
 	help
 	  Interfaces to support HVM domains.  HVM domains require hardware
 	  virtualisation extensions (e.g. Intel VT-x, AMD SVM), but can boot
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 50e0b6e63b..952db00dd7 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -26,7 +26,7 @@ obj-y += save.o
 obj-y += stdvga.o
 obj-y += vioapic.o
 obj-y += vlapic.o
-obj-y += vm_event.o
+obj-$(CONFIG_VM_EVENT) += vm_event.o
 obj-y += vmsi.o
 obj-y += vpic.o
 obj-y += vpt.o
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index fe75b0516d..d56ef02baf 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1615,6 +1615,7 @@ static int cf_check hvmemul_blk(
     return rc;
 }
 
+#ifdef CONFIG_VM_EVENT
 static int cf_check hvmemul_write_discard(
     enum x86_segment seg,
     unsigned long offset,
@@ -1717,6 +1718,7 @@ static int cf_check hvmemul_cache_op_discard(
 {
     return X86EMUL_OKAY;
 }
+#endif /* CONFIG_VM_EVENT */
 
 static int cf_check hvmemul_cmpxchg(
     enum x86_segment seg,
@@ -2750,33 +2752,6 @@ static const struct x86_emulate_ops hvm_emulate_ops = {
     .vmfunc        = hvmemul_vmfunc,
 };
 
-static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
-    .read          = hvmemul_read,
-    .insn_fetch    = hvmemul_insn_fetch,
-    .write         = hvmemul_write_discard,
-    .cmpxchg       = hvmemul_cmpxchg_discard,
-    .rep_ins       = hvmemul_rep_ins_discard,
-    .rep_outs      = hvmemul_rep_outs_discard,
-    .rep_movs      = hvmemul_rep_movs_discard,
-    .rep_stos      = hvmemul_rep_stos_discard,
-    .read_segment  = hvmemul_read_segment,
-    .write_segment = hvmemul_write_segment,
-    .read_io       = hvmemul_read_io_discard,
-    .write_io      = hvmemul_write_io_discard,
-    .read_cr       = hvmemul_read_cr,
-    .write_cr      = hvmemul_write_cr,
-    .read_xcr      = hvmemul_read_xcr,
-    .write_xcr     = hvmemul_write_xcr,
-    .read_msr      = hvmemul_read_msr,
-    .write_msr     = hvmemul_write_msr_discard,
-    .cache_op      = hvmemul_cache_op_discard,
-    .tlb_op        = hvmemul_tlb_op,
-    .cpuid         = x86emul_cpuid,
-    .get_fpu       = hvmemul_get_fpu,
-    .put_fpu       = hvmemul_put_fpu,
-    .vmfunc        = hvmemul_vmfunc,
-};
-
 /*
  * Note that passing VIO_no_completion into this function serves as kind
  * of (but not fully) an "auto select completion" indicator.  When there's
@@ -2887,6 +2862,34 @@ int hvm_emulate_one(
     return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops, completion);
 }
 
+#ifdef CONFIG_VM_EVENT
+static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
+    .read          = hvmemul_read,
+    .insn_fetch    = hvmemul_insn_fetch,
+    .write         = hvmemul_write_discard,
+    .cmpxchg       = hvmemul_cmpxchg_discard,
+    .rep_ins       = hvmemul_rep_ins_discard,
+    .rep_outs      = hvmemul_rep_outs_discard,
+    .rep_movs      = hvmemul_rep_movs_discard,
+    .rep_stos      = hvmemul_rep_stos_discard,
+    .read_segment  = hvmemul_read_segment,
+    .write_segment = hvmemul_write_segment,
+    .read_io       = hvmemul_read_io_discard,
+    .write_io      = hvmemul_write_io_discard,
+    .read_cr       = hvmemul_read_cr,
+    .write_cr      = hvmemul_write_cr,
+    .read_xcr      = hvmemul_read_xcr,
+    .write_xcr     = hvmemul_write_xcr,
+    .read_msr      = hvmemul_read_msr,
+    .write_msr     = hvmemul_write_msr_discard,
+    .cache_op      = hvmemul_cache_op_discard,
+    .tlb_op        = hvmemul_tlb_op,
+    .cpuid         = x86emul_cpuid,
+    .get_fpu       = hvmemul_get_fpu,
+    .put_fpu       = hvmemul_put_fpu,
+    .vmfunc        = hvmemul_vmfunc,
+};
+
 void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
     unsigned int errcode)
 {
@@ -2949,6 +2952,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
 
     hvm_emulate_writeback(&ctx);
 }
+#endif /* CONFIG_VM_EVENT */
 
 void hvm_emulate_init_once(
     struct hvm_emulate_ctxt *hvmemul_ctxt,
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c3417b0593..196de2db67 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5297,6 +5297,7 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
     return rc;
 }
 
+#ifdef CONFIG_VM_EVENT
 void hvm_toggle_singlestep(struct vcpu *v)
 {
     ASSERT(atomic_read(&v->pause_count));
@@ -5323,6 +5324,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx)
     v->arch.hvm.fast_single_step.p2midx = p2midx;
 }
 #endif
+#endif /* CONFIG_VM_EVENT */
 
 /*
  * Segment caches in VMCB/VMCS are inconsistent about which bits are checked,
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 53f681bbb2..0070fe42a6 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -170,13 +170,10 @@ config HAS_VMAP
 config LIBFDT
 	bool
 
-config MEM_ACCESS_ALWAYS_ON
-	bool
-
 config VM_EVENT
-	def_bool MEM_ACCESS_ALWAYS_ON
-	prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
+	bool "Memory Access and VM events"
 	depends on HVM
+	default X86
 	help
 
 	  Framework to configure memory access types for guests and receive
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index 27d0c74216..1b76ce632e 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -51,7 +51,14 @@ struct vm_event_domain
 };
 
 /* Returns whether a ring has been set up */
+#ifdef CONFIG_VM_EVENT
 bool vm_event_check_ring(struct vm_event_domain *ved);
+#else
+static inline bool vm_event_check_ring(struct vm_event_domain *ved)
+{
+    return false;
+}
+#endif /* CONFIG_VM_EVENT */
 
 /* Returns 0 on success, -ENOSYS if there is no ring, -EBUSY if there is no
  * available space and the caller is a foreign domain. If the guest itself
-- 
2.34.1

Re: [PATCH v1 1/7] xen/svm: limit the scope of "rc"

[Jan Beulich]

On 13.11.2025 04:16, Penny Zheng wrote:
> To make codes less fragile, we limit the scope of "rc" through adding several
> instances in relatively narrow scopes. We also fixes wrong indentation.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> @@ -2748,16 +2749,18 @@ void asmlinkage svm_vmexit_handler(void)
>          }
>          else
>          {
> -           rc = hvm_monitor_debug(regs->rip,
> -                                  HVM_MONITOR_SOFTWARE_BREAKPOINT,
> -                                  X86_ET_SW_EXC,
> -                                  insn_len, 0);
> -           if ( rc < 0 )
> -               goto unexpected_exit_type;
> -           if ( !rc )
> -               hvm_inject_exception(X86_EXC_BP,
> -                                    X86_ET_SW_EXC,
> -                                    insn_len, X86_EVENT_NO_EC);
> +            int rc;
> +
> +            rc = hvm_monitor_debug(regs->rip,
> +                                   HVM_MONITOR_SOFTWARE_BREAKPOINT,
> +                                   X86_ET_SW_EXC,
> +                                   insn_len, 0);

As you touch the code anyway, make this the initializer of rc? And at the same
time join the latter two lines? (I may take the liberty of making these
adjustments while committing.)

Jan

Re: [PATCH v1 2/7] xen/vm_event: introduce vm_event_is_enabled()

[Jan Beulich]

On 13.11.2025 04:16, Penny Zheng wrote:
> Function vm_event_is_enabled() is introduced to check if vm event is enabled,
> and also make the checking conditional upon CONFIG_VM_EVENT, which could help
> DCE a lot unreachable calls/codes, such as hvm_monitor_io(), etc, when having
> VM_EVENT=n.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v2 -> v3:
> - move assignment (to ev) past the check.
> - remove redundant check
> - make "may_defer" & with vm_event_is_enabled() and have assertion back
> - add vm_event_is_enabled() for hvm_monitor_xxx() in svm.c/vmx.c/mem_sharing.c, then later there is no need to add stubs for them
> ---
> v3 -> v4:
> - move "may_defer" & with vm_event_is_enabled() to the very top of the function
> - use ?: to avoid introducing a new local variable
> - fix the wrong order
> - use pointer-to-const when possible
> - use IS_ENABLED(xxx) instead of #ifdef-block

This is irritating, as the subject (bogusly) says v1.

> ---
>  xen/arch/x86/hvm/emulate.c          |  9 +++--
>  xen/arch/x86/hvm/hvm.c              | 24 ++++++++---
>  xen/arch/x86/hvm/svm/intr.c         |  2 +-
>  xen/arch/x86/hvm/svm/svm.c          | 54 +++++++++++++++----------
>  xen/arch/x86/hvm/vmx/intr.c         |  2 +-
>  xen/arch/x86/hvm/vmx/vmx.c          | 63 +++++++++++++++++------------
>  xen/arch/x86/include/asm/vm_event.h |  8 ++++
>  xen/arch/x86/mm/mem_sharing.c       |  3 ++
>  8 files changed, 106 insertions(+), 59 deletions(-)

As before - why's there no "x86" in the subject prefix?

> @@ -3860,9 +3870,11 @@ int hvm_descriptor_access_intercept(uint64_t exit_info,
>      struct vcpu *curr = current;
>      struct domain *currd = curr->domain;
>  
> -    if ( currd->arch.monitor.descriptor_access_enabled )
> +    if ( vm_event_is_enabled(curr) &&
> +         currd->arch.monitor.descriptor_access_enabled )
>      {
>          ASSERT(curr->arch.vm_event);
> +
>          hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
>                                        descriptor, is_write);

Stray (and not really necessary) addition of a blank line? Did you perhaps
rather mean to remove the now pointless ASSERT()?

> @@ -2907,16 +2914,19 @@ void asmlinkage svm_vmexit_handler(void)
>  
>      case VMEXIT_IOIO:
>      {
> -        int rc;
> +        if ( vm_event_is_enabled(v) )
> +        {
> +            int rc;

With this the outer figure braces introduced by patch 1 can also go away again.

> @@ -4703,16 +4711,20 @@ void asmlinkage vmx_vmexit_handler(struct cpu_user_regs *regs)
>              };
>          } io_qual;
>          unsigned int bytes;
> -        int rc;
>  
>          __vmread(EXIT_QUALIFICATION, &io_qual.raw);
>          bytes = io_qual.size + 1;
>  
> -        rc = hvm_monitor_io(io_qual.port, bytes, io_qual.in, io_qual.str);
> -        if ( rc < 0 )
> -            goto exit_and_crash;
> -        if ( rc )
> -            break;
> +        if ( vm_event_is_enabled(v) )
> +        {
> +            int rc;
> +
> +            rc = hvm_monitor_io(io_qual.port, bytes, io_qual.in, io_qual.str);

Make this the initializer of the variable?

> --- a/xen/arch/x86/include/asm/vm_event.h
> +++ b/xen/arch/x86/include/asm/vm_event.h
> @@ -45,4 +45,12 @@ void vm_event_sync_event(struct vcpu *v, bool value);
>  
>  void vm_event_reset_vmtrace(struct vcpu *v);
>  
> +static inline bool vm_event_is_enabled(const struct vcpu *v)
> +{
> +    if ( IS_ENABLED(CONFIG_VM_EVENT) )
> +        return v->arch.vm_event != NULL;
> +
> +    return false;
> +}

Simply

    return IS_ENABLED(CONFIG_VM_EVENT) && v->arch.vm_event != NULL;

?

I guess I might as well do the adjustments while committing, even if it's quite
a few of them. In any event, with the adjustments
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v1 3/7] xen/monitor: wrap monitor_op under CONFIG_VM_EVENT

[Jan Beulich]

On 13.11.2025 04:16, Penny Zheng wrote:
> Feature monitor_op is based on vm event subsystem, so monitor.o shall be
> wrapped under CONFIG_VM_EVENT.
> The following functions are only invoked by monitor-op, so they all shall be
> wrapped with CONFIG_VM_EVENT (otherwise they will become unreachable and
> violate Misra rule 2.1 when VM_EVENT=n):
> - hvm_enable_msr_interception
>   - hvm_function_table.enable_msr_interception
> - hvm_has_set_descriptor_access_existing
>   - hvm_function_table.set_descriptor_access_existi
> - arch_monitor_get_capabilities
> Function monitored_msr() still needs a stub to pass compilation when
> VM_EVENT=n.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v3 -> v4:
> - a new commit split from previous "xen/vm_event: consolidate CONFIG_VM_EVENT"
> - Another blank line ahead of the #ifdef
> - Move hvm_enable_msr_interception() up into the earlier #ifdef
> - only arch_monitor_get_capabilities() needs wrapping, as this static inline
> function calls hvm_has_set_descriptor_access_exiting(), which is declared only
> when VM_EVENT=y
> ---
>  xen/arch/x86/hvm/Makefile          |  2 +-
>  xen/arch/x86/hvm/svm/svm.c         |  8 +++++++-
>  xen/arch/x86/hvm/vmx/vmx.c         | 10 ++++++++++
>  xen/arch/x86/include/asm/hvm/hvm.h | 18 +++++++++++-------
>  xen/arch/x86/include/asm/monitor.h |  9 +++++++++
>  5 files changed, 38 insertions(+), 9 deletions(-)

Same remark as for patch 2 regarding the subject prefix. Then
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v1 4/7] xen/p2m: move xenmem_access_to_p2m_access() to common p2m.c

[Jan Beulich]

On 13.11.2025 04:16, Penny Zheng wrote:
> Memory access and ALTP2M are two seperate features, while both depending on
> helper xenmem_access_to_p2m_access(). So it betters lives in common p2m.c,
> other than mem_access.c which will be compiled out when VM_EVENT=n && ALTP2M=y.

Yet when both are off, the function becomes unreachable? I.e. doesn't there need
to be #ifdef around it, to please Misra?

> Coding style has been corrected at the same time.

That is, blank lines were inserted and nothing else, afaics. Could have been said
explicitly.

> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v3 -> v4:
> - new commit
> ---
>  xen/arch/x86/mm/mem_access.c | 36 ----------------------------------
>  xen/arch/x86/mm/p2m.c        | 38 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 36 deletions(-)

Imo with this movement, the declaration wants to move out of mem_access.h as well.

Jan

Re: [PATCH v1 5/7] xen/x86: move declaration from mem_access.h to altp2m.h

[Jan Beulich]

On 13.11.2025 04:16, Penny Zheng wrote:
> Memory access and ALTP2M are two seperate features, and each could be
> controlled via VM_EVENT or ALTP2M. In order to avoid implicit declaration
> when ALTP2M=y and VM_EVENT=n on compiling hvm.o/altp2m.o, we move declaration
> of the following functions from <asm/mem_access.h> to <asm/altp2m.h>:
> - p2m_set_suppress_ve
> - p2m_set_suppress_ve_multi
> - p2m_get_suppress_ve
> Potential error on altp2m.c also breaks Misra Rule 8.4.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

This looks to be independent of all earlier changes, and hence could go in ahead
of any of them?

Jan

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

[Jan Beulich]

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

Re: [PATCH v1 7/7] xen/vm_event: consolidate CONFIG_VM_EVENT

[Jan Beulich]

On 13.11.2025 04:16, Penny Zheng wrote:
> File hvm/vm_event.c and x86/vm_event.c are the extend to vm_event handling
> routines, and its compilation shall be guarded by CONFIG_VM_EVENT too.
> 
> Although CONFIG_VM_EVENT is right now forcibly enabled on x86 via
> MEM_ACCESS_ALWAYS_ON, we could disable it through disabling
> CONFIG_MGMT_HYPERCALLS later. So we remove MEM_ACCESS_ALWAYS_ON and
> make VM_EVENT=y on default only on x86 to retain the same.
> 
> The following functions are developed on the basis of vm event framework, or
> only invoked by vm_event.c, so they all shall be wrapped with CONFIG_VM_EVENT
> (otherwise they will become unreachable and
> violate Misra rule 2.1 when VM_EVENT=n):
> - hvm_toggle_singlestep
> - hvm_fast_singlestep
> - hvm_emulate_one_vm_event
>     - hvmemul_write{,cmpxchg,rep_ins,rep_outs,rep_movs,rep_stos,read_io,write_io}_discard
> And Function vm_event_check_ring() needs stub to pass compilation when
> VM_EVENT=n.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - split out XSM changes

Isn't that split out patch also needed as a prereq to the one here? The one
here on its own:
Acked-by: Jan Beulich <jbeulich@suse.com>

But the possibly missing dependency question needs addressing before this may
go in.

> - remove unnecessary stubs
> - move "struct p2m_domain" declaration ahead of the #ifdef
> ---
> v2 -> v3:
> - move .enable_msr_interception and .set_descriptor_access_exiting together
> - with the introduction of "vm_event_is_enabled()", all hvm_monitor_xxx()
> stubs are no longer needed
> - change to use in-place stubs in do_altp2m_op()
> - no need to add stub for monitor_traps(), __vm_event_claim_slot(),
> vm_event_put_request() and vm_event_vcpu_pause()
> - remove MEM_ACCESS_ALWAYS_ON
> - return default p2m_access_rwx for xenmem_access_to_p2m_access() when
> VM_EVENT=n
> - add wrapping for hvm_emulate_one_vm_event/
> hvmemul_write{,cmpxchg,rep_ins,rep_outs,rep_movs,rep_stos,read_io,write_io}_discard
> ---

No changes at all in this version? The v3 patch was much larger, after all.

Jan

RE: [PATCH v1 1/7] xen/svm: limit the scope of "rc"

[Penny, Zheng]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, November 13, 2025 4:56 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Andryuk, Jason <Jason.Andryuk@amd.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v1 1/7] xen/svm: limit the scope of "rc"
>
> On 13.11.2025 04:16, Penny Zheng wrote:
> > To make codes less fragile, we limit the scope of "rc" through adding
> > several instances in relatively narrow scopes. We also fixes wrong indentation.
> >
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>

Thx

> > @@ -2748,16 +2749,18 @@ void asmlinkage svm_vmexit_handler(void)
> >          }
> >          else
> >          {
> > -           rc = hvm_monitor_debug(regs->rip,
> > -                                  HVM_MONITOR_SOFTWARE_BREAKPOINT,
> > -                                  X86_ET_SW_EXC,
> > -                                  insn_len, 0);
> > -           if ( rc < 0 )
> > -               goto unexpected_exit_type;
> > -           if ( !rc )
> > -               hvm_inject_exception(X86_EXC_BP,
> > -                                    X86_ET_SW_EXC,
> > -                                    insn_len, X86_EVENT_NO_EC);
> > +            int rc;
> > +
> > +            rc = hvm_monitor_debug(regs->rip,
> > +                                   HVM_MONITOR_SOFTWARE_BREAKPOINT,
> > +                                   X86_ET_SW_EXC,
> > +                                   insn_len, 0);
>
> As you touch the code anyway, make this the initializer of rc? And at the same time
> join the latter two lines? (I may take the liberty of making these adjustments while
> committing.)
>

Thx, plz

> Jan

RE: [PATCH v1 2/7] xen/vm_event: introduce vm_event_is_enabled()

[Penny, Zheng]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, November 13, 2025 5:14 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Andryuk, Jason <Jason.Andryuk@amd.com>; Tamas K
> Lengyel <tamas@tklengyel.com>; Alexandru Isaila <aisaila@bitdefender.com>;
> Petre Pircalabu <ppircalabu@bitdefender.com>; Grygorii Strashko
> <grygorii_strashko@epam.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1 2/7] xen/vm_event: introduce vm_event_is_enabled()
>
> On 13.11.2025 04:16, Penny Zheng wrote:
> > Function vm_event_is_enabled() is introduced to check if vm event is
> > enabled, and also make the checking conditional upon CONFIG_VM_EVENT,
> > which could help DCE a lot unreachable calls/codes, such as
> > hvm_monitor_io(), etc, when having VM_EVENT=n.
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> > ---
> > v2 -> v3:
> > - move assignment (to ev) past the check.
> > - remove redundant check
> > - make "may_defer" & with vm_event_is_enabled() and have assertion
> > back
> > - add vm_event_is_enabled() for hvm_monitor_xxx() in
> > svm.c/vmx.c/mem_sharing.c, then later there is no need to add stubs
> > for them
> > ---
> > v3 -> v4:
> > - move "may_defer" & with vm_event_is_enabled() to the very top of the
> > function
> > - use ?: to avoid introducing a new local variable
> > - fix the wrong order
> > - use pointer-to-const when possible
> > - use IS_ENABLED(xxx) instead of #ifdef-block
>
> This is irritating, as the subject (bogusly) says v1.
>

I'm not sure whether I need to bring previous diff or not.

> > ---
> > +static inline bool vm_event_is_enabled(const struct vcpu *v) {
> > +    if ( IS_ENABLED(CONFIG_VM_EVENT) )
> > +        return v->arch.vm_event != NULL;
> > +
> > +    return false;
> > +}
>
> Simply
>
>     return IS_ENABLED(CONFIG_VM_EVENT) && v->arch.vm_event != NULL;
>
> ?
>
> I guess I might as well do the adjustments while committing, even if it's quite a few
> of them. In any event, with the adjustments
> Acked-by: Jan Beulich <jbeulich@suse.com>

Mant thanks

> Jan

RE: [PATCH v1 5/7] xen/x86: move declaration from mem_access.h to altp2m.h

[Penny, Zheng]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, November 13, 2025 5:35 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: 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 v1 5/7] xen/x86: move declaration from mem_access.h to
> altp2m.h
>
> On 13.11.2025 04:16, Penny Zheng wrote:
> > Memory access and ALTP2M are two seperate features, and each could be
> > controlled via VM_EVENT or ALTP2M. In order to avoid implicit
> > declaration when ALTP2M=y and VM_EVENT=n on compiling hvm.o/altp2m.o,
> > we move declaration of the following functions from <asm/mem_access.h> to
> <asm/altp2m.h>:
> > - p2m_set_suppress_ve
> > - p2m_set_suppress_ve_multi
> > - p2m_get_suppress_ve
> > Potential error on altp2m.c also breaks Misra Rule 8.4.
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>

Thx


> This looks to be independent of all earlier changes, and hence could go in ahead of
> any of them?

Yes, it could.

>
> Jan

Re: [PATCH v1 1/7] xen/svm: limit the scope of "rc"

[Jason Andryuk]

On 2025-11-18 02:11, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> On 13.11.2025 04:16, Penny Zheng wrote:
>>> To make codes less fragile, we limit the scope of "rc" through adding
>>> several instances in relatively narrow scopes. We also fixes wrong indentation.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>

>> As you touch the code anyway, make this the initializer of rc? And at the same time
>> join the latter two lines? (I may take the liberty of making these adjustments while
>> committing.)
>>
> 
> Thx, plz

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH v1 2/7] xen/vm_event: introduce vm_event_is_enabled()

[Jason Andryuk]

On 2025-11-18 05:11, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, November 13, 2025 5:14 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné
>> <roger.pau@citrix.com>; Andryuk, Jason <Jason.Andryuk@amd.com>; Tamas K
>> Lengyel <tamas@tklengyel.com>; Alexandru Isaila <aisaila@bitdefender.com>;
>> Petre Pircalabu <ppircalabu@bitdefender.com>; Grygorii Strashko
>> <grygorii_strashko@epam.com>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v1 2/7] xen/vm_event: introduce vm_event_is_enabled()
>>
>> On 13.11.2025 04:16, Penny Zheng wrote:
>>> Function vm_event_is_enabled() is introduced to check if vm event is
>>> enabled, and also make the checking conditional upon CONFIG_VM_EVENT,
>>> which could help DCE a lot unreachable calls/codes, such as
>>> hvm_monitor_io(), etc, when having VM_EVENT=n.
>>>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>> I guess I might as well do the adjustments while committing, even if it's quite a few
>> of them. In any event, with the adjustments
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Mant thanks

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Regards,
Jason

Re: [PATCH v1 3/7] xen/monitor: wrap monitor_op under CONFIG_VM_EVENT

[Jason Andryuk]

On 2025-11-13 04:18, Jan Beulich wrote:
> On 13.11.2025 04:16, Penny Zheng wrote:
>> Feature monitor_op is based on vm event subsystem, so monitor.o shall be
>> wrapped under CONFIG_VM_EVENT.
>> The following functions are only invoked by monitor-op, so they all shall be
>> wrapped with CONFIG_VM_EVENT (otherwise they will become unreachable and
>> violate Misra rule 2.1 when VM_EVENT=n):
>> - hvm_enable_msr_interception
>>    - hvm_function_table.enable_msr_interception
>> - hvm_has_set_descriptor_access_existing
>>    - hvm_function_table.set_descriptor_access_existi
>> - arch_monitor_get_capabilities
>> Function monitored_msr() still needs a stub to pass compilation when
>> VM_EVENT=n.
>>
>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>> ---
>> v3 -> v4:
>> - a new commit split from previous "xen/vm_event: consolidate CONFIG_VM_EVENT"
>> - Another blank line ahead of the #ifdef
>> - Move hvm_enable_msr_interception() up into the earlier #ifdef
>> - only arch_monitor_get_capabilities() needs wrapping, as this static inline
>> function calls hvm_has_set_descriptor_access_exiting(), which is declared only
>> when VM_EVENT=y
>> ---
>>   xen/arch/x86/hvm/Makefile          |  2 +-
>>   xen/arch/x86/hvm/svm/svm.c         |  8 +++++++-
>>   xen/arch/x86/hvm/vmx/vmx.c         | 10 ++++++++++
>>   xen/arch/x86/include/asm/hvm/hvm.h | 18 +++++++++++-------
>>   xen/arch/x86/include/asm/monitor.h |  9 +++++++++
>>   5 files changed, 38 insertions(+), 9 deletions(-)
> 
> Same remark as for patch 2 regarding the subject prefix. Then
> Acked-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH v1 5/7] xen/x86: move declaration from mem_access.h to altp2m.h

[Jason Andryuk]

On 2025-11-18 06:09, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, November 13, 2025 5:35 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: 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 v1 5/7] xen/x86: move declaration from mem_access.h to
>> altp2m.h
>>
>> On 13.11.2025 04:16, Penny Zheng wrote:
>>> Memory access and ALTP2M are two seperate features, and each could be
>>> controlled via VM_EVENT or ALTP2M. In order to avoid implicit
>>> declaration when ALTP2M=y and VM_EVENT=n on compiling hvm.o/altp2m.o,
>>> we move declaration of the following functions from <asm/mem_access.h> to
>> <asm/altp2m.h>:
>>> - p2m_set_suppress_ve
>>> - p2m_set_suppress_ve_multi
>>> - p2m_get_suppress_ve
>>> Potential error on altp2m.c also breaks Misra Rule 8.4.
>>>
>>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
> 
> Thx

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH v1 7/7] xen/vm_event: consolidate CONFIG_VM_EVENT

[Jason Andryuk]

On 2025-11-13 05:16, Jan Beulich wrote:
> On 13.11.2025 04:16, Penny Zheng wrote:
>> File hvm/vm_event.c and x86/vm_event.c are the extend to vm_event handling
>> routines, and its compilation shall be guarded by CONFIG_VM_EVENT too.
>>
>> Although CONFIG_VM_EVENT is right now forcibly enabled on x86 via
>> MEM_ACCESS_ALWAYS_ON, we could disable it through disabling
>> CONFIG_MGMT_HYPERCALLS later. So we remove MEM_ACCESS_ALWAYS_ON and
>> make VM_EVENT=y on default only on x86 to retain the same.
>>
>> The following functions are developed on the basis of vm event framework, or
>> only invoked by vm_event.c, so they all shall be wrapped with CONFIG_VM_EVENT
>> (otherwise they will become unreachable and
>> violate Misra rule 2.1 when VM_EVENT=n):
>> - hvm_toggle_singlestep
>> - hvm_fast_singlestep
>> - hvm_emulate_one_vm_event
>>      - hvmemul_write{,cmpxchg,rep_ins,rep_outs,rep_movs,rep_stos,read_io,write_io}_discard
>> And Function vm_event_check_ring() needs stub to pass compilation when
>> VM_EVENT=n.
>>
>> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>> ---
>> v1 -> v2:
>> - split out XSM changes
> 
> Isn't that split out patch also needed as a prereq to the one here? The one
> here on its own:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>