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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b9beb435-30e4-44a3-b27d-0567a8e873bd@amd.com>
Date:   Wed, 13 Dec 2023 10:35:55 -0500
From:   Yazen Ghannam <yazen.ghannam@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     yazen.ghannam@....com, linux-edac@...r.kernel.org,
        linux-kernel@...r.kernel.org, tony.luck@...el.com, x86@...nel.org,
        avadhut.naik@....com, john.allen@....com, william.roche@...cle.com,
        muralidhara.mk@....com
Subject: Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library

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