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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxx>
> ---
> 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@xxxxxxxx>

Jan