Re: [PATCH v3 08/28] xen/vm_event: introduce vm_event_is_enabled()

[Jan Beulich <jbeulich@suse.com>]

On 13.10.2025 12:15, Penny Zheng wrote:
> @@ -2340,6 +2341,7 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
>           (value & (X86_CR0_PE | X86_CR0_PG)) == X86_CR0_PG )
>          return X86EMUL_EXCEPTION;
>  
> +    may_defer &= vm_event_is_enabled(v);

I think this wants to move to the very top of the function, so that it's clear
that all uses (current and future) in the function are covered. Same for the
sibling functions, of course.

> @@ -3544,6 +3548,7 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len)
>      struct vcpu *curr = current;
>      unsigned int leaf = regs->eax, subleaf = regs->ecx;
>      struct cpuid_leaf res;
> +    int ret = 0;
>  
>      if ( curr->arch.msrs->misc_features_enables.cpuid_faulting &&
>           hvm_get_cpl(curr) > 0 )
> @@ -3560,7 +3565,10 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len)
>      regs->rcx = res.c;
>      regs->rdx = res.d;
>  
> -    return hvm_monitor_cpuid(inst_len, leaf, subleaf);
> +    if ( vm_event_is_enabled(curr) )
> +        ret = hvm_monitor_cpuid(inst_len, leaf, subleaf);
> +
> +    return ret;

Just to mention, could also do without a new local variable:

    return vm_event_is_enabled(curr)
           ? hvm_monitor_cpuid(inst_len, leaf, subleaf)
           : 0;

> @@ -3860,9 +3869,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 ( currd->arch.monitor.descriptor_access_enabled &&
> +         vm_event_is_enabled(curr) )

While functionally it won't matter, logically this looks the wrong way round.
In principle I'd expect the "monitor" field to also disappear when VM_EVENT=n,
at which point the order will need to be the other way around anyway (as the
explicit CONFIG_VM_EVENT then needs to come first).

>      {
>          ASSERT(curr->arch.vm_event);
> +
>          hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
>                                        descriptor, is_write);
>      }

Stray change (especially in an already big patch)?

> @@ -2551,7 +2552,7 @@ void asmlinkage svm_vmexit_handler(void)
>      uint64_t exit_reason;
>      struct vcpu *v = current;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> -    int insn_len, rc;
> +    int insn_len, rc = 0;

This is almost 200 lines apart from ...

> @@ -2720,11 +2721,14 @@ void asmlinkage svm_vmexit_handler(void)
>                      break;
>              }
>  
> -            rc = hvm_monitor_debug(regs->rip,
> -                                   HVM_MONITOR_DEBUG_EXCEPTION,
> -                                   trap_type, insn_len, 0);
> -            if ( rc < 0 )
> -                goto unexpected_exit_type;
> +            if ( vm_event_is_enabled(v) )
> +            {
> +                rc = hvm_monitor_debug(regs->rip,
> +                                       HVM_MONITOR_DEBUG_EXCEPTION,
> +                                       trap_type, insn_len, 0);
> +                if ( rc < 0 )
> +                    goto unexpected_exit_type;
> +            }
>              if ( !rc )
>                  hvm_inject_exception(X86_EXC_DB,
>                                       trap_type, insn_len, X86_EVENT_NO_EC);

... where it first comes into play. I wonder whether the initialization
wouldn't better move closer, also for the code to be less fragile going
forward (where a new earlier use of rc might easily appear). Or, like VMX
code has it, limit the scope of "rc" (requiring several instances then in
relatively narrow scopes).

> --- a/xen/arch/x86/include/asm/vm_event.h
> +++ b/xen/arch/x86/include/asm/vm_event.h
> @@ -45,4 +45,13 @@ 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(struct vcpu *v)

Pointer-to-const please for (almost?) all predicate-like functions.

> +{
> +#ifdef CONFIG_VM_EVENT
> +    return v->arch.vm_event != NULL;
> +#else
> +    return false;
> +#endif
> +}

As long a vm_event is always a member of struct arch_vcpu, IS_ENABLED() should
be sufficient here? Or did you merely forget to add an #ifdef around the field
as well? Albeit I realize that would break with e.g. the use in
hvmemul_rep_outs_set_context(). Otoh that function (and likely others there)
should be compiled out when VM_EVENT=n. Maybe that's the subject of the next
patch ...

Jan