* RE: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 1:42 ` Liang, Kan
@ 2018-03-13 5:06 ` Kroening, Gary
2018-03-13 12:28 ` Liang, Kan
2018-03-13 5:24 ` Kroening, Gary
2018-03-13 15:58 ` Andy Shevchenko
2 siblings, 1 reply; 16+ messages in thread
From: Kroening, Gary @ 2018-03-13 5:06 UTC (permalink / raw)
To: Liang, Kan, mingo, hpa, tglx, peterz
Cc: Travis, Mike, Banman, Andrew, Sivanich, Dimitri, Anderson, Russ,
x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 5:06 ` Kroening, Gary
@ 2018-03-13 12:28 ` Liang, Kan
0 siblings, 0 replies; 16+ messages in thread
From: Liang, Kan @ 2018-03-13 12:28 UTC (permalink / raw)
To: Kroening, Gary, mingo, hpa, tglx, peterz
Cc: Travis, Mike, Banman, Andrew, Sivanich, Dimitri, Anderson, Russ,
x86, linux-kernel
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 1:42 ` Liang, Kan
2018-03-13 5:06 ` Kroening, Gary
@ 2018-03-13 5:24 ` Kroening, Gary
2018-03-13 15:58 ` Andy Shevchenko
2 siblings, 0 replies; 16+ messages in thread
From: Kroening, Gary @ 2018-03-13 5:24 UTC (permalink / raw)
To: Liang, Kan, mingo, hpa, tglx, peterz
Cc: Travis, Mike, Banman, Andrew, Sivanich, Dimitri, Anderson, Russ,
x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 1:42 ` Liang, Kan
2018-03-13 5:06 ` Kroening, Gary
2018-03-13 5:24 ` Kroening, Gary
@ 2018-03-13 15:58 ` Andy Shevchenko
2018-03-13 16:00 ` Andy Shevchenko
2018-03-13 17:16 ` Liang, Kan
2 siblings, 2 replies; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 15:58 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 15:58 ` Andy Shevchenko
@ 2018-03-13 16:00 ` Andy Shevchenko
2018-03-13 17:15 ` Liang, Kan
2018-03-13 17:16 ` Liang, Kan
1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 16:00 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 16:00 ` Andy Shevchenko
@ 2018-03-13 17:15 ` Liang, Kan
2018-03-13 17:22 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Liang, Kan @ 2018-03-13 17:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:15 ` Liang, Kan
@ 2018-03-13 17:22 ` Andy Shevchenko
2018-03-13 17:28 ` Liang, Kan
0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 17:22 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:22 ` Andy Shevchenko
@ 2018-03-13 17:28 ` Liang, Kan
2018-03-13 17:31 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Liang, Kan @ 2018-03-13 17:28 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:28 ` Liang, Kan
@ 2018-03-13 17:31 ` Andy Shevchenko
0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 17:31 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 15:58 ` Andy Shevchenko
2018-03-13 16:00 ` Andy Shevchenko
@ 2018-03-13 17:16 ` Liang, Kan
2018-03-13 17:42 ` Liang, Kan
1 sibling, 1 reply; 16+ messages in thread
From: Liang, Kan @ 2018-03-13 17:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:16 ` Liang, Kan
@ 2018-03-13 17:42 ` Liang, Kan
2018-03-13 17:46 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Liang, Kan @ 2018-03-13 17:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:42 ` Liang, Kan
@ 2018-03-13 17:46 ` Andy Shevchenko
2018-03-13 17:49 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 17:46 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
>>>> + 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:46 ` Andy Shevchenko
@ 2018-03-13 17:49 ` Andy Shevchenko
2018-03-13 17:50 ` Andy Shevchenko
0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 17:49 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/platform/x86: Fix count of CHas on multi-pci-segment arches
2018-03-13 17:49 ` Andy Shevchenko
@ 2018-03-13 17:50 ` Andy Shevchenko
0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2018-03-13 17:50 UTC (permalink / raw)
To: Liang, Kan
Cc: Kroening, Gary, mingo, hpa, tglx, peterz, Travis, Mike, Banman,
Andrew, Sivanich, Dimitri, Anderson, Russ, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread