24.04_linux-nvidia-6.17-next: MPAM: Please pull arm_mpam: Consider overflow in bandwidth counter state

[fyu1]

Backport this commit to fix a bug: https://nvbugspro.nvidia.com/bug/6207279

Since the commit is in 6.19 upstream already, 7.0 bos and lts don't have this issue.

Use the overflow status bit to track overflow on each bandwidth counter read and add the counter size to the correction when overflow is detected.

This assumes that only a single overflow has occurred since the last read of the counter. Overflow interrupts, on hardware that supports them could be used to remove this limitation.

Cc: Zeng Heng zengheng4@huawei.com
Reviewed-by: Gavin Shan gshan@redhat.com
Reviewed-by: Zeng Heng zengheng4@huawei.com
Reviewed-by: Jonathan Cameron jonathan.cameron@huawei.com
Reviewed-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
Reviewed-by: Fenghua Yu fenghuay@nvidia.com
Tested-by: Carl Worth carl@os.amperecomputing.com
Tested-by: Gavin Shan gshan@redhat.com
Tested-by: Zeng Heng zengheng4@huawei.com
Tested-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
Tested-by: Hanjun Guo guohanjun@huawei.com

(backported from commit b353637)

[fenghuay: Fix mem bw monitoring counter overflow issue.

• Resolve conflict in mpam_msmon_overflow_val();
• Resolve conflict in __ris_msmon_read(); ]

[fyu1]

@fyu1 Codex is calling out these findings...

Line numbers below are from remotes/fyu1/24.04_linux-nvidia-6.17-next.mpam.extras.fixes3 at 65dbf0f.

1. Overflow increment is off by one

At drivers/resctrl/mpam_devices.c:1383-1392:

case mpam_feat_msmon_mbwu_63counter: return GENMASK_ULL(62, 0); case mpam_feat_msmon_mbwu_44counter: return GENMASK_ULL(43, 0); case mpam_feat_msmon_mbwu_31counter: return GENMASK_ULL(30, 0);

Those are maximum counter values: 2^63 - 1, 2^44 - 1, 2^31 - 1.

But after this patch, the value is added directly on overflow at :1488-1489:

if (overflow) mbwu_state->correction += mpam_msmon_overflow_val(m->type);

The upstream commit adds the counter modulus instead: for the 31-bit counter, upstream computes BIT_ULL(31), i.e. 2^31, not GENMASK_ULL(30, 0). So every detected overflow undercounts by one counter unit. For 44/63-bit downstream counters, the same logic should be BIT_ULL(44) and BIT_ULL(63).

Fixed.

2. T241 scaling was lost for overflow correction

Existing downstream code scales the sampled MBWU value at :1480-1481:

if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc)) now *= 64;

Before this backport, overflow correction also applied the same scale:

overflow_val = mpam_msmon_overflow_val(m->type); if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc)) overflow_val *= 64; overflow_val -= mbwu_state->prev_val;

The patch removed that path and now adds the unscaled helper result directly. That mixes units: now is bytes on T241, while correction is raw counter ticks. A 31-bit overflow should add roughly 2^31 * 64 bytes, but the current code adds only about 2^31.

The fix should make the overflow correction use the same unit as now.

3. Long MBWU overflow status is ignored

This branch has long MBWU counter support and defines two relevant status bits:

MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L /* bit 15 / MSMON_CFG_x_CTL_OFLOW_STATUS / bit 26 */

clean_msmon_ctl_val() already knows about the long-counter bit and clears it for config comparison at :1337-1343:

*cur_ctl &= ~MSMON_CFG_x_CTL_OFLOW_STATUS;

if (FIELD_GET(MSMON_CFG_x_CTL_TYPE, *cur_ctl) == MSMON_CFG_MBWU_CTL_TYPE_MBWU) *cur_ctl &= ~MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L;

But the new overflow detection only checks bit 26 at :1435:

overflow = cur_ctl & MSMON_CFG_x_CTL_OFLOW_STATUS;

