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.zh...@amd.com>
> ---
> 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
