Re: [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue

Re: [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue

[Shuah Khan]

On 4/19/21 8:27 PM, 徐福海 wrote:
> From: xufuhai<xufuhai@kuaishou.com>
> 
> For the old  AMD processor (family < 0x17), cpupower will call the
> amd_pci_get_num_boost_states function, but for the non-root user
> pci_read_byte function (implementation comes from the psutil library),
> val will be set to 0xff, indicating that there is no read function
> callback. At this time, the original logic will set the cpupower turbo
> active state to yes. This is an obvious issue~
> 
> Reproduce procedure:
> 	cpupower frequency-info
> 
> Reported-by: yangrui<yangrui@kuaishou.com>
> Signed-off-by: xufuhai<xufuhai@kuaishou.com>

Also your Signed-off-by should match the from address.
There is a mismatch between the two.

> Signed-off-by: chenguanqiao<chenguanqiao@kuaishou.com>
> Signed-off-by: lishujin<lishujin@kuaishou.com>
> Reviewed-by: Thomas Renninger<trenn@suse.com>
> ---
>   tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
> index 97f2c857048e..6f9504906afa 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
>   		return -ENODEV;
>   
>   	val = pci_read_byte(device, 0x15c);
> +
> +	/* If val is 0xff, meaning has no permisson to
> +	 * get the boost states, return -1
> +	 */
> +	if (val == 0xff)
> +		return -1;
> +
>   	if (val & 3)
>   		*active = 1;
>   	else
> -- 
> 2.24.3 (Apple Git-128)
> 

I am seeing two patches with the same commit summary,
should these two be a singles patch?

https://patchwork.kernel.org/project/linux-pm/patch/6e35df20-753a-6c9c-8786-3fc87cdd17ba@gmail.com/

Please combine the two and send single patch if they fix the
same problem. If not, please change the commit log to reflect
the difference.

thanks,
-- Shuah

Re: [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue

[徐福海]

okay, I believe the two patches are for fixing defferent issue, I will update my patches as your mention

THANKS

在 2021/4/24 上午6:26, Shuah Khan 写道:
> On 4/19/21 8:27 PM, 徐福海 wrote:
>> From: xufuhai<xufuhai@kuaishou.com>
>>
>> For the old  AMD processor (family < 0x17), cpupower will call the
>> amd_pci_get_num_boost_states function, but for the non-root user
>> pci_read_byte function (implementation comes from the psutil library),
>> val will be set to 0xff, indicating that there is no read function
>> callback. At this time, the original logic will set the cpupower turbo
>> active state to yes. This is an obvious issue~
>>
>> Reproduce procedure:
>>     cpupower frequency-info
>>
>> Reported-by: yangrui<yangrui@kuaishou.com>
>> Signed-off-by: xufuhai<xufuhai@kuaishou.com>
>
> Also your Signed-off-by should match the from address.
> There is a mismatch between the two.
>
>> Signed-off-by: chenguanqiao<chenguanqiao@kuaishou.com>
>> Signed-off-by: lishujin<lishujin@kuaishou.com>
>> Reviewed-by: Thomas Renninger<trenn@suse.com>
>> ---
>>   tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
>> index 97f2c857048e..6f9504906afa 100644
>> --- a/tools/power/cpupower/utils/helpers/amd.c
>> +++ b/tools/power/cpupower/utils/helpers/amd.c
>> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
>>           return -ENODEV;
>>         val = pci_read_byte(device, 0x15c);
>> +
>> +    /* If val is 0xff, meaning has no permisson to
>> +     * get the boost states, return -1
>> +     */
>> +    if (val == 0xff)
>> +        return -1;
>> +
>>       if (val & 3)
>>           *active = 1;
>>       else
>> -- 
>> 2.24.3 (Apple Git-128)
>>
>
> I am seeing two patches with the same commit summary,
> should these two be a singles patch?
>
> https://patchwork.kernel.org/project/linux-pm/patch/6e35df20-753a-6c9c-8786-3fc87cdd17ba@gmail.com/
>
> Please combine the two and send single patch if they fix the
> same problem. If not, please change the commit log to reflect
> the difference.
>
> thanks,
> -- Shuah
>
>

Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[Thomas Renninger]

Hi,

Am Mittwoch, 24. März 2021, 09:28:37 CEST schrieb xufuhai:
> From: xufuhai <xufuhai@kuaishou.com>
> 
> For the old  AMD processor (family < 0x17), cpupower will call the
> amd_pci_get_num_boost_states function, but for the non-root user
> pci_read_byte function (implementation comes from the psutil library),
> val will be set to 0xff, indicating that there is no read function
> callback. At this time, the original logic will set the cpupower turbo
> active state to yes. This is an obvious issue~
> 
> Reproduce procedure:
> 	cpupower frequency-info
> 
> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
> ---
>  tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c
> b/tools/power/cpupower/utils/helpers/amd.c index 97f2c857048e..6f9504906afa
> 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int
> *states) return -ENODEV;
> 
>  	val = pci_read_byte(device, 0x15c);
> +
> +	/* If val is 0xff, meaning has no permisson to
> +	 * get the boost states, return -1
> +	 */
> +	if (val == 0xff)
> +		return -1;
> +
There is certainly a cleaner way to do this.., theoretically
pci_read_byte can return 0xff in other cases?

But I guess this is a sufficient way to handle this for now.

Reviewed-by: Thomas Renninger <trenn@suse.de>

Thanks,

        Thomas

Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[xufuhai]

hi Thomas,

Thanks for your reply

I believe the only way that pci_read_byte can return 0xff is no permission to 
access the pci_dev read function. Because for pci_read_byte, the pos offset is
0x15c that the offset has excessed the capacity of pci_dev cache_len, so can't
get val via memcpy from cache. 

And then for read callback function, I think that is read val from pci_dev 0x15c 
register. I have looked up the amd family 0x15 code, the 0x15c register is called
"Core Performance Boost Control Register" and this register base addr is D18F4x15C.
We just concern the lower 8bit of D18F4x15C register, the detailed as below: 

Quote from https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/796af1
7f18554380a49d69d7768ac18ee039d711/src/vendorcode/amd/agesa/f15/Proc/CPU/Family/0x15/cpuF15PowerMgmt.h

/* Core Performance Boost Control Register D18F4x15C */
#define CPB_CTRL_REG 0x15C
#define CPB_CTRL_PCI_ADDR (MAKE_SBDFO (0, 0, 0x18, FUNC_4, CPB_CTRL_REG))
/// Core Performance Boost Control Register of Family 15h common aceess
typedef struct {
  UINT32 BoostSrc:2;                 ///< Boost source
  UINT32 NumBoostStates:3;           ///< Number of boosted states
  UINT32 :2;                         ///< Reserved
  UINT32 ApmMasterEn:1;              ///< APM master enable
  UINT32 :23;                        ///< Reserved
  UINT32 BoostLock:1;                ///<
} F15_CPB_CTRL_REGISTER;

the amd 0x15 family specification pdf:
https://www.amd.com/system/files/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf

so I believe that val is nealy impossible to return 0xff unless no permisson to access read callback.

what do you think? Thomas

And at last I have a new doubt why the amd_pci_get_num_boost_states about setting "*active=1"
condition is "val & 3", should not be "val & 1"? I accidently found that "00b Boosting disabled / 01b Boosting enabled / 1*b Reserved" 
via above amd specification about D18F4x15C register description.
I am just curious about this point, maybe I am wrong~ : 

	if (val & 3)
		*active = 1;
	else
		*active = 0;

Thanks for your reading, Thomas

在 2021/3/29 下午7:10, Thomas Renninger 写道:
> Hi,
> 
> Am Mittwoch, 24. März 2021, 09:28:37 CEST schrieb xufuhai:
>> From: xufuhai <xufuhai@kuaishou.com>
>>
>> For the old  AMD processor (family < 0x17), cpupower will call the
>> amd_pci_get_num_boost_states function, but for the non-root user
>> pci_read_byte function (implementation comes from the psutil library),
>> val will be set to 0xff, indicating that there is no read function
>> callback. At this time, the original logic will set the cpupower turbo
>> active state to yes. This is an obvious issue~
>>
>> Reproduce procedure:
>> 	cpupower frequency-info
>>
>> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
>> ---
>>  tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/power/cpupower/utils/helpers/amd.c
>> b/tools/power/cpupower/utils/helpers/amd.c index 97f2c857048e..6f9504906afa
>> 100644
>> --- a/tools/power/cpupower/utils/helpers/amd.c
>> +++ b/tools/power/cpupower/utils/helpers/amd.c
>> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int
>> *states) return -ENODEV;
>>
>>  	val = pci_read_byte(device, 0x15c);
>> +
>> +	/* If val is 0xff, meaning has no permisson to
>> +	 * get the boost states, return -1
>> +	 */
>> +	if (val == 0xff)
>> +		return -1;
>> +
> There is certainly a cleaner way to do this.., theoretically
> pci_read_byte can return 0xff in other cases?
> 
> But I guess this is a sufficient way to handle this for now.
> 
> Reviewed-by: Thomas Renninger <trenn@suse.de>
> 
> Thanks,
> 
>         Thomas
> 
> 

Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[xufuhai]

Any reply? Thomas

在 2021/3/30 上午11:47, xufuhai 写道:
> hi Thomas,
> 
> Thanks for your reply
> 
> I believe the only way that pci_read_byte can return 0xff is no permission to 
> access the pci_dev read function. Because for pci_read_byte, the pos offset is
> 0x15c that the offset has excessed the capacity of pci_dev cache_len, so can't
> get val via memcpy from cache. 
> 
> And then for read callback function, I think that is read val from pci_dev 0x15c 
> register. I have looked up the amd family 0x15 code, the 0x15c register is called
> "Core Performance Boost Control Register" and this register base addr is D18F4x15C.
> We just concern the lower 8bit of D18F4x15C register, the detailed as below: 
> 
> Quote from https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/796af1
> 7f18554380a49d69d7768ac18ee039d711/src/vendorcode/amd/agesa/f15/Proc/CPU/Family/0x15/cpuF15PowerMgmt.h
> 
> /* Core Performance Boost Control Register D18F4x15C */
> #define CPB_CTRL_REG 0x15C
> #define CPB_CTRL_PCI_ADDR (MAKE_SBDFO (0, 0, 0x18, FUNC_4, CPB_CTRL_REG))
> /// Core Performance Boost Control Register of Family 15h common aceess
> typedef struct {
>   UINT32 BoostSrc:2;                 ///< Boost source
>   UINT32 NumBoostStates:3;           ///< Number of boosted states
>   UINT32 :2;                         ///< Reserved
>   UINT32 ApmMasterEn:1;              ///< APM master enable
>   UINT32 :23;                        ///< Reserved
>   UINT32 BoostLock:1;                ///<
> } F15_CPB_CTRL_REGISTER;
> 
> the amd 0x15 family specification pdf:
> https://www.amd.com/system/files/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf
> 
> so I believe that val is nealy impossible to return 0xff unless no permisson to access read callback.
> 
> what do you think? Thomas
> 
> And at last I have a new doubt why the amd_pci_get_num_boost_states about setting "*active=1"
> condition is "val & 3", should not be "val & 1"? I accidently found that "00b Boosting disabled / 01b Boosting enabled / 1*b Reserved" 
> via above amd specification about D18F4x15C register description.
> I am just curious about this point, maybe I am wrong~ : 
> 
> 	if (val & 3)
> 		*active = 1;
> 	else
> 		*active = 0;
> 
> Thanks for your reading, Thomas
> 
> 在 2021/3/29 下午7:10, Thomas Renninger 写道:
>> Hi,
>>
>> Am Mittwoch, 24. März 2021, 09:28:37 CEST schrieb xufuhai:
>>> From: xufuhai <xufuhai@kuaishou.com>
>>>
>>> For the old  AMD processor (family < 0x17), cpupower will call the
>>> amd_pci_get_num_boost_states function, but for the non-root user
>>> pci_read_byte function (implementation comes from the psutil library),
>>> val will be set to 0xff, indicating that there is no read function
>>> callback. At this time, the original logic will set the cpupower turbo
>>> active state to yes. This is an obvious issue~
>>>
>>> Reproduce procedure:
>>> 	cpupower frequency-info
>>>
>>> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
>>> ---
>>>  tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/tools/power/cpupower/utils/helpers/amd.c
>>> b/tools/power/cpupower/utils/helpers/amd.c index 97f2c857048e..6f9504906afa
>>> 100644
>>> --- a/tools/power/cpupower/utils/helpers/amd.c
>>> +++ b/tools/power/cpupower/utils/helpers/amd.c
>>> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int
>>> *states) return -ENODEV;
>>>
>>>  	val = pci_read_byte(device, 0x15c);
>>> +
>>> +	/* If val is 0xff, meaning has no permisson to
>>> +	 * get the boost states, return -1
>>> +	 */
>>> +	if (val == 0xff)
>>> +		return -1;
>>> +
>> There is certainly a cleaner way to do this.., theoretically
>> pci_read_byte can return 0xff in other cases?
>>
>> But I guess this is a sufficient way to handle this for now.
>>
>> Reviewed-by: Thomas Renninger <trenn@suse.de>
>>
>> Thanks,
>>
>>         Thomas
>>
>>

[PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[xufuhai]

From: xufuhai <xufuhai@kuaishou.com>

For the old  AMD processor (family < 0x17), cpupower will call the
amd_pci_get_num_boost_states function, but for the non-root user
pci_read_byte function (implementation comes from the psutil library),
val will be set to 0xff, indicating that there is no read function
callback. At this time, the original logic will set the cpupower turbo
active state to yes. This is an obvious issue~

Reproduce procedure:
	cpupower frequency-info

Signed-off-by: xufuhai <xufuhai@kuaishou.com>
Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com>
Signed-off-by: lishujin <lishujin@kuaishou.com>
---
 tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 97f2c857048e..6f9504906afa 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
 		return -ENODEV;
 
 	val = pci_read_byte(device, 0x15c);
+
+	/* If val is 0xff, meaning has no permisson to
+	 * get the boost states, return -1
+	 */
+	if (val == 0xff)
+		return -1;
+
 	if (val & 3)
 		*active = 1;
 	else
-- 
2.24.3 (Apple Git-128)

Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[Shuah Khan]

On 3/24/21 4:27 AM, xufuhai wrote:
> From: xufuhai <xufuhai@kuaishou.com>
> 
> For the old  AMD processor (family < 0x17), cpupower will call the
> amd_pci_get_num_boost_states function, but for the non-root user
> pci_read_byte function (implementation comes from the psutil library),
> val will be set to 0xff, indicating that there is no read function
> callback. At this time, the original logic will set the cpupower turbo
> active state to yes. This is an obvious issue~
> 

Please run get_maintainer.pl and send patch maintainers
and others suggested by the tool. I don't see this in my
Inbox for me to review/accept and send it to pm maintainer.

Hmm. I am seeing on my AMD

as non-root
current CPU frequency: Unable to call hardware

as root
current CPU frequency: 3.60 GHz (asserted by call to hardware)

Are you sure this change is necessary?

Please give me more information on this fix.


> Reproduce procedure:
> 	cpupower frequency-info
> 
> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com>
> Signed-off-by: lishujin <lishujin@kuaishou.com>
> ---
>   tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
> index 97f2c857048e..6f9504906afa 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
>   		return -ENODEV;
>   
>   	val = pci_read_byte(device, 0x15c);
> +
> +	/* If val is 0xff, meaning has no permisson to

Typo: permisson. Reads better as "0xff indicates user
doesn't have permission to read boot states"

> +	 * get the boost states, return -1
> +	 */
> +	if (val == 0xff)
> +		return -1;
> +
>   	if (val & 3)
>   		*active = 1;
>   	else
> 

thanks,
-- Shuah

Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[xufuhai]

yeah Shuah~ thanks for your reply

For this issue, not meaning "current CPU frequency" but "boost state support--->Active" during 
"cpupower frequency-info" command as below:

	boost state support:
    		Supported: yes
    		Active: yes/no

I think the state returned from the command for amd cpu (family < 0x17) should be like as below:

	as non-root Active state:
		Active: Error while evaluating Boost Capabilities on CPU xx -- are you root?
	
	as root Active state:
		Active: yes (if supported)
			no  (if not supprted)

I don't wanna see the state returned like below:
	
	as non-root Active state:
		Active: yes
	
	as root Active state:
		Active: yes (if supported)
			no  (if not supprted)
	
I will paste the related code by detailed for showing the issue:
	
	if amd cpu family < 0x17 , will run amd_pci_get_num_boost_states function:
-----------------------------------------------------
/linux/tools/power/cpupower/utils/helper/misc.c
 
	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB) {
		*support = 1;

		/* AMD Family 0x17 does not utilize PCI D18F4 like prior
		 * families and has no fixed discrete boost states but
		 * has Hardware determined variable increments instead.
		 */

		if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
			if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
				if (!(val & CPUPOWER_AMD_CPBDIS))
					*active = 1;
			}
		} else {
			ret = amd_pci_get_num_boost_states(active, states);
			if (ret)
				return ret;
		}