So a long-counter overflow signaled only by MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L will not increment correction, and because overflow is false, the code also will not write the cleaned control value back to clear the sticky status bit.

The backport needs to treat OFLOW_STATUS_L as overflow for the 44/63-bit MBWU paths.

4. Dead/stale state remains after removing prev-value overflow detection

At :1405:

u64 now, overflow_val = 0;

overflow_val is no longer used after the patch. That should produce an unused-variable warning.
Fixed.

Also, struct msmon_mbwu_state still says prev_val is “Used to detect overflow” at drivers/resctrl/mpam_internal.h:325-326, and write_msmon_ctl_flt_vals() still resets it at mpam_devices.c:1374-1375. But this patch removed the only read of prev_val. That is no longer functionally harmful by itself, but it is stale backport debris and makes the state model misleading.
Won't fixed since it's harmless.

[fyu1]

@fyu1 Codex is calling out these findings...
Line numbers below are from remotes/fyu1/24.04_linux-nvidia-6.17-next.mpam.extras.fixes3 at 65dbf0f.

1. Overflow increment is off by one

At drivers/resctrl/mpam_devices.c:1383-1392:
case mpam_feat_msmon_mbwu_63counter: return GENMASK_ULL(62, 0); case mpam_feat_msmon_mbwu_44counter: return GENMASK_ULL(43, 0); case mpam_feat_msmon_mbwu_31counter: return GENMASK_ULL(30, 0);
Those are maximum counter values: 2^63 - 1, 2^44 - 1, 2^31 - 1.
But after this patch, the value is added directly on overflow at :1488-1489:
if (overflow) mbwu_state->correction += mpam_msmon_overflow_val(m->type);
The upstream commit adds the counter modulus instead: for the 31-bit counter, upstream computes BIT_ULL(31), i.e. 2^31, not GENMASK_ULL(30, 0). So every detected overflow undercounts by one counter unit. For 44/63-bit downstream counters, the same logic should be BIT_ULL(44) and BIT_ULL(63).

2. T241 scaling was lost for overflow correction

Existing downstream code scales the sampled MBWU value at :1480-1481:
if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc)) now *= 64;
Before this backport, overflow correction also applied the same scale:
overflow_val = mpam_msmon_overflow_val(m->type); if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc)) overflow_val *= 64; overflow_val -= mbwu_state->prev_val;
The patch removed that path and now adds the unscaled helper result directly. That mixes units: now is bytes on T241, while correction is raw counter ticks. A 31-bit overflow should add roughly 2^31 * 64 bytes, but the current code adds only about 2^31.
The fix should make the overflow correction use the same unit as now.

3. Long MBWU overflow status is ignored

This branch has long MBWU counter support and defines two relevant status bits:
MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L /* bit 15 / MSMON_CFG_x_CTL_OFLOW_STATUS / bit 26 */
clean_msmon_ctl_val() already knows about the long-counter bit and clears it for config comparison at :1337-1343:
*cur_ctl &= ~MSMON_CFG_x_CTL_OFLOW_STATUS;
if (FIELD_GET(MSMON_CFG_x_CTL_TYPE, *cur_ctl) == MSMON_CFG_MBWU_CTL_TYPE_MBWU) *cur_ctl &= ~MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L;
But the new overflow detection only checks bit 26 at :1435:
overflow = cur_ctl & MSMON_CFG_x_CTL_OFLOW_STATUS;
So a long-counter overflow signaled only by MSMON_CFG_MBWU_CTL_OFLOW_STATUS_L will not increment correction, and because overflow is false, the code also will not write the cleaned control value back to clear the sticky status bit.
The backport needs to treat OFLOW_STATUS_L as overflow for the 44/63-bit MBWU paths.

4. Dead/stale state remains after removing prev-value overflow detection

