Re: [RFC PATCH v2 5/8] x86: ibs: Enable IBS profiling for memory accesses

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 10 Sep 2025 20:16:50 +0530
Bharata B Rao <bharata@amd.com> wrote:

> Enable IBS memory access data collection for user memory
> accesses by programming the required MSRs. The profiling
> is turned ON only for user mode execution and turned OFF
> for kernel mode execution. Profiling is explicitly disabled
> for NMI handler too.
> 
> TODOs:
> 
> - IBS sampling rate is kept fixed for now.
> - Arch/vendor separation/isolation of the code needs relook.
> 
> Signed-off-by: Bharata B Rao <bharata@amd.com>
One "oops I misread it" wrt to review of previous patch.
+ a really trivial thing.

J
> ---
>  arch/x86/include/asm/entry-common.h |  3 +++
>  arch/x86/include/asm/hardirq.h      |  2 ++
>  arch/x86/include/asm/ibs.h          |  2 ++
>  arch/x86/mm/ibs.c                   | 32 +++++++++++++++++++++++++++++
>  4 files changed, 39 insertions(+)

> diff --git a/arch/x86/mm/ibs.c b/arch/x86/mm/ibs.c
> index 6669710dd35b..3128e8fa5f39 100644
> --- a/arch/x86/mm/ibs.c
> +++ b/arch/x86/mm/ibs.c
> @@ -16,6 +16,7 @@ static u64 ibs_config __read_mostly;
>  static u32 ibs_caps;
>  
>  #define IBS_NR_SAMPLES	150
> +#define IBS_SAMPLE_PERIOD      10000
>  
>  /*
>   * Basic access info captured for each memory access.
> @@ -98,6 +99,36 @@ static void ibs_irq_handler(struct irq_work *i)
>  	schedule_work_on(smp_processor_id(), &ibs_work);
>  }
>  
> +void hw_access_profiling_stop(void)
> +{
> +	u64 ops_ctl;
> +
> +	if (!arch_hw_access_profiling)
> +		return;
> +
> +	rdmsrl(MSR_AMD64_IBSOPCTL, ops_ctl);
> +	wrmsrl(MSR_AMD64_IBSOPCTL, ops_ctl & ~IBS_OP_ENABLE);
> +}
> +
> +void hw_access_profiling_start(void)
> +{
> +	u64 config = 0;
> +	unsigned int period = IBS_SAMPLE_PERIOD;
> +
> +	if (!arch_hw_access_profiling)
> +		return;
> +
> +	/* Disable IBS for kernel thread */
> +	if (!current->mm)
> +		goto out;
> +
> +	config = (period >> 4)  & IBS_OP_MAX_CNT;

Bonus space though before & that can go.

> +	config |= (period & IBS_OP_MAX_CNT_EXT_MASK);
> +	config |= ibs_config;

Ah. Ignore comment in previous patch on this not being global. Clearly it needs
to be. Oops I misread this earlier.

> +out:
> +	wrmsrl(MSR_AMD64_IBSOPCTL, config);
> +}
> +
>  /*
>   * IBS NMI handler: Process the memory access info reported by IBS.
>   *
> @@ -304,6 +335,7 @@ static int __init ibs_access_profiling_init(void)
>  			  x86_amd_ibs_access_profile_startup,
>  			  x86_amd_ibs_access_profile_teardown);
>  
> +	arch_hw_access_profiling = true;
>  	pr_info("IBS setup for memory access profiling\n");
>  	return 0;
>  }