Re: [PATCH v12 00/25] Allow AET to use PMT as loadable module

["Luck, Tony" <tony.luck@intel.com>]

On Wed, Sep 16, 2026 at 04:12:55PM -0700, Tony Luck wrote:
> Requiring INTEL_PMT_TELEMETRY=y to enable AET is a functional workaround
> to enable enumeration of Application Energy Telemetry (AET) events, but
> unacceptable to many users. It results in increased configuration complexity,
> increased kernel memory footprint and inability to patch problems by unloading
> a module and loading an updated version.
> 
> Add a registration function to the AET code that can be used by
> INTEL_PMT_TELEMETRY to provide the enumeration functions.
> 
> INTEL_PMT_TELEMETRY can be loaded/unloaded independently of
> resctrl file system mount/unmount. Perform enumeration on
> every mount and cleanup on every unmount.

Sashiko report here:
	https://sashiko.dev/#/patchset/20260916231320.14502-1-tony.luck%40intel.com

Only issues in parts 11, 20, 21

Patch 11: [PATCH v12 11/25] fs/resctrl: Add interface to disable a monitor event

    This isn't a bug, but the kerneldoc for resctrl_disable_mon_event() appears
    to contradict the core safety invariant described in the commit message.
    The commit message states the architecture is responsible for calling this
    interface "only while resctrl is unmounted", but this documentation says not
    to disable an event that may be accessed while "unmounted".
    Could this lead to confusion for callers reading the header file? Should this
    say "while the file system is mounted" instead?

The kerneldoc comment is the better description here (supplied by
Reinette in the review of the v11 version of this series).

  https://lore.kernel.org/all/f9f3cb40-bc98-449d-a801-6af836900e76@intel.com/

With the intent of reminding developers that resctrl code may not be
idle just because the file system is not mounted. The limbo timer code
will continue to run until LLC cache occupancy counters reduce to the
threshold value to stop tracking.

Commit message could be updated to match if we need a new series.


Patch 20: [PATCH v12 20/25] x86/resctrl: Enforce system RMID limit on AET

    Does this code successfully enforce the system RMID limit on systems with SNC
    enabled as stated in the commit message?
    When SNC is enabled, the true maximum usable RMID limit is scaled down and
    available via resctrl_arch_system_max_rmid_idx(). By capping AET's num_rmid
    against pqr_assoc_num_rmid (the unscaled physical limit), the resulting limit
    could remain incorrectly large, continuing to display an unachievable value to
    users in info/PERF_PKG_MON/num_rmids.

This code is doing what I intend. Making sure that the value reported in
info/PERF_PKG_MON/num_rmids shows how many RMIDs can be supported by AET.

Perhaps the commit message could better explain this intent.


Patch 21: [PATCH v12 21/25] x86/resctrl: Export interface to report telemetry unbind/remove

    Can this result in an invalid cast for non-PCI devices?
    The PMT subsystem allows non-PCI devices (such as ACPI platform devices from
    pwrm_telemetry.c) to register endpoints. Using to_pci_dev() blindly here
    without verifying dev_is_pci() generates a bogus pointer for non-PCI devices.
    ...
    When this bogus pointer is passed into intel_vsec_get_mapping() and
    eventually to pci_match_id(), will it cause out-of-bounds memory reads or
    KASAN panics when dereferencing pdev->vendor and pdev->device?

The AET endpoints are always PCIe (enumeration uses the VSEC feature).

    Does dropping ep_lock here create a race condition?
    While ep_lock is dropped, stale endpoints still remain in the global
    telem_array list. A concurrent resctrl mount could invoke
    intel_pmt_get_regions_by_feature(), acquire the lock, and cache pointers to
    the MMIO resources of the devices currently being removed.
    When pmt_telem_remove() resumes and re-acquires the lock, it unmaps those
    regions. Won't the concurrent reader be left with validly cached but unmapped
    memory pointers, leading to a kernel panic when dereferenced by AET?

This is an existing issue in the pmt_telemetry driver. Scenario is a
race between a resctrl mount and an unbind of a device. The unbind gets
to pmt_telem_remove() but loses the race to acquire ep_lock to the mount
code calling intel_pmt_get_regions_by_feature(). All devices report
valid MMIO addresses and ep_lock is released then pmt_telem_remove()
invalidates the MMIO mappings for the device being unbound/removed.

Perhaps the telemetry driver should prevent removal of devices for the
interval from intel_pmt_get_regions_by_feature() to intel_pmt_put_feature_group()?

Can it do that?

-Tony