Re: [PATCH v2 24/26] xen/domctl: wrap arch-specific domctl-op with CONFIG_MGMT_HYPERCALLS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2025 09:38, Penny Zheng wrote:
> Function arch_do_domctl() is responsible for arch-specific domctl-op,
> and shall be wrapped with CONFIG_MGMT_HYPERCALLS
> Tracking its calling chain and the following functions shall be wrapped with
> CONFIG_MGMT_HYPERCALLS:
> For x86:
> - hvm_save_one
> - hvm_acpi_power_button
> - hvm_acpi_sleep_button
> - hvm_debug_op
> - mem_sharing_domctl
> - make P2M_AUDIT depend on CONFIG_MGMT_HYPERCALLS
> - make PG_log_dirty depend on CONFIG_MGMT_HYPERCALLS
> - make policy.o depend on CONFIG_MGMT_HYPERCALLS
> - do_vmtrace_op
>   - hvm_vmtrace_control
>     - hvm_funcs.vmtrace_control
>   - hvm_vmtrace_get_option
>     - hvm_funcs.vmtrace_get_option
>   - hvm_vmtrace_set_option
>     - hvm_funcs.vmtrace_set_option
> - paging_domctl_cont
> For ARM:
> - subarch_do_domctl
> 
> Also, remove all #ifdef CONFIG_MGMT_HYPERCALLS-s in arch-specific domctl.c, as
> we put the guardian in Makefile for the whole file.
> Wrap default-case and arch_get_domain_info() transiently with
> CONFIG_MGMT_HYPERCALLS, and it will be removed when introducing
> CONFIG_MGMT_HYPERCALLS on the common/domctl.c in the last.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v1 -> v2:
> - split out xsm parts
> - adapt to changes of "unify DOMCTL to MGMT_HYPERCALLS"
> - wrap default-case and arch_get_domain_info() transiently
> ---
>  xen/Kconfig.debug                  |  2 +-
>  xen/arch/arm/arm32/Makefile        |  2 +-
>  xen/arch/arm/arm64/Makefile        |  2 +-
>  xen/arch/arm/domctl.c              |  2 --

Isn't there a change missing to arm/Makefile? Or else, how can ...

> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -184,7 +184,6 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>      }
>  }
>  
> -#ifdef CONFIG_MGMT_HYPERCALLS
>  void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>  {
>      struct vcpu_guest_context *ctxt = c.nat;
> @@ -200,7 +199,6 @@ void arch_get_info_guest(struct vcpu *v, 
> vcpu_guest_context_u c)
>      if ( !test_bit(_VPF_down, &v->pause_flags) )
>          ctxt->flags |= VGCF_online;
>  }
> -#endif /* CONFIG_MGMT_HYPERCALLS */

... this be correct?

> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -121,6 +121,7 @@ size_t hvm_save_size(struct domain *d)
>      return sz;
>  }

Both this and ...

> +#ifdef CONFIG_MGMT_HYPERCALLS
>  /*
>   * Extract a single instance of a save record, by marshalling all records of
>   * that type and copying out the one we need.
> @@ -195,6 +196,7 @@ int hvm_save_one(struct domain *d, unsigned int typecode, 
> unsigned int instance,
>      xfree(ctxt.data);
>      return rv;
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */
>  
>  int hvm_save(struct domain *d, hvm_domain_context_t *h)
>  {

... this and hvm_load() (and some others) will end up unreachable when
MGMT_HYPERCALLS=n and MEM_SHARING=n.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2585,6 +2585,7 @@ static bool cf_check vmx_get_pending_event(
>      (RTIT_STATUS_FILTER_EN | RTIT_STATUS_CONTEXT_EN | RTIT_STATUS_TRIGGER_EN 
> | \
>       RTIT_STATUS_ERROR | RTIT_STATUS_STOPPED)
>  
> +#ifdef CONFIG_MGMT_HYPERCALLS
>  static int cf_check vmtrace_get_option(
>      struct vcpu *v, uint64_t key, uint64_t *output)
>  {

This #ifdef wants to move up a few lines, to also cover the two #define-s.

> @@ -2693,6 +2694,7 @@ static int cf_check vmtrace_control(struct vcpu *v, 
> bool enable, bool reset)
>  
>      return 0;
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */
>  
>  static int cf_check vmtrace_output_position(struct vcpu *v, uint64_t *pos)
>  {
> @@ -2883,10 +2885,14 @@ static struct hvm_function_table 
> __initdata_cf_clobber vmx_function_table = {
>      .altp2m_vcpu_emulate_ve = vmx_vcpu_emulate_ve,
>      .altp2m_vcpu_emulate_vmfunc = vmx_vcpu_emulate_vmfunc,
>  #endif
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      .vmtrace_control = vmtrace_control,
> +#endif
>      .vmtrace_output_position = vmtrace_output_position,

Why would this remain? Patch 05 makes VM_EVENT dependent upon MGMT_HYPERCALLS,
and outside of domctl.c the only other caller is in vm_event.c.

> @@ -747,8 +751,10 @@ bool altp2m_vcpu_emulate_ve(struct vcpu *v);
>  
>  static inline int hvm_vmtrace_control(struct vcpu *v, bool enable, bool 
> reset)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      if ( hvm_funcs.vmtrace_control )
>          return alternative_call(hvm_funcs.vmtrace_control, v, enable, reset);
> +#endif
>  
>      return -EOPNOTSUPP;
>  }
> @@ -765,8 +771,10 @@ static inline int hvm_vmtrace_output_position(struct 
> vcpu *v, uint64_t *pos)
>  static inline int hvm_vmtrace_set_option(
>      struct vcpu *v, uint64_t key, uint64_t value)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      if ( hvm_funcs.vmtrace_set_option )
>          return alternative_call(hvm_funcs.vmtrace_set_option, v, key, value);
> +#endif
>  
>      return -EOPNOTSUPP;
>  }
> @@ -774,8 +782,10 @@ static inline int hvm_vmtrace_set_option(
>  static inline int hvm_vmtrace_get_option(
>      struct vcpu *v, uint64_t key, uint64_t *value)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      if ( hvm_funcs.vmtrace_get_option )
>          return alternative_call(hvm_funcs.vmtrace_get_option, v, key, value);
> +#endif
>  
>      return -EOPNOTSUPP;
>  }

Why #ifdef inside the functions? The sole users each are in domctl.c.

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -114,7 +114,9 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>  
>      memcpy(info->handle, d->handle, sizeof(xen_domain_handle_t));
>  
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      arch_get_domain_info(d, info);
> +#endif
>  }

This shouldn't be necessary; instead imo patch 18 should be extended to cover
getdomainfo() altogether.

> --- a/xen/lib/x86/Makefile
> +++ b/xen/lib/x86/Makefile
> @@ -1,3 +1,3 @@
>  obj-y += cpuid.o
>  obj-y += msr.o
> -obj-y += policy.o
> +obj-$(CONFIG_MGMT_HYPERCALLS) += policy.o

Fair parts of cpuid.c also become unreachable. And all of msr.c afaics.

Jan