Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library

From: Yazen Ghannam <yazen.ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: yazen.ghannam@amd.com, linux-edac@vger.kernel.org,
	linux-kernel@vger.kernel.org, tony.luck@intel.com,
	x86@kernel.org, avadhut.naik@amd.com, john.allen@amd.com,
	william.roche@oracle.com, muralidhara.mk@amd.com
Subject: Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library
Date: Wed, 13 Dec 2023 10:35:55 -0500	[thread overview]
Message-ID: <b9beb435-30e4-44a3-b27d-0567a8e873bd@amd.com> (raw)
In-Reply-To: <20231212153449.GEZXh9mSJ5epD13Dp7@fat_crate.local>

On 12/12/2023 10:34 AM, Borislav Petkov wrote:
> On Tue, Dec 12, 2023 at 09:23:44AM -0500, Yazen Ghannam wrote:
>> I'm thinking that the warning only happens if the "assert" condition above
>> is hit.
> 
> assert usually means "assert - abort the program if assertion is false"
> - from assert(3).
>

Right, agreed. In this context, the program is the translation method. 
But yeah, it doesn't make much sense describing the kernel. I'll change 
the wording if I don't drop the macros completely.

>> In older revisions, I had all these messages as "debug" loglevel. I don't
>> think there's anything a user can do to fix these issues. They're either
>> coding bugs in the library or system configuration.
>>
>> I'd rather go back to the debug messages if you don't mind. It's not
>> difficult to enable dynamic debug messages compared to DEBUG Kconfig
>> options. So I think it'd be okay to work with users on this if they
>> encounter an issue.
> 
> Makes sense.
> 
>>>> +static const struct x86_cpu_id amd_atl_cpuids[] = {
>>>> +	X86_MATCH_FEATURE(X86_FEATURE_SMCA, NULL),
>>>
>>> I'd expect for only this one to be needed, but not those below.
>>>
>>
>> Me too. Those below are to workaround a current module loading issue. I'll
>> add a code comment for that.
> 
> You mean the systemdoofus crap?
> 
> Fget it - we don't fix the kernel because luserspace is nuts.
> 
>>
>>>> +	X86_MATCH_FEATURE(X86_FEATURE_ZEN, NULL),
> 
> ...and those are influx - this is called X86_FEATURE_ZEN1 now and
> X86_FEATURE_ZEN is set on all Zens. So you might as well match on
> X86_FEATURE_ZEN only.
> 
> But you should not need it - if SMCA doesn't match then we have another
> problem. ATL should load on SMCA systems only.
>

I agree in principle. But I don't think it hurts to include an 
additional line to avoid the confusion when the module doesn't load.

Also, the SMCA feature is used here as a short-cut to match on systems 
with a Data Fabric. We could use the Zen feature in the same way.

Thanks,
Yazen