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

[Mario Limonciello <mario.limonciello@amd.com>]

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()?

> +{
> +	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.

> +
> +	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.

> +		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);
>   	}