At :1405:
u64 now, overflow_val = 0;
overflow_val is no longer used after the patch. That should produce an unused-variable warning.
Also, struct msmon_mbwu_state still says prev_val is “Used to detect overflow” at drivers/resctrl/mpam_internal.h:325-326, and write_msmon_ctl_flt_vals() still resets it at mpam_devices.c:1374-1375. But this patch removed the only read of prev_val. That is no longer functionally harmful by itself, but it is stale backport debris and makes the state model misleading.

Claude missed 1, but also flagged 2-4.

Additionally, the commit message ought to have your SOB go after the backport notes:

...
[fenghuay: Fix mem bw monitoring counter overflow issue.
 - Resolve conflict in mpam_msmon_overflow_val();
 - Resolve conflict in __ris_msmon_read();
]
Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>

Fixed.

[fyu1]

Rebased. No code change.

[clsotog]

This is a finding at codex:
drivers/resctrl/mpam_devices.c:1415 declares overflow without initializing it. Initialize overflow = false.

[nvmochs]

@fyu1

In addition to the initialization issue that Carol pointed out, my review also popped up these findings:

• Medium: T241 63-bit overflow correction is still inconsistent if the 63-bit path is used. now is scaled for all MBWU widths at mpam_devices.c:1502-1503, but mpam_msmon_overflow_val() skips * 64 for mpam_feat_msmon_mbwu_63counter at :1403-1405. That mixes byte-scaled samples with raw-count overflow correction for 63-bit counters. Either both now and overflow correction should scale for 63-bit, or neither should.

More detail...

› In the final branch, for T241 MBWU reads:

    if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc))
          now *= 64;

That scales now for all MBWU counter widths, including mpam_feat_msmon_mbwu_63counter.

But overflow correction does:

    if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc) &&
        type != mpam_feat_msmon_mbwu_63counter)
          overflow_val *= 64;

So for 63-bit counters specifically:

now = scaled by 64
overflow correction = not scaled by 64

---

• Medium/Low: the final branch still prefers 44-bit counters over 63-bit counters at mpam_devices.c:1574-1577. Upstream 9e5afb7 prefers mpam_feat_msmon_mbwu_63counter first. Since probe sets 44-bit when long counters exist and then also sets 63-bit when LWD exists, this branch will normally never choose 63-bit via the generic MBWU path. If intentional, it should be called out in the backport notes; otherwise swap the order.

---

Lastly, it appears Jamie's comment about the ordering of the annotation notes in relation to your SOB have not been addressed - I'm still seeing the notes come after the SOB, when they should be after the pick tag, but before the SOB.

Example of the ordering I was anticipating:

   (backported from commit 9e5afb7c32830bcd123976a7729ef4e2dff0cd77)
    [fenghuay: Partial port; NV tree already has long-counter read paths,
     mpam_msc_read_mbwu_l(), and T241-MPAM-6 workaround (d7c811dd0171).
     This commit ports per-width overflow correction, OFLOW_STATUS_L overflow
     detection, and T241 overflow correction scaling via mpam_msmon_overflow_val().
     Differences from upstream 9e5afb7/dc48eb1:
     - 44/63-bit overflow: check OFLOW_STATUS_L | OFLOW_STATUS; upstream
       9e5afb7 uses OFLOW_STATUS_L only.
     - T241 now *= 64 applies to all counter widths; upstream dc48eb1 skips 63-bit.
    ]
    Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>

[fyu1]

To clarify, this is a patch from v6.19 (not v6.18) and was part of the "[PATCH v6 00/34] arm_mpam: Add basic mpam driver".

@fyu1 Are there other patches from this series that are missing from the 6.17-HWE kernel?

One more patch is added to help the main patch. Other patches are cosmetic patches and no need to be ported.

[fyu1]

This is a finding at codex: drivers/resctrl/mpam_devices.c:1415 declares overflow without initializing it. Initialize overflow = false.

Fixed.

[fyu1]

@fyu1

In addition to the initialization issue that Carol pointed out, my review also popped up these findings:

