[PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Kroening, Gary]

For systems with a single PCI segment, it is sufficient to look for the
bus number to change in order to determine that all of the CHa's have
been counted for a single socket.

However, for multi PCI segment systems, each socket is given a new
segment and the bus number does NOT change.  So looking only for the
bus number to change ends up counting all of the CHa's on all sockets
in the system.  This leads to writing CPU MSRs beyond a valid range and
causes an error in ivbep_uncore_msr_init_box().

The fix is to check for either the bus number or segment number to change.

Signed-off-by: Gary Kroening <gary.kroening@hpe.com>
Signed-off-by: Mike Travis <mike.travis@hpe.com>
Reviewed-by: Dimitri Sivanich <dimitri.sivanich@hpe.com>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

--- linux-4.4.orig/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c
+++ linux-4.4/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c
@@ -3525,15 +3525,19 @@ static struct intel_uncore_type *skx_msr
 static int skx_count_chabox(void)
 {
 	struct pci_dev *chabox_dev = NULL;
-	int bus, count = 0;
+	int bus, seg, count = 0;
 
 	while (1) {
 		chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d, chabox_dev);
 		if (!chabox_dev)
 			break;
-		if (count == 0)
+		if (count == 0) {
 			bus = chabox_dev->bus->number;
-		if (bus != chabox_dev->bus->number)
+			seg = pci_domain_nr(chabox_dev->bus);
+		}
+		/* check for change in both bus and domain/segment */
+		if (bus != chabox_dev->bus->number ||
+		    seg != pci_domain_nr(chabox_dev->bus))
 			break;
 		count++;
 	}

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Liang, Kan]

On 3/7/2018 3:33 PM, Kroening, Gary wrote:
> For systems with a single PCI segment, it is sufficient to look for the
> bus number to change in order to determine that all of the CHa's have
> been counted for a single socket.
> 
> However, for multi PCI segment systems, each socket is given a new
> segment and the bus number does NOT change.  So looking only for the
> bus number to change ends up counting all of the CHa's on all sockets
> in the system.  This leads to writing CPU MSRs beyond a valid range and
> causes an error in ivbep_uncore_msr_init_box().
> 
> The fix is to check for either the bus number or segment number to change.
> 

Hi Gary,

There is a recommended way in uncore document to query the number of 
CHAs on Skylake server.
I have a patch to implement the new way.

Could you please take a look at the patch and see if it can fix your issue?


Thanks,
Kan

------
 From 55f54b2fa3021c691c2fd4f5cfc8f441fd104e91 Mon Sep 17 00:00:00 2001
From: Kan Liang <kan.liang@linux.intel.com>
Date: Mon, 12 Mar 2018 13:03:40 -0700
Subject: [PATCH] perf/x86/intel/uncore: Querying number of CHAs from 
CAPID6 register

The number of CHAs is miscalculated on multi PCI domain systems on
Skylake server

(From Kroening, Gary:

For systems with a single PCI segment, it is sufficient to look for the
bus number to change in order to determine that all of the CHa's have
been counted for a single socket.
However, for multi PCI segment systems, each socket is given a new
segment and the bus number does NOT change.  So looking only for the
bus number to change ends up counting all of the CHa's on all sockets
in the system.  This leads to writing CPU MSRs beyond a valid range and
causes an error in ivbep_uncore_msr_init_box().)

To determine the number of CHAs, it should read bits 27:0 in the CAPID6
register located at Device 30, Function 3, Offset 0x9C. These 28 bits
form a bit vector of available LLC slices and the CHAs that manage those
slices.

