Re: [PATCH v1 04/25] xen: consolidate CONFIG_VM_EVENT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.08.2025 11:47, 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.
> Futhermore, features about monitor_op and memory access are both based on
> vm event subsystem, so monitor.o/mem_access.o shall be wrapped under
> CONFIG_VM_EVENT.
> 
> Although CONFIG_VM_EVENT is forcibly enabled on x86, we could disable it
> through disabling CONFIG_DOMCTL in the future.
> In consequence, a few functions, like the ones defined in hvm/monitor.h,
> needs stub to pass compilation when CONFIG_VM_EVENT=n.
> Remove the CONFIG_VM_EVENT wrapper for "#include <asm/mem_access.h>", as
> we need declaration there to pass compilation when CONFIG_VM_EVENT=n
> 
> The following functions are developed on the basis of vm event framework, or
> only invoked by vm_event.c/monitor.c/mem_access.c, so they all shall be
> wrapped with CONFIG_VM_EVENT:
> - hvm_toggle_singlestep
> - hvm_fast_singlestep
> - p2m_mem_paging_drop_page
> - p2m_mem_paging_populate_page
> - p2m_mem_paging_resume
> - hvm_enable_msr_interception
>   - hvm_function_table.enable_msr_interception
> - hvm_has_set_descriptor_access_existing
>   - hvm_function_table.set_descriptor_access_existing
> - xsm_vm_event_control
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
>  xen/arch/ppc/stubs.c                    |  2 +
>  xen/arch/x86/Makefile                   |  2 +-
>  xen/arch/x86/hvm/Makefile               |  4 +-
>  xen/arch/x86/hvm/hvm.c                  |  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      | 10 ++++
>  xen/arch/x86/include/asm/hvm/monitor.h  | 65 ++++++++++++++++++++++++-
>  xen/arch/x86/include/asm/hvm/vm_event.h |  4 ++
>  xen/arch/x86/include/asm/mem_access.h   |  9 ++++
>  xen/arch/x86/include/asm/monitor.h      | 15 ++++++
>  xen/arch/x86/include/asm/p2m.h          |  6 +++
>  xen/arch/x86/mm/mem_paging.c            |  2 +
>  xen/include/xen/mem_access.h            | 36 ++++++++++++--
>  xen/include/xen/monitor.h               |  8 ++-
>  xen/include/xen/vm_event.h              | 24 ++++++++-
>  xen/include/xsm/xsm.h                   |  4 +-
>  xen/xsm/dummy.c                         |  2 +-
>  xen/xsm/flask/hooks.c                   |  4 +-
>  19 files changed, 200 insertions(+), 17 deletions(-)

Overall it looks like the patch could be split some. E.g. the XSM changes look
as if they could go separately.

> --- a/xen/arch/ppc/stubs.c
> +++ b/xen/arch/ppc/stubs.c
> @@ -60,6 +60,7 @@ void vcpu_show_execution_state(struct vcpu *v)
>      BUG_ON("unimplemented");
>  }
>  
> +#ifdef CONFIG_VM_EVENT
>  /* vm_event.c */
>  
>  void vm_event_fill_regs(vm_event_request_t *req)
> @@ -76,6 +77,7 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
>  {
>      /* Not supported on PPC. */
>  }
> +#endif /* CONFIG_VM_EVENT */

Is this really needed? I wouldn't bother editing stubs.c files unless really
necessary.

> --- 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,13 @@ static inline uint32_t 
> arch_monitor_get_capabilities(struct domain *d)
>  
>      return capabilities;
>  }
> +#else
> +static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> +{
> +    ASSERT_UNREACHABLE();
> +    return 0;
> +}

Instead of this, a mere declaration (with no definition) would make a possible
problem known at build time, rather than only at runtime.

> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -775,10 +775,16 @@ static inline int relinquish_p2m_mapping(struct domain 
> *d)
>  /* Modify p2m table for shared gfn */
>  int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
>  
> +#ifdef CONFIG_VM_EVENT
>  /* Tell xenpaging to drop a paged out frame */
>  void p2m_mem_paging_drop_page(struct domain *d, gfn_t gfn, p2m_type_t p2mt);
>  /* Start populating a paged out frame */
>  void p2m_mem_paging_populate(struct domain *d, gfn_t gfn);
> +#else
> +static inline void p2m_mem_paging_drop_page(struct domain *d, gfn_t gfn,
> +                                            p2m_type_t p2mt) {}

This, I think, isn't needed. p2m_is_paging() is already short-circuited into
0 / false when MEM_PAGING=n (implying VM_EVENT=n in your supposed future
configuration, so the call site is being DCE-ed, and hence the declaration
that was already there will suffice.

> --- a/xen/arch/x86/mm/mem_paging.c
> +++ b/xen/arch/x86/mm/mem_paging.c
> @@ -15,6 +15,7 @@
>  
>  #include "mm-locks.h"
>  
> +#ifdef CONFIG_VM_EVENT
>  /*
>   * p2m_mem_paging_drop_page - Tell pager to drop its reference to a paged 
> page
>   * @d: guest domain
> @@ -186,6 +187,7 @@ void p2m_mem_paging_resume(struct domain *d, 
> vm_event_response_t *rsp)
>          gfn_unlock(p2m, gfn, 0);
>      }
>  }
> +#endif /* CONFIG_VM_EVENT */

As per the previous remark: Why would this be needed? We already have

obj-$(CONFIG_MEM_PAGING) += mem_paging.o

in the corresponding Makefile.

> --- a/xen/include/xen/mem_access.h
> +++ b/xen/include/xen/mem_access.h
> @@ -33,9 +33,7 @@
>   */
>  struct vm_event_st;
>  
> -#ifdef CONFIG_VM_EVENT
>  #include <asm/mem_access.h>
> -#endif

Why?

> @@ -73,6 +71,7 @@ typedef enum {
>      /* NOTE: Assumed to be only 4 bits right now on x86. */
>  } p2m_access_t;
>  
> +#ifdef CONFIG_VM_EVENT
>  struct p2m_domain;
>  bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
>                                   xenmem_access_t xaccess,
> @@ -99,10 +98,41 @@ long p2m_set_mem_access_multi(struct domain *d,
>  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
>                         unsigned int altp2m_idx);
>  
> -#ifdef CONFIG_VM_EVENT
>  int mem_access_memop(unsigned long cmd,
>                       XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
>  #else
> +struct p2m_domain;

No point in duplicating this; just move it ahead of the #ifdef.

Jan