• Medium: T241 63-bit overflow correction is still inconsistent if the 63-bit path is used. now is scaled for all MBWU widths at mpam_devices.c:1502-1503, but mpam_msmon_overflow_val() skips * 64 for mpam_feat_msmon_mbwu_63counter at :1403-1405. That mixes byte-scaled samples with raw-count overflow correction for 63-bit counters. Either both now and overflow correction should scale for 63-bit, or neither should.

More detail...

› In the final branch, for T241 MBWU reads:

    if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc))
          now *= 64;

That scales now for all MBWU counter widths, including mpam_feat_msmon_mbwu_63counter.

But overflow correction does:

    if (mpam_has_quirk(T241_MBW_COUNTER_SCALE_64, msc) &&
        type != mpam_feat_msmon_mbwu_63counter)
          overflow_val *= 64;

So for 63-bit counters specifically:

now = scaled by 64 overflow correction = not scaled by 64

Fixed.

• Medium/Low: the final branch still prefers 44-bit counters over 63-bit counters at mpam_devices.c:1574-1577. Upstream 9e5afb7 prefers mpam_feat_msmon_mbwu_63counter first. Since probe sets 44-bit when long counters exist and then also sets 63-bit when LWD exists, this branch will normally never choose 63-bit via the generic MBWU path. If intentional, it should be called out in the backport notes; otherwise swap the order.

Fixed.

Lastly, it appears Jamie's comment about the ordering of the annotation notes in relation to your SOB have not been addressed - I'm still seeing the notes come after the SOB, when they should be after the pick tag, but before the SOB.

Example of the ordering I was anticipating:

   (backported from commit 9e5afb7c32830bcd123976a7729ef4e2dff0cd77)
    [fenghuay: Partial port; NV tree already has long-counter read paths,
     mpam_msc_read_mbwu_l(), and T241-MPAM-6 workaround (d7c811dd0171).
     This commit ports per-width overflow correction, OFLOW_STATUS_L overflow
     detection, and T241 overflow correction scaling via mpam_msmon_overflow_val().
     Differences from upstream 9e5afb7/dc48eb1:
     - 44/63-bit overflow: check OFLOW_STATUS_L | OFLOW_STATUS; upstream
       9e5afb7 uses OFLOW_STATUS_L only.
     - T241 now *= 64 applies to all counter widths; upstream dc48eb1 skips 63-bit.
    ]
    Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>

Fixed.

[fyu1]

The updated PR addressed comments from Mat and Carol.

Please review and merge it if it's good.

Thanks.

-Fenghua

[clsotog]

@fyu1 Can you rebase the PR?

[nvmochs]

@fyu1 All of the coding issues have been addressed.

The remaining issue is commit-message ordering in the second commit 8e479a9.

It still has:

  (backported from commit 9e5afb7c32830bcd123976a7729ef4e2dff0cd77)
  Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>
  [fenghuay: Partial port; this tree already has long-counter read paths,
  ...
  ]

That should be:

  (backported from commit 9e5afb7c32830bcd123976a7729ef4e2dff0cd77)
  [fenghuay: ...]
  Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>

The first commit 3d2e100 has the ordering fixed correctly.

[fyu1]

@fyu1 Can you rebase the PR?

Rebased.

[fyu1]

@fyu1 All of the coding issues have been addressed.

The remaining issue is commit-message ordering in the second commit 8e479a9.

It still has:

  (backported from commit 9e5afb7c32830bcd123976a7729ef4e2dff0cd77)
  Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>
  [fenghuay: Partial port; this tree already has long-counter read paths,
  ...
  ]

That should be:

  (backported from commit 9e5afb7c32830bcd123976a7729ef4e2dff0cd77)
  [fenghuay: ...]
  Signed-off-by: Fenghua Yu <fenghuay@nvidia.com>

The first commit 3d2e100 has the ordering fixed correctly.

Fixed.

[nvmochs]

Thanks @fyu1! No further issues from me.

Acked-by: Matthew R. Ochs <mochs@nvidia.com>

[jamieNguyenNVIDIA]

Acked-by: Jamie Nguyen <jamien@nvidia.com>

[clsotog]

Acked-by: Carol L Soto <csoto@nvidia.com>