Fixes: cd34cd97b7b4 ("perf/x86/intel/uncore: Add Skylake server uncore
support")
Reported-by: Kroening, Gary <gary.kroening@hpe.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
  arch/x86/events/intel/uncore_snbep.c | 24 ++++++++++--------------
  1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c 
b/arch/x86/events/intel/uncore_snbep.c
index d4672ed..a42715b 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3575,24 +3575,20 @@ static struct intel_uncore_type 
*skx_msr_uncores[] = {
  	NULL,
  };

+#define SKX_CAPID6		0x9c
+#define SKX_CHA_BIT_WIDTH	28
+
  static int skx_count_chabox(void)
  {
-	struct pci_dev *chabox_dev = NULL;
-	int bus, count = 0;
+	struct pci_dev *dev = NULL;
+	u32 val = 0;

-	while (1) {
-		chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d, chabox_dev);
-		if (!chabox_dev)
-			break;
-		if (count == 0)
-			bus = chabox_dev->bus->number;
-		if (bus != chabox_dev->bus->number)
-			break;
-		count++;
-	}
+	dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
+	if (!dev)
+		return 0;

-	pci_dev_put(chabox_dev);
-	return count;
+	pci_read_config_dword(dev, SKX_CAPID6, &val);
+	return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
  }

  void skx_uncore_cpu_init(void)
-- 
2.7.4

RE: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Kroening, Gary]

Thanks, Kan -- your patch looks good, and cleaner than the previous method!

Tonight, I've tested it on our in-house simulator for the following configurations. The simulator has been matching hardware well for this issue -- we'll do more testing on real hardware, but I'm confident you have it right.

Terminology:

"hubless" -- equivalent to "glueless" or "white box" where our BIOS sets things up with a single bus for all sockets.

"scalable" -- BIOS assigns a new segment/domain for each socket

Configurations tested so far:
- single-socket hubless/scalable
- two sockets, scalable
- four sockets, hubless
- eight sockets, scalable

In all cases, skx_count_chabox() is returning 28 for Skylake server.
Thanks for the quick response!
Gary

> -----Original Message-----
> From: Liang, Kan [mailto:kan.liang@linux.intel.com]
> Sent: Monday, March 12, 2018 8:43 PM
> To: Kroening, Gary; mingo@redhat.com; hpa@zytor.com; tglx@linutronix.de;
> peterz@infradead.org
> Cc: Travis, Mike; Banman, Andrew; Sivanich, Dimitri; Anderson, Russ;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-
> segment arches
> 
> 
> 
> On 3/7/2018 3:33 PM, Kroening, Gary wrote:
> > For systems with a single PCI segment, it is sufficient to look for the
> > bus number to change in order to determine that all of the CHa's have
> > been counted for a single socket.
> >
> > However, for multi PCI segment systems, each socket is given a new
> > segment and the bus number does NOT change.  So looking only for the
> > bus number to change ends up counting all of the CHa's on all sockets
> > in the system.  This leads to writing CPU MSRs beyond a valid range and
> > causes an error in ivbep_uncore_msr_init_box().
> >
> > The fix is to check for either the bus number or segment number to
> change.
> >
> 
> Hi Gary,
> 
> There is a recommended way in uncore document to query the number of
> CHAs on Skylake server.
> I have a patch to implement the new way.
> 
> Could you please take a look at the patch and see if it can fix your
> issue?
> 
> 
> Thanks,
> Kan
> 
> ------
>  From 55f54b2fa3021c691c2fd4f5cfc8f441fd104e91 Mon Sep 17 00:00:00 2001
> From: Kan Liang <kan.liang@linux.intel.com>
> Date: Mon, 12 Mar 2018 13:03:40 -0700
> Subject: [PATCH] perf/x86/intel/uncore: Querying number of CHAs from
> CAPID6 register
> 
> The number of CHAs is miscalculated on multi PCI domain systems on
> Skylake server
> 
> (From Kroening, Gary:
> 
> For systems with a single PCI segment, it is sufficient to look for the
> bus number to change in order to determine that all of the CHa's have
> been counted for a single socket.
> However, for multi PCI segment systems, each socket is given a new
> segment and the bus number does NOT change.  So looking only for the
> bus number to change ends up counting all of the CHa's on all sockets
> in the system.  This leads to writing CPU MSRs beyond a valid range and
> causes an error in ivbep_uncore_msr_init_box().)
> 
> To determine the number of CHAs, it should read bits 27:0 in the CAPID6
> register located at Device 30, Function 3, Offset 0x9C. These 28 bits
> form a bit vector of available LLC slices and the CHAs that manage those
> slices.
> 
> Fixes: cd34cd97b7b4 ("perf/x86/intel/uncore: Add Skylake server uncore
> support")
> Reported-by: Kroening, Gary <gary.kroening@hpe.com>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>   arch/x86/events/intel/uncore_snbep.c | 24 ++++++++++--------------
>   1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c
> b/arch/x86/events/intel/uncore_snbep.c
> index d4672ed..a42715b 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3575,24 +3575,20 @@ static struct intel_uncore_type
> *skx_msr_uncores[] = {
>   	NULL,
>   };
> 
> +#define SKX_CAPID6		0x9c
> +#define SKX_CHA_BIT_WIDTH	28
> +
>   static int skx_count_chabox(void)
>   {
> -	struct pci_dev *chabox_dev = NULL;
> -	int bus, count = 0;
> +	struct pci_dev *dev = NULL;
> +	u32 val = 0;
> 
> -	while (1) {
> -		chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d,
> chabox_dev);
> -		if (!chabox_dev)
> -			break;
> -		if (count == 0)
> -			bus = chabox_dev->bus->number;
> -		if (bus != chabox_dev->bus->number)
> -			break;
> -		count++;
> -	}
> +	dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
> +	if (!dev)
> +		return 0;
> 
> -	pci_dev_put(chabox_dev);
> -	return count;
> +	pci_read_config_dword(dev, SKX_CAPID6, &val);
> +	return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
>   }
> 
>   void skx_uncore_cpu_init(void)
> --
> 2.7.4

