Re: [XEN][PATCH] xen: make VMTRACE support optional

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 03.11.25 11:47, Teddy Astie wrote:
> Le 31/10/2025 à 22:24, Grygorii Strashko a écrit :
> > From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> >
> > The VMTRACE feature depends on Platform/Arch HW and code support and now
> > can be used only on x86 HVM with Intel VT-x (INTEL_VMX) enabled.
> > This makes VMTRACE support optional by introducing two Kconfig options:
> > - CONFIG_HAS_VMTRACE which allows Platform/Arch to advertise VMTRACE
> > support readiness
> > - CONFIG_VMTRACE to enable/disable the feature.
> I like the idea of making it optional since it's only used in specific
> contexts.
>
> > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > ---
> > Based on top of patch "domain: adjust soft-reset arch dependency" [1]
> >
> > [1] 
> > https://patchwork.kernel.org/project/xen-devel/patch/c9c8c9c6-a155-4986-bf77-5766cdcd6024@xxxxxxxx/
> >
> >    xen/Kconfig.debug                       | 15 +++++++++++++++
> >    xen/arch/x86/domctl.c                   |  4 ++++
> >    xen/arch/x86/hvm/Kconfig                |  1 +
> >    xen/arch/x86/hvm/vmx/vmcs.c             |  2 ++
> >    xen/arch/x86/hvm/vmx/vmx.c              | 10 ++++++++++
> >    xen/arch/x86/include/asm/guest-msr.h    |  2 ++
> >    xen/arch/x86/include/asm/hvm/hvm.h      |  9 +++++++++
> >    xen/arch/x86/include/asm/hvm/vmx/vmcs.h |  2 ++
> >    xen/arch/x86/mm/mem_sharing.c           |  2 ++
> >    xen/arch/x86/vm_event.c                 |  6 ++++--
> >    xen/common/domain.c                     | 10 ++++++++++
> >    xen/common/memory.c                     |  6 ++++++
> >    xen/common/vm_event.c                   |  3 ++-
> >    xen/include/xen/domain.h                |  4 ++++
> >    xen/include/xen/sched.h                 |  4 ++++
> >    15 files changed, 77 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> > index d900d926c555..70ec4f0d14a5 100644
> > --- a/xen/Kconfig.debug
> > +++ b/xen/Kconfig.debug
> > @@ -155,4 +155,19 @@ config DEBUG_INFO
> >           "make install-xen" for installing xen.efi, stripping needs to be
> >           done outside the Xen build environment).
> >    
> > +config HAS_VMTRACE
> > +    bool
> > +
> > +config VMTRACE
> > +    bool "HW VM tracing support"
> > +    depends on HAS_VMTRACE
> > +    default y
> > +    help
> > +      Enables HW VM tracing support which allows to configure HW processor
> > +      features (vmtrace_op) to enable capturing information about software
> > +      execution using dedicated hardware facilities with minimal interference
> > +      to the software being traced. The trace date can be retrieved using 
> > buffer
> > +      shared between Xen and domain
> > +      (see XENMEM_acquire_resource(XENMEM_resource_vmtrace_buf)).
> > +
> >    endmenu
[...]

> >    
> > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> > index fc349270b9c5..f4c8696ce54e 100644
> > --- a/xen/arch/x86/vm_event.c
> > +++ b/xen/arch/x86/vm_event.c
> > @@ -253,7 +253,9 @@ void vm_event_fill_regs(vm_event_request_t *req)
> >        req->data.regs.x86.shadow_gs = ctxt.shadow_gs;
> >        req->data.regs.x86.dr6 = ctxt.dr6;
> >    
> > +#ifdef CONFIG_VMTRACE
> >        if ( hvm_vmtrace_output_position(curr, &req->data.regs.x86.vmtrace_pos) 
> > != 1 )
> > +#endif
> >            req->data.regs.x86.vmtrace_pos = ~0;
> This if-def looks very oddly placed.
I assume you are asking for hvm_vmtrace_output_position() stub, Right?

