Re: [PATCH V2 2/7] perf/x86/intel/uncore: Guard against invalid box control address

[Mi, Dapeng]

Reviewed-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>


On 5/27/2026 11:11 PM, Zide Chen wrote:
> Theoretically, intel_uncore_find_discovery_unit() could return NULL,
> e.g., when a CPU die is offline during uncore enumeration and its PMU
> units are not added to the discovery RB-tree.
>
> Guard against a NULL return value and the resulting invalid box control
> address (0) before accessing hardware.
>
> Signed-off-by: Zide Chen <zide.chen@xxxxxxxxx>
> ---
> V2:
> - New patch.
> - Address pre-existing invalid box control address issue (Sashiko).
> ---
>  arch/x86/events/intel/uncore_discovery.c | 36 ++++++++++++++++++------
>  1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 60e1200c4691..af2217b44a81 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
> @@ -490,17 +490,28 @@ static u64 intel_generic_uncore_box_ctl(struct intel_uncore_box *box)
>  
>  void intel_generic_uncore_msr_init_box(struct intel_uncore_box *box)
>  {
> -	wrmsrq(intel_generic_uncore_box_ctl(box), GENERIC_PMON_BOX_CTL_INT);
> +	u64 box_ctl = intel_generic_uncore_box_ctl(box);
> +
> +	if (!box_ctl)
> +		return;
> +
> +	wrmsrq(box_ctl, GENERIC_PMON_BOX_CTL_INT);
>  }
>  
>  void intel_generic_uncore_msr_disable_box(struct intel_uncore_box *box)
>  {
> -	wrmsrq(intel_generic_uncore_box_ctl(box), GENERIC_PMON_BOX_CTL_FRZ);
> +	u64 box_ctl = intel_generic_uncore_box_ctl(box);
> +
> +	if (box_ctl)
> +		wrmsrq(box_ctl, GENERIC_PMON_BOX_CTL_FRZ);
>  }
>  
>  void intel_generic_uncore_msr_enable_box(struct intel_uncore_box *box)
>  {
> -	wrmsrq(intel_generic_uncore_box_ctl(box), 0);
> +	u64 box_ctl = intel_generic_uncore_box_ctl(box);
> +
> +	if (box_ctl)
> +		wrmsrq(box_ctl, 0);
>  }
>  
>  static void intel_generic_uncore_msr_enable_event(struct intel_uncore_box *box,
> @@ -549,6 +560,10 @@ bool intel_generic_uncore_assign_hw_event(struct perf_event *event,
>  
>  	if (box->pci_dev) {
>  		box_ctl = UNCORE_DISCOVERY_PCI_BOX_CTRL(box_ctl);
> +
> +		if (!box_ctl)
> +			return false;
> +
>  		hwc->config_base = box_ctl + uncore_pci_event_ctl(box, hwc->idx);
>  		hwc->event_base  = box_ctl + uncore_pci_perf_ctr(box, hwc->idx);
>  		return true;
> @@ -567,27 +582,30 @@ static inline int intel_pci_uncore_box_ctl(struct intel_uncore_box *box)
>  
>  void intel_generic_uncore_pci_init_box(struct intel_uncore_box *box)
>  {
> -	struct pci_dev *pdev = box->pci_dev;
>  	int box_ctl = intel_pci_uncore_box_ctl(box);
>  
> +	if (!box_ctl)
> +		return;
> +
>  	__set_bit(UNCORE_BOX_FLAG_CTL_OFFS8, &box->flags);
> -	pci_write_config_dword(pdev, box_ctl, GENERIC_PMON_BOX_CTL_INT);
> +	pci_write_config_dword(box->pci_dev, box_ctl, GENERIC_PMON_BOX_CTL_INT);
>  }
>  
>  void intel_generic_uncore_pci_disable_box(struct intel_uncore_box *box)
>  {
> -	struct pci_dev *pdev = box->pci_dev;
>  	int box_ctl = intel_pci_uncore_box_ctl(box);
>  
> -	pci_write_config_dword(pdev, box_ctl, GENERIC_PMON_BOX_CTL_FRZ);
> +	if (box_ctl)
> +		pci_write_config_dword(box->pci_dev, box_ctl,
> +				       GENERIC_PMON_BOX_CTL_FRZ);
>  }
>  
>  void intel_generic_uncore_pci_enable_box(struct intel_uncore_box *box)
>  {
> -	struct pci_dev *pdev = box->pci_dev;
>  	int box_ctl = intel_pci_uncore_box_ctl(box);
>  
> -	pci_write_config_dword(pdev, box_ctl, 0);
> +	if (box_ctl)
> +		pci_write_config_dword(box->pci_dev, box_ctl, 0);
>  }
>  
>  static void intel_generic_uncore_pci_enable_event(struct intel_uncore_box *box,