RE: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Kroening, Gary]

Kan -- sorry for my late-night typo. My description for "hubless" should have said "a single segment/domain" rather than single bus. 

> "hubless" -- equivalent to "glueless" or "white box" where our BIOS sets
> things up with a single bus for all sockets.

******** single segment/domain *********
Sorry for the spam!
Gary


> -----Original Message-----
> From: Kroening, Gary
> Sent: Tuesday, March 13, 2018 12:07 AM
> To: 'Liang, Kan'; mingo@redhat.com; hpa@zytor.com; tglx@linutronix.de;
> peterz@infradead.org
> Cc: Travis, Mike; Banman, Andrew; Sivanich, Dimitri; Anderson, Russ;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-
> segment arches
> 
> Thanks, Kan -- your patch looks good, and cleaner than the previous
> method!
> 
> Tonight, I've tested it on our in-house simulator for the following
> configurations. The simulator has been matching hardware well for this
> issue -- we'll do more testing on real hardware, but I'm confident you
> have it right.
> 
> Terminology:
> 
> "hubless" -- equivalent to "glueless" or "white box" where our BIOS sets
> things up with a single bus for all sockets.
> 
> "scalable" -- BIOS assigns a new segment/domain for each socket
> 
> Configurations tested so far:
> - single-socket hubless/scalable
> - two sockets, scalable
> - four sockets, hubless
> - eight sockets, scalable
> 
> In all cases, skx_count_chabox() is returning 28 for Skylake server.
> Thanks for the quick response!
> Gary
> 
> > -----Original Message-----
> > From: Liang, Kan [mailto:kan.liang@linux.intel.com]
> > Sent: Monday, March 12, 2018 8:43 PM
> > To: Kroening, Gary; mingo@redhat.com; hpa@zytor.com; tglx@linutronix.de;
> > peterz@infradead.org
> > Cc: Travis, Mike; Banman, Andrew; Sivanich, Dimitri; Anderson, Russ;
> > x86@kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-
> pci-
> > segment arches
> >
> >
> >
> > On 3/7/2018 3:33 PM, Kroening, Gary wrote:
> > > For systems with a single PCI segment, it is sufficient to look for
> the
> > > bus number to change in order to determine that all of the CHa's have
> > > been counted for a single socket.
> > >
> > > However, for multi PCI segment systems, each socket is given a new
> > > segment and the bus number does NOT change.  So looking only for the
> > > bus number to change ends up counting all of the CHa's on all sockets
> > > in the system.  This leads to writing CPU MSRs beyond a valid range
> and
> > > causes an error in ivbep_uncore_msr_init_box().
> > >
> > > The fix is to check for either the bus number or segment number to
> > change.
> > >
> >
> > Hi Gary,
> >
> > There is a recommended way in uncore document to query the number of
> > CHAs on Skylake server.
> > I have a patch to implement the new way.
> >
> > Could you please take a look at the patch and see if it can fix your
> > issue?
> >
> >
> > Thanks,
> > Kan
> >
> > ------
> >  From 55f54b2fa3021c691c2fd4f5cfc8f441fd104e91 Mon Sep 17 00:00:00 2001
> > From: Kan Liang <kan.liang@linux.intel.com>
> > Date: Mon, 12 Mar 2018 13:03:40 -0700
> > Subject: [PATCH] perf/x86/intel/uncore: Querying number of CHAs from
> > CAPID6 register
> >
> > The number of CHAs is miscalculated on multi PCI domain systems on
> > Skylake server
> >
> > (From Kroening, Gary:
> >
> > For systems with a single PCI segment, it is sufficient to look for the
> > bus number to change in order to determine that all of the CHa's have
> > been counted for a single socket.
> > However, for multi PCI segment systems, each socket is given a new
> > segment and the bus number does NOT change.  So looking only for the
> > bus number to change ends up counting all of the CHa's on all sockets
> > in the system.  This leads to writing CPU MSRs beyond a valid range and
> > causes an error in ivbep_uncore_msr_init_box().)
> >
> > To determine the number of CHAs, it should read bits 27:0 in the CAPID6
> > register located at Device 30, Function 3, Offset 0x9C. These 28 bits
> > form a bit vector of available LLC slices and the CHAs that manage those
> > slices.
> >
> > Fixes: cd34cd97b7b4 ("perf/x86/intel/uncore: Add Skylake server uncore
> > support")
> > Reported-by: Kroening, Gary <gary.kroening@hpe.com>
> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> > ---
> >   arch/x86/events/intel/uncore_snbep.c | 24 ++++++++++--------------
> >   1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/x86/events/intel/uncore_snbep.c
> > b/arch/x86/events/intel/uncore_snbep.c
> > index d4672ed..a42715b 100644
> > --- a/arch/x86/events/intel/uncore_snbep.c
> > +++ b/arch/x86/events/intel/uncore_snbep.c
> > @@ -3575,24 +3575,20 @@ static struct intel_uncore_type
> > *skx_msr_uncores[] = {
> >   	NULL,
> >   };
> >
> > +#define SKX_CAPID6		0x9c
> > +#define SKX_CHA_BIT_WIDTH	28
> > +
> >   static int skx_count_chabox(void)
> >   {
> > -	struct pci_dev *chabox_dev = NULL;
> > -	int bus, count = 0;
> > +	struct pci_dev *dev = NULL;
> > +	u32 val = 0;
> >
> > -	while (1) {
> > -		chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d,
> > chabox_dev);
> > -		if (!chabox_dev)
> > -			break;
> > -		if (count == 0)
> > -			bus = chabox_dev->bus->number;
> > -		if (bus != chabox_dev->bus->number)
> > -			break;
> > -		count++;
> > -	}
> > +	dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
> > +	if (!dev)
> > +		return 0;
> >
> > -	pci_dev_put(chabox_dev);
> > -	return count;
> > +	pci_read_config_dword(dev, SKX_CAPID6, &val);
> > +	return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
> >   }
> >
> >   void skx_uncore_cpu_init(void)
> > --
> > 2.7.4

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Liang, Kan]

On 3/13/2018 1:06 AM, Kroening, Gary wrote:
> Thanks, Kan -- your patch looks good, and cleaner than the previous method!
> 
> Tonight, I've tested it on our in-house simulator for the following configurations. The simulator has been matching hardware well for this issue -- we'll do more testing on real hardware, but I'm confident you have it right.
>

That's great. Thanks a lot for the verification. :)

Thanks,
Kan

> Terminology:
> 
> "hubless" -- equivalent to "glueless" or "white box" where our BIOS sets things up with a single bus for all sockets.
> 
> "scalable" -- BIOS assigns a new segment/domain for each socket
> 
> Configurations tested so far:
> - single-socket hubless/scalable
> - two sockets, scalable
> - four sockets, hubless
> - eight sockets, scalable
> 
> In all cases, skx_count_chabox() is returning 28 for Skylake server.
> Thanks for the quick response!
> Gary
> 
>> -----Original Message-----
>> From: Liang, Kan [mailto:kan.liang@linux.intel.com]
>> Sent: Monday, March 12, 2018 8:43 PM
>> To: Kroening, Gary; mingo@redhat.com; hpa@zytor.com; tglx@linutronix.de;
>> peterz@infradead.org
>> Cc: Travis, Mike; Banman, Andrew; Sivanich, Dimitri; Anderson, Russ;
>> x86@kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-
>> segment arches
>>
>>
>>
>> On 3/7/2018 3:33 PM, Kroening, Gary wrote:
>>> For systems with a single PCI segment, it is sufficient to look for the
>>> bus number to change in order to determine that all of the CHa's have
>>> been counted for a single socket.
>>>
>>> However, for multi PCI segment systems, each socket is given a new
>>> segment and the bus number does NOT change.  So looking only for the
>>> bus number to change ends up counting all of the CHa's on all sockets
>>> in the system.  This leads to writing CPU MSRs beyond a valid range and
>>> causes an error in ivbep_uncore_msr_init_box().
>>>
>>> The fix is to check for either the bus number or segment number to
>> change.
>>>
>>
>> Hi Gary,
>>
>> There is a recommended way in uncore document to query the number of
>> CHAs on Skylake server.
>> I have a patch to implement the new way.
>>
>> Could you please take a look at the patch and see if it can fix your
>> issue?
>>
>>
>> Thanks,
>> Kan
>>
>> ------
>>   From 55f54b2fa3021c691c2fd4f5cfc8f441fd104e91 Mon Sep 17 00:00:00 2001
>> From: Kan Liang <kan.liang@linux.intel.com>
>> Date: Mon, 12 Mar 2018 13:03:40 -0700
>> Subject: [PATCH] perf/x86/intel/uncore: Querying number of CHAs from
>> CAPID6 register
>>
>> The number of CHAs is miscalculated on multi PCI domain systems on
>> Skylake server
>>
>> (From Kroening, Gary:
>>
>> For systems with a single PCI segment, it is sufficient to look for the
>> bus number to change in order to determine that all of the CHa's have
>> been counted for a single socket.
>> However, for multi PCI segment systems, each socket is given a new
>> segment and the bus number does NOT change.  So looking only for the
>> bus number to change ends up counting all of the CHa's on all sockets
>> in the system.  This leads to writing CPU MSRs beyond a valid range and
>> causes an error in ivbep_uncore_msr_init_box().)
>>
>> To determine the number of CHAs, it should read bits 27:0 in the CAPID6
>> register located at Device 30, Function 3, Offset 0x9C. These 28 bits
>> form a bit vector of available LLC slices and the CHAs that manage those
>> slices.
>>
>> Fixes: cd34cd97b7b4 ("perf/x86/intel/uncore: Add Skylake server uncore
>> support")
>> Reported-by: Kroening, Gary <gary.kroening@hpe.com>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>    arch/x86/events/intel/uncore_snbep.c | 24 ++++++++++--------------
>>    1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/uncore_snbep.c
>> b/arch/x86/events/intel/uncore_snbep.c
>> index d4672ed..a42715b 100644
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -3575,24 +3575,20 @@ static struct intel_uncore_type
>> *skx_msr_uncores[] = {
>>    	NULL,
>>    };
>>
>> +#define SKX_CAPID6		0x9c
>> +#define SKX_CHA_BIT_WIDTH	28
>> +
>>    static int skx_count_chabox(void)
>>    {
>> -	struct pci_dev *chabox_dev = NULL;
>> -	int bus, count = 0;
>> +	struct pci_dev *dev = NULL;
>> +	u32 val = 0;
>>
>> -	while (1) {
>> -		chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d,
>> chabox_dev);
>> -		if (!chabox_dev)
>> -			break;
>> -		if (count == 0)
>> -			bus = chabox_dev->bus->number;
>> -		if (bus != chabox_dev->bus->number)
>> -			break;
>> -		count++;
>> -	}
>> +	dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
>> +	if (!dev)
>> +		return 0;
>>
>> -	pci_dev_put(chabox_dev);
>> -	return count;
>> +	pci_read_config_dword(dev, SKX_CAPID6, &val);
>> +	return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
>>    }
>>
>>    void skx_uncore_cpu_init(void)
>> --
>> 2.7.4
> 

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan <kan.liang@linux.intel.com> wrote:

