Re: [PATCH v2 1/2] platforms/x86: Add AMD system management interface

[Nathan Fontenot <nafonten@amd.com>]

On 2/3/22 10:03, Carlos Bilbao wrote:
> Hello,
> 
> On 2/3/2022 9:48 AM, Nathan Fontenot wrote:
>> On 2/3/22 06:04, Naveen Krishna Chatradhi wrote:
>>> From: Suma Hegde <suma.hegde@amd.com>
>>>
>>> (...)
>>> +static bool check_model_support(void)
>>> +{
>>> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
>>> +	    boot_cpu_data.x86 != 0x19)
>>
>> Check for boot_cpu_data.x86 < 0x19, lets avoid having to do driver updates any
>> time future families support HSMP.
>>
>>> +		return false;
>>> +
>>> +	switch (boot_cpu_data.x86_model) {
>>> +	case 0x00 ... 0x1f:
>>> +	case 0x30 ... 0x3f:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>
>> I don't think the model check here is necessary. Adding a model check will only
>> add driver updates for any new models.
>>
>> HSMP is only supported on f19h so we do want to keep that. For the different
>> models that may not support HSMP, the call to hsmp_test() in the init routine
>> will fail if HSMP isn't supported.
> 
> So, for this I think Naveen was trying to avoid people with Fam19th client processors 
> complaining about errors in HSMP and filing bugs. You are equally right pointing out 
> that updating this in the future would be inconvenient. Personally, if I put myself in 
> the shoes of an owner of one of those systems, I would be a bit disappointed to run HSMP
> and just fail. So, maybe a solution in the middle would be to remove the model-checking code 
> but also include a warning/clarification in the documentation. 

I agree that some kind of updated message would be good.

The family check is already made and an appropriate message that HSMP is not supported
is made.

Removing the model check and using hsmp_test() needs to account for hsmp_test failing
in two ways. The call can fail because we are on a model that does not support
HSMP, or it can fail because HSMP has been disabled in the BIOS.

The hsmp_test() call should output a message along the lines of "HSMP is not supported
on this family/model or has been disabled in BIOS."

-Nathan

> 
> Thanks,
> Carlos.