Message-ID: <9367d0f5-815a-e870-c8d0-047ca9f81c3f@redhat.com>
Date:   Wed, 1 Mar 2023 15:46:06 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 05/12] platform/x86: ISST: Add support for MSR 0x54
Content-Language: en-US, nl
To:     srinivas pandruvada <srinivas.pandruvada@linux.intel.com>,
        markgross@kernel.org
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230211063257.311746-1-srinivas.pandruvada@linux.intel.com>
 <20230211063257.311746-6-srinivas.pandruvada@linux.intel.com>
 <1950741c-b5a9-6883-0c16-391001540a25@redhat.com>
 <d79bc5e85ca61cbd978d7b9b349e076b7b94770a.camel@linux.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <d79bc5e85ca61cbd978d7b9b349e076b7b94770a.camel@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@linux.intel.com>
>>
>> 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;
>>
>