> +#define SKX_CAPID6             0x9c
> +#define SKX_CHA_BIT_WIDTH      28
> +
>  static int skx_count_chabox(void)
>  {
> +       struct pci_dev *dev = NULL;
> +       u32 val = 0;
>

> +       dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
> +       if (!dev)
> +               return 0;

Where is pci_dev_put()?

>
> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
> +       return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);

UB is here.
Fix is simple, use unsigned long and drop this ugly casting.

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 5:58 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>> +#define SKX_CAPID6             0x9c

>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);

Moreover, this is too non-flexible. Can't you find a capability based
on CAP ID  + offset?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Liang, Kan]

On 3/13/2018 12:00 PM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 5:58 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>> +#define SKX_CAPID6             0x9c
> 
>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
> 
> Moreover, this is too non-flexible. Can't you find a capability based
> on CAP ID  + offset?
> 

It looks it doesn't use capability.

16:1e.3 System peripheral: Intel Corporation Sky Lake-E PCU Registers 
(rev 04)
00: 86 80 83 20 00 00 00 00 04 00 80 08 00 00 80 00
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 00 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Thanks,
Kan

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Liang, Kan]

On 3/13/2018 11:58 AM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan <kan.liang@linux.intel.com> wrote:
> 
>> +#define SKX_CAPID6             0x9c
>> +#define SKX_CHA_BIT_WIDTH      28
>> +
>>   static int skx_count_chabox(void)
>>   {
>> +       struct pci_dev *dev = NULL;
>> +       u32 val = 0;
>>
> 
>> +       dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
>> +       if (!dev)
>> +               return 0;
> 
> Where is pci_dev_put()?
> 
>>
>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
>> +       return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
> 
> UB is here.
> Fix is simple, use unsigned long and drop this ugly casting.
> 

Thanks.
I will fix them in V2.

Thanks,
Kan

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 7:15 PM, Liang, Kan <kan.liang@linux.intel.com> wrote:
> On 3/13/2018 12:00 PM, Andy Shevchenko wrote:
>> On Tue, Mar 13, 2018 at 5:58 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan <kan.liang@linux.intel.com>
>>> wrote:

>>>> +#define SKX_CAPID6             0x9c

>>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);