> >    #endif
> >    }
> > @@ -303,12 +305,12 @@ void vm_event_emulate_check(struct vcpu *v, 
> > vm_event_response_t *rsp)
> >    #endif
> >    }
> >    
> > +#ifdef CONFIG_VMTRACE
> >    void vm_event_reset_vmtrace(struct vcpu *v)
> >    {
> > -#ifdef CONFIG_HVM
> >        hvm_vmtrace_reset(v);
> > -#endif
> >    }
> > +#endif
> >    
> >    /*
> >     * Local variables:
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 7dcd466e5a12..2be6ee03d004 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -136,7 +136,9 @@ struct vcpu *idle_vcpu[NR_CPUS] __read_mostly;
> >    
> >    vcpu_info_t dummy_vcpu_info;
> >    
> > +#ifdef CONFIG_VMTRACE
> >    bool __read_mostly vmtrace_available;
> > +#endif
> >    
> >    bool __read_mostly vpmu_is_available;
> >    
> > @@ -318,6 +320,7 @@ static void vcpu_info_reset(struct vcpu *v)
> >    
> >    static void vmtrace_free_buffer(struct vcpu *v)
> >    {
> > +#ifdef CONFIG_VMTRACE
> >        const struct domain *d = v->domain;
> >        struct page_info *pg = v->vmtrace.pg;
> >        unsigned int i;
> > @@ -332,10 +335,12 @@ static void vmtrace_free_buffer(struct vcpu *v)
> >            put_page_alloc_ref(&pg[i]);
> >            put_page_and_type(&pg[i]);
> >        }
> > +#endif
> >    }
> >    
> >    static int vmtrace_alloc_buffer(struct vcpu *v)
> >    {
> > +#ifdef CONFIG_VMTRACE
> >        struct domain *d = v->domain;
> >        struct page_info *pg;
> >        unsigned int i;
> > @@ -377,6 +382,9 @@ static int vmtrace_alloc_buffer(struct vcpu *v)
> >        }
> >    
> >        return -ENODATA;
> > +#else
> > +    return 0;
> > +#endif
> >    }
> >    
> >    /*
> > @@ -825,7 +833,9 @@ struct domain *domain_create(domid_t domid,
> >            ASSERT(!config->altp2m.nr);
> >    #endif
> >    
> > +#ifdef CONFIG_VMTRACE
> >            d->vmtrace_size = config->vmtrace_size;
> > +#endif
> >        }
> >    
> >        /* Sort out our idea of is_control_domain(). */
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index 3688e6dd5032..66dc7f7a0a41 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1155,8 +1155,10 @@ static unsigned int resource_max_frames(const struct 
> > domain *d,
> >        case XENMEM_resource_ioreq_server:
> >            return ioreq_server_max_frames(d);
> >    
> > +#ifdef CONFIG_VMTRACE
> >        case XENMEM_resource_vmtrace_buf:
> >            return d->vmtrace_size >> PAGE_SHIFT;
> > +#endif
> >    
> >        default:
> >            return 0;
> > @@ -1198,6 +1200,7 @@ static int acquire_ioreq_server(struct domain *d,
> >    #endif
> >    }
> >    
> > +#ifdef CONFIG_VMTRACE
> >    static int acquire_vmtrace_buf(
> >        struct domain *d, unsigned int id, unsigned int frame,
> >        unsigned int nr_frames, xen_pfn_t mfn_list[])
> > @@ -1220,6 +1223,7 @@ static int acquire_vmtrace_buf(
> >    
> >        return nr_frames;
> >    }
> > +#endif
> >    
> Given that vmtrace feature is fairly isolated in VMX code, wouldn't it
> be better to move all vmtrace related functions (including vmx_init_ipt)
> in a separate vmtrace.c file and make such file gated behind
> CONFIG_VMTRACE ?
Are you thinking about:
 xen/common/
 |- vmtrace.c
 xen/arch/x86/hvm/vmx/
 |- vmtrace.c
?

or smth else?

[...]

--
Best regards,
-grygorii