Re: [PATCH 1/2] perf/x86/intel: Fix OMR snoop information parsing issues

[Ian Rogers]

On Wed, Mar 11, 2026 at 12:56 AM Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx> wrote:
>
> When omr_source is 0x2, the omr_snoop (bit[6]) and omr_promoted (bit[7])
> fields are combined to represent the snoop information. However, the
> omr_promoted field was not left-shifted by 1 bit, resulting in incorrect
> snoop information.
>
> Besides, the snoop information parsing is not accurate for some OMR
> sources, like the snoop information should be SNOOP_NONE for these memory
> access (omr_source >= 7) instead of SNOOP_HIT.
>
> Fix these issues.
>
> Reported-by: Ian Rogers <irogers@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/CAP-5=fW4zLWFw1v38zCzB9-cseNSTTCtup=p2SDxZq7dPayVww@xxxxxxxxxxxxxx/
> Fixes: d2bdcde9626c ("perf/x86/intel: Add support for PEBS memory auxiliary info field in DMR")
> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

> ---
>  arch/x86/events/intel/ds.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 5027afc97b65..7f0d515c07c5 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -345,12 +345,12 @@ static u64 parse_omr_data_source(u8 dse)
>         if (omr.omr_remote)
>                 val |= REM;
>
> -       val |= omr.omr_hitm ? P(SNOOP, HITM) : P(SNOOP, HIT);
> -
>         if (omr.omr_source == 0x2) {
> -               u8 snoop = omr.omr_snoop | omr.omr_promoted;
> +               u8 snoop = omr.omr_snoop | (omr.omr_promoted << 1);
>
> -               if (snoop == 0x0)
> +               if (omr.omr_hitm)
> +                       val |= P(SNOOP, HITM);
> +               else if (snoop == 0x0)
>                         val |= P(SNOOP, NA);
>                 else if (snoop == 0x1)
>                         val |= P(SNOOP, MISS);
> @@ -359,7 +359,10 @@ static u64 parse_omr_data_source(u8 dse)
>                 else if (snoop == 0x3)
>                         val |= P(SNOOP, NONE);
>         } else if (omr.omr_source > 0x2 && omr.omr_source < 0x7) {
> +               val |= omr.omr_hitm ? P(SNOOP, HITM) : P(SNOOP, HIT);
>                 val |= omr.omr_snoop ? P(SNOOPX, FWD) : 0;
> +       } else {
> +               val |= P(SNOOP, NONE);

nit: the else here is fired for omr.omr_source of 0 (invalid) and 1
(reserved) but the commit message says it handles omr_source >= 7. In
the case of 0 there's an overlap between  P(SNOOP, NA) (from
omr_data_source[0])and the P(SNOOP,NONE) here.
nit: why not make omr_data_source const?

>         }
>
>         return val;
>
> base-commit: 73cee0aad1ee2479fde2c9b753a1b66acb7d1b9a
> --
> 2.34.1
>