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 <[email protected]>
> ---
> 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 <[email protected]>
Jan