---------------------------------------------------

/linux/tools/power/cpupower/utils/helper/amd.c
amd_pci_get_num_boost_states:
	
	val = pci_read_byte(device, 0x15c);

 	if (val & 3)
 		*active = 1;
 	else
----------------------------------------------------

pci_read_byte will memset val to 0xff if caller has no permission to access to read from pci_dev
but for amd_pci_get_num_boost_states, active state will set 1 meaning "yes". I believe that active
state should return negative value to caller function meaning "have no permission" will showing "
Error while evaluating Boost Capabilities on CPU xx -- are you root?"  

----------------------------------------------------
static inline void
pci_read_data(struct pci_dev *d, void *buf, int pos, int len)
{
  if (pos & (len-1))
    d->access->error("Unaligned read: pos=%02x, len=%d", pos, len);
  if (pos + len <= d->cache_len)
    memcpy(buf, d->cache + pos, len);
  else if (!d->methods->read(d, pos, buf, len))
    memset(buf, 0xff, len);
}

byte
pci_read_byte(struct pci_dev *d, int pos)
{
  byte buf;
  pci_read_data(d, &buf, pos, 1);
  return buf;
}
----------------------------------------------------


在 2021/3/24 下午6:27, xufuhai 写道:
> From: xufuhai <xufuhai@kuaishou.com>
> 
> For the old  AMD processor (family < 0x17), cpupower will call the
> amd_pci_get_num_boost_states function, but for the non-root user
> pci_read_byte function (implementation comes from the psutil library),
> val will be set to 0xff, indicating that there is no read function
> callback. At this time, the original logic will set the cpupower turbo
> active state to yes. This is an obvious issue~
> 
> Reproduce procedure:
> 	cpupower frequency-info
> 
> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com>
> Signed-off-by: lishujin <lishujin@kuaishou.com>
> ---
>  tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
> index 97f2c857048e..6f9504906afa 100644
> --- a/tools/power/cpupower/utils/helpers/amd.c
> +++ b/tools/power/cpupower/utils/helpers/amd.c
> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
>  		return -ENODEV;
>  
>  	val = pci_read_byte(device, 0x15c);
> +
> +	/* If val is 0xff, meaning has no permisson to
> +	 * get the boost states, return -1
> +	 */
> +	if (val == 0xff)
> +		return -1;
> +
>  	if (val & 3)
>  		*active = 1;
>  	else
>