>> Moreover, this is too non-flexible. Can't you find a capability based
>> on CAP ID  + offset?
>>
>
> It looks it doesn't use capability.

See below. It would be sad if it's true. (Will need comment in that case)

> 16:1e.3 System peripheral: Intel Corporation Sky Lake-E PCU Registers (rev
> 04)
> 00: 86 80 83 20 00 00 00 00 04 00 80 08 00 00 80 00
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 00 00
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Can you instead provide
% lspci -nk -vvv -xx -s 16:1e.3
?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Liang, Kan]

On 3/13/2018 1:22 PM, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 7:15 PM, Liang, Kan <kan.liang@linux.intel.com> wrote:
>> On 3/13/2018 12:00 PM, Andy Shevchenko wrote:
>>> On Tue, Mar 13, 2018 at 5:58 PM, Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan <kan.liang@linux.intel.com>
>>>> wrote:
> 
>>>>> +#define SKX_CAPID6             0x9c
> 
>>>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
> 
>>> Moreover, this is too non-flexible. Can't you find a capability based
>>> on CAP ID  + offset?
>>>
>>
>> It looks it doesn't use capability.
> 
> See below. It would be sad if it's true. (Will need comment in that case)
> 
>> 16:1e.3 System peripheral: Intel Corporation Sky Lake-E PCU Registers (rev
>> 04)
>> 00: 86 80 83 20 00 00 00 00 04 00 80 08 00 00 80 00
>> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 00 00
>> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> Can you instead provide
> % lspci -nk -vvv -xx -s 16:1e.3
> ?

