Re: [PATCH 1/3] platform/x86/amd/hsmp: Check HSMP support on AMD family of processors

["Hegde, Suma" <Suma.Hegde@amd.com>]

Hi Mario,

On 4/23/2024 10:41 PM, Mario Limonciello wrote:
> On 4/23/2024 04:14, Suma Hegde wrote:
>> HSMP interface is supported only on few x86 processors from AMD.
>> Accessing HSMP registers on rest of the platforms might cause
>> unexpected behaviour. So add a check.
>>
>> Also unavailability of this interface on rest of the processors
>> is not an error. Hence, use pr_info() instead of the pr_err() to
>> log the message.
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>>   drivers/platform/x86/amd/hsmp.c | 54 ++++++++++++++++++++++++++++-----
>>   1 file changed, 47 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp.c 
>> b/drivers/platform/x86/amd/hsmp.c
>> index 1927be901108..807a5066dacc 100644
>> --- a/drivers/platform/x86/amd/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp.c
>> @@ -907,16 +907,48 @@ static int hsmp_plat_dev_register(void)
>>       return ret;
>>   }
>>   +/*
>> + * This check is only needed for backward compatibility of previous 
>> platforms.
>> + * All new platforms are expected to support ACPI based probing.
>> + */
>> +static bool is_hsmp_supported(void)
>
> I think it's better to use "legacy" in this function name to make it 
> clear that this isn't a requirement for newer platforms going forward. 
> Maybe like
>
> legacy_hsmp_support()?
Ok. I will rename it.
>
>> +{
>> +    bool ret = true;
>
> As this is a simple boolean return with no needing to clear memory, I 
> think you can just "return true" or "return false" everywhere in this 
> function.
Ok, I will check this.
>
>> +
>> +    if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>> +        return false;
>> +
>> +    switch (boot_cpu_data.x86) {
>> +    case 0x19:
>> +        switch (boot_cpu_data.x86_model) {
>> +        case 0x00 ... 0x1F:
>> +        case 0x30 ... 0x3F:
>> +        case 0x90 ... 0x9F:
>> +        case 0xA0 ... 0xAF:
>> +            break;
>> +        default:
>> +            ret = false;
>> +        }
>> +        break;
>> +    case 0x1A:
>> +        switch (boot_cpu_data.x86_model) {
>> +        case 0x00 ... 0x1F:
>> +            break;
>
> There actually are going to be 0x1A platforms that don't use ACPI?
> I guess I'm surprised.
>
Model 0x00-0x1f are transitional platforms in 0x1A family, where both 
ACPI and non-ACPI is supported.
>> +        default:
>> +            ret = false;
>> +        }
>> +        break;
>> +    default:
>> +        ret = false;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static int __init hsmp_plt_init(void)
>>   {
>>       int ret = -ENODEV;
>>   -    if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || 
>> boot_cpu_data.x86 < 0x19) {
>> -        pr_err("HSMP is not supported on Family:%x model:%x\n",
>> -               boot_cpu_data.x86, boot_cpu_data.x86_model);
>> -        return ret;
>> -    }
>> -
>>       /*
>>        * amd_nb_num() returns number of SMN/DF interfaces present in 
>> the system
>>        * if we have N SMN/DF interfaces that ideally means N sockets
>> @@ -930,7 +962,15 @@ static int __init hsmp_plt_init(void)
>>           return ret;
>>         if (!plat_dev.is_acpi_device) {
>> -        ret = hsmp_plat_dev_register();
>> +        if (is_hsmp_supported()) {
>> +            /* Not ACPI device, but supports HSMP, register a 
>> plat_dev */
>> +            ret = hsmp_plat_dev_register();
>> +        } else {
>> +            /* Not ACPI, Does not support HSMP */
>> +            pr_info("HSMP is not supported on Family:%x model:%x\n",
>> +                boot_cpu_data.x86, boot_cpu_data.x86_model);
>> +            ret = -ENODEV;
>> +        }
>>           if (ret)
>>               platform_driver_unregister(&amd_hsmp_driver);
>>       }
>
Thanks and Regards,

Suma