Date:   Mon, 19 Apr 2021 15:38:38 -0500
From:   Steve Wahl <steve.wahl@....com>
To:     kan.liang@...ux.intel.com
Cc:     peterz@...radead.org, mingo@...hat.com,
        linux-kernel@...r.kernel.org, ak@...ux.intel.com, acme@...nel.org,
        andreas@...ler-net.de, eranian@...gle.com, osk@...gle.com,
        tglx@...utronix.de, mark@...dzero.net, steve.wahl@....com
Subject: Re: [PATCH] perf/x86/intel/uncore: Remove uncore extra PCI dev
 HSWEP_PCI_PCU_3

Tested-by: Steve Wahl <steve.wahl@....com>

On Thu, Apr 15, 2021 at 02:22:43PM -0700, kan.liang@...ux.intel.com wrote:
> From: Kan Liang <kan.liang@...ux.intel.com>
> 
> There may be a kernel panic on the Haswell server and the Broadwell
> server, if the snbep_pci2phy_map_init() return error.
> 
> The uncore_extra_pci_dev[HSWEP_PCI_PCU_3] is used in the cpu_init() to
> detect the existence of the SBOX, which is a MSR type of PMON unit.
> The uncore_extra_pci_dev is allocated in the uncore_pci_init(). If the
> snbep_pci2phy_map_init() returns error, perf doesn't initialize the
> PCI type of the PMON units, so the uncore_extra_pci_dev will not be
> allocated. But perf may continue initializing the MSR type of PMON
> units. A null dereference kernel panic will be triggered.
> 
> The sockets in a Haswell server or a Broadwell server are identical.
> Only need to detect the existence of the SBOX once.
> Current perf probes all available PCU devices and stores them into the
> uncore_extra_pci_dev. It's unnecessary.
> Use the pci_get_device() to replace the uncore_extra_pci_dev. Only
> detect the existence of the SBOX on the first available PCU device once.
> 
> Factor out hswep_has_limit_sbox(), since the Haswell server and the
> Broadwell server uses the same way to detect the existence of the SBOX.
> 
> Add some macros to replace the magic number.
> 
> Fixes: 5306c31c5733 ("perf/x86/uncore/hsw-ep: Handle systems with only two SBOXes")
> Reported-by: Steve Wahl <steve.wahl@....com>
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> ---
>  arch/x86/events/intel/uncore_snbep.c | 61 +++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index b79951d..9b89376 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -1159,7 +1159,6 @@ enum {
>  	SNBEP_PCI_QPI_PORT0_FILTER,
>  	SNBEP_PCI_QPI_PORT1_FILTER,
>  	BDX_PCI_QPI_PORT2_FILTER,
> -	HSWEP_PCI_PCU_3,
>  };
>  
>  static int snbep_qpi_hw_config(struct intel_uncore_box *box, struct perf_event *event)
> @@ -2857,22 +2856,33 @@ static struct intel_uncore_type *hswep_msr_uncores[] = {
>  	NULL,
>  };
>  
> -void hswep_uncore_cpu_init(void)
> +#define HSWEP_PCU_DID			0x2fc0
> +#define HSWEP_PCU_CAPID4_OFFET		0x94
> +#define hswep_get_chop(_cap)		(((_cap) >> 6) & 0x3)
> +
> +static bool hswep_has_limit_sbox(unsigned int device)
>  {
> -	int pkg = boot_cpu_data.logical_proc_id;
> +	struct pci_dev *dev = pci_get_device(PCI_VENDOR_ID_INTEL, device, NULL);
> +	u32 capid4;
> +
> +	if (!dev)
> +		return false;
> +
> +	pci_read_config_dword(dev, HSWEP_PCU_CAPID4_OFFET, &capid4);
> +	if (!hswep_get_chop(capid4))
> +		return true;
>  
> +	return false;
> +}
> +
> +void hswep_uncore_cpu_init(void)
> +{
>  	if (hswep_uncore_cbox.num_boxes > boot_cpu_data.x86_max_cores)
>  		hswep_uncore_cbox.num_boxes = boot_cpu_data.x86_max_cores;
>  
>  	/* Detect 6-8 core systems with only two SBOXes */
> -	if (uncore_extra_pci_dev[pkg].dev[HSWEP_PCI_PCU_3]) {
> -		u32 capid4;
> -
> -		pci_read_config_dword(uncore_extra_pci_dev[pkg].dev[HSWEP_PCI_PCU_3],
> -				      0x94, &capid4);
> -		if (((capid4 >> 6) & 0x3) == 0)
> -			hswep_uncore_sbox.num_boxes = 2;
> -	}
> +	if (hswep_has_limit_sbox(HSWEP_PCU_DID))
> +		hswep_uncore_sbox.num_boxes = 2;
>  
>  	uncore_msr_uncores = hswep_msr_uncores;
>  }
> @@ -3135,11 +3145,6 @@ static const struct pci_device_id hswep_uncore_pci_ids[] = {
>  		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>  						   SNBEP_PCI_QPI_PORT1_FILTER),
>  	},
> -	{ /* PCU.3 (for Capability registers) */
> -		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2fc0),
> -		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
> -						   HSWEP_PCI_PCU_3),
> -	},
>  	{ /* end: all zeroes */ }
>  };
>  
> @@ -3231,27 +3236,18 @@ static struct event_constraint bdx_uncore_pcu_constraints[] = {
>  	EVENT_CONSTRAINT_END
>  };
>  
> +#define BDX_PCU_DID			0x6fc0
> +
>  void bdx_uncore_cpu_init(void)
>  {
> -	int pkg = topology_phys_to_logical_pkg(boot_cpu_data.phys_proc_id);
> -
>  	if (bdx_uncore_cbox.num_boxes > boot_cpu_data.x86_max_cores)
>  		bdx_uncore_cbox.num_boxes = boot_cpu_data.x86_max_cores;
>  	uncore_msr_uncores = bdx_msr_uncores;
>  
> -	/* BDX-DE doesn't have SBOX */
> -	if (boot_cpu_data.x86_model == 86) {
> -		uncore_msr_uncores[BDX_MSR_UNCORE_SBOX] = NULL;
>  	/* Detect systems with no SBOXes */
> -	} else if (uncore_extra_pci_dev[pkg].dev[HSWEP_PCI_PCU_3]) {
> -		struct pci_dev *pdev;
> -		u32 capid4;
> -
> -		pdev = uncore_extra_pci_dev[pkg].dev[HSWEP_PCI_PCU_3];
> -		pci_read_config_dword(pdev, 0x94, &capid4);
> -		if (((capid4 >> 6) & 0x3) == 0)
> -			bdx_msr_uncores[BDX_MSR_UNCORE_SBOX] = NULL;
> -	}
> +	if ((boot_cpu_data.x86_model == 86) || hswep_has_limit_sbox(BDX_PCU_DID))
> +		uncore_msr_uncores[BDX_MSR_UNCORE_SBOX] = NULL;
> +
>  	hswep_uncore_pcu.constraints = bdx_uncore_pcu_constraints;
>  }
>  
> @@ -3472,11 +3468,6 @@ static const struct pci_device_id bdx_uncore_pci_ids[] = {
>  		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>  						   BDX_PCI_QPI_PORT2_FILTER),
>  	},
> -	{ /* PCU.3 (for Capability registers) */
> -		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
> -		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
> -						   HSWEP_PCI_PCU_3),
> -	},
>  	{ /* end: all zeroes */ }
>  };
>  
> -- 
> 2.7.4
> 

-- 
Steve Wahl, Hewlett Packard Enterprise

Powered by blists - more mailing lists