$ lspci -nk -vvv -xx -s 16:1e.3
16:1e.3 0880: 8086:2083 (rev 04)
         Subsystem: 8086:0000
         Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- 
ParErr- Stepping- SERR- FastB2B- DisINTx-
         Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
<TAbort- <MAbort- >SERR- <PERR- INTx-
00: 86 80 83 20 00 00 00 00 04 00 80 08 00 00 80 00
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 00 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 7:28 PM, Liang, Kan <kan.liang@linux.intel.com> wrote:

>>>>>> +#define SKX_CAPID6             0x9c

^^^ This needs a comment.

>>> It looks it doesn't use capability.

> $ lspci -nk -vvv -xx -s 16:1e.3
> 16:1e.3 0880: 8086:2083 (rev 04)
>         Subsystem: 8086:0000
>         Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
> <TAbort- <MAbort- >SERR- <PERR- INTx-
> 00: 86 80 83 20 00 00 00 00 04 00 80 08 00 00 80 00
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 00 00
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Pity.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Liang, Kan]

On 3/13/2018 1:16 PM, Liang, Kan wrote:
> 
> 
> On 3/13/2018 11:58 AM, Andy Shevchenko wrote:
>> On Tue, Mar 13, 2018 at 3:42 AM, Liang, Kan 
>> <kan.liang@linux.intel.com> wrote:
>>
>>> +#define SKX_CAPID6             0x9c
>>> +#define SKX_CHA_BIT_WIDTH      28
>>> +
>>>   static int skx_count_chabox(void)
>>>   {
>>> +       struct pci_dev *dev = NULL;
>>> +       u32 val = 0;
>>>
>>
>>> +       dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x2083, dev);
>>> +       if (!dev)
>>> +               return 0;
>>
>> Where is pci_dev_put()?
>>
>>>
>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
>>> +       return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
>>
>> UB is here.
>> Fix is simple, use unsigned long and drop this ugly casting.
>>

Just noticed that we have to do casting anyway.
pci_read_config_dword uses u32.
bitmap_weight uses unsigned long.

Thanks,
Kan

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

>>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
>>>> +       return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
>>>
>>>
>>> UB is here.
>>> Fix is simple, use unsigned long and drop this ugly casting.
>>>
>
> Just noticed that we have to do casting anyway.

No.

> pci_read_config_dword uses u32.
> bitmap_weight uses unsigned long.

...which would lead to UB.

So,

unsigned long bits;
u32 value;

...(..., &value);
bits = value;
...(&bits, ...);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 7:46 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>>>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
>>>>> +       return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);

Forgot about hweight32(). Can you use that one directly?

hweight32(x & (BIT(_WIDTH) - 1));

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 7:49 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 13, 2018 at 7:46 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>>>>> +       pci_read_config_dword(dev, SKX_CAPID6, &val);
>>>>>> +       return bitmap_weight((unsigned long *)&val, SKX_CHA_BIT_WIDTH);
>
> Forgot about hweight32(). Can you use that one directly?
>
> hweight32(x & (BIT(_WIDTH) - 1));
>
> ?

Or even provide

#define SKX_CHA_BIT_MASK GENMASK(...)



-- 
With Best Regards,
Andy Shevchenko