Hi,
On 3/1/23 15:41, srinivas pandruvada wrote:
> On Wed, 2023-03-01 at 15:30 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/11/23 07:32, Srinivas Pandruvada wrote:
>>> To map Linux CPU numbering scheme to hardware CPU numbering scheme
>>> MSR 0x53 is getting used. But for new generation of CPUs, this MSR
>>> is not valid. Since this is model specific MSR, this is possible.
>>>
>>> A new MSR 0x54 is defined. Use this MSR and convert the IOCTL
>>> format
>>> to match existing MSR 0x53, in this case user spaces don't need to
>>> be aware of this change.
>>>
>>> Signed-off-by: Srinivas Pandruvada
>>> <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>>
>> I am not a fan of this. I expect that users of these new CPUs will
>> very likely also need a new intel-speed-select userspace tool
>> regardless
>> of doing this MSR munging/shuffling in the kernel. So why not fix
>> the tool to teach it about the MSR instead ?
>
> Sure.
>
> I can remove the format conversion in the kernel, so that user space
> tool will do that.
>
> I think that's what you mean.
Yes that is what I mean.
>> If you have good arguments for doing this in the kernel please
>> add them the commit message for the next version, but my initial
>> reaction to this is that it is wrong to do this in the kernel
>> and that the tool should be fixed instead. So my preference
>> would be for this patch to be dropped from the next version of
>> the patch-set.
>
> Since we can't read MSR from user space, this patch is still required
> to read only MSR 0x54. Just it will not do any format conversion. So
> format conversion will happen in user space tool.
Yes that would be great, having the kernel read the MSR is fine
(of course).
Regards,
Hans
>>> ---
>>> .../intel/speed_select_if/isst_if_common.c | 51
>>> +++++++++++++++++++
>>> 1 file changed, 51 insertions(+)
>>>
>>> diff --git
>>> a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
>>> b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
>>> index 60e58b0b3835..97d1b4566535 100644
>>> --- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
>>> +++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
>>> @@ -19,9 +19,13 @@
>>> #include <linux/uaccess.h>
>>> #include <uapi/linux/isst_if.h>
>>>
>>> +#include <asm/cpu_device_id.h>
>>> +#include <asm/intel-family.h>
>>> +
>>> #include "isst_if_common.h"
>>>
>>> #define MSR_THREAD_ID_INFO 0x53
>>> +#define MSR_PM_LOGICAL_ID 0x54
>>> #define MSR_CPU_BUS_NUMBER 0x128
>>>
>>> static struct isst_if_cmd_cb punit_callbacks[ISST_IF_DEV_MAX];
>>> @@ -31,6 +35,7 @@ static int punit_msr_white_list[] = {
>>> MSR_CONFIG_TDP_CONTROL,
>>> MSR_TURBO_RATIO_LIMIT1,
>>> MSR_TURBO_RATIO_LIMIT2,
>>> + MSR_PM_LOGICAL_ID,
>>> };
>>>
>>> struct isst_valid_cmd_ranges {
>>> @@ -73,6 +78,8 @@ struct isst_cmd {
>>> u32 param;
>>> };
>>>
>>> +static bool isst_hpm_support;
>>> +
>>> static DECLARE_HASHTABLE(isst_hash, 8);
>>> static DEFINE_MUTEX(isst_hash_lock);
>>>
>>> @@ -411,11 +418,43 @@ static int isst_if_cpu_online(unsigned int
>>> cpu)
>>> isst_cpu_info[cpu].pci_dev[1] =
>>> _isst_if_get_pci_dev(cpu, 1, 30, 1);
>>> }
>>>
>>> + if (isst_hpm_support) {
>>> + u64 raw_data;
>>> +
>>> + ret = rdmsrl_safe(MSR_PM_LOGICAL_ID, &raw_data);
>>> + if (!ret) {
>>> + /*
>>> + * Use the same format as MSR 53, for user
>>> space harmony
>>> + * Format
>>> + * Bit 0 – thread ID
>>> + * Bit 8:1 – core ID
>>> + * Bit 13:9 – Compute domain ID (aka
>>> die ID)
>>> + * From the MSR 0x54 format
>>> + * [15:11] PM_DOMAIN_ID
>>> + * [10:3] MODULE_ID (aka IDI_AGENT_ID)
>>> + * [2:0] LP_ID (We don't care about
>>> these bits we only
>>> + * care die and core
>>> id
>>> + * For Atom:
>>> + * [2] Always 0
>>> + * [1:0] core ID within module
>>> + * For Core
>>> + * [2:1] Always 0
>>> + * [0] thread ID
>>> + */
>>> + data = (raw_data >> 11) & 0x1f;
>>> + data <<= 9;
>>> + data |= (((raw_data >> 3) & 0xff) << 1);
>>> + goto set_punit_id;
>>> + }
>>> + }
>>> +
>>> ret = rdmsrl_safe(MSR_THREAD_ID_INFO, &data);
>>> if (ret) {
>>> isst_cpu_info[cpu].punit_cpu_id = -1;
>>> return ret;
>>> }
>>> +
>>> +set_punit_id:
>>> isst_cpu_info[cpu].punit_cpu_id = data;
>>>
>>> isst_restore_msr_local(cpu);
>>> @@ -704,6 +743,12 @@ static struct miscdevice isst_if_char_driver =
>>> {
>>> .fops = &isst_if_char_driver_ops,
>>> };
>>>
>>> +static const struct x86_cpu_id hpm_cpu_ids[] = {
>>> + X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X, NULL),
>>> + X86_MATCH_INTEL_FAM6_MODEL(SIERRAFOREST_X, NULL),
>>> + {}
>>> +};
>>> +
>>> static int isst_misc_reg(void)
>>> {
>>> mutex_lock(&punit_misc_dev_reg_lock);
>>> @@ -711,6 +756,12 @@ static int isst_misc_reg(void)
>>> goto unlock_exit;
>>>
>>> if (!misc_usage_count) {
>>> + const struct x86_cpu_id *id;
>>> +
>>> + id = x86_match_cpu(hpm_cpu_ids);
>>> + if (id)
>>> + isst_hpm_support = true;
>>> +
>>> misc_device_ret = isst_if_cpu_info_init();
>>> if (misc_device_ret)
>>> goto unlock_exit;
>>
>
