linux-kernel - Re: [PATCH v7 07/10] misc: amd-sbi: Add support for CPUID protocol

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] [day] [month] [year] [list]

Message-ID: <17283434-d730-4798-8d41-106eff8c98ab@amd.com>
Date: Mon, 7 Apr 2025 17:01:25 +0530
From: "Gupta, Akshay" <Akshay.Gupta@....com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Arnd Bergmann <arnd@...db.de>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 Guenter Roeck <linux@...ck-us.net>, shyam-sundar.s-k@....com,
 gautham.shenoy@....com, Mario Limonciello <mario.limonciello@....com>,
 naveenkrishna.chatradhi@....com, anand.umarji@....com
Subject: Re: [PATCH v7 07/10] misc: amd-sbi: Add support for CPUID protocol


On 4/2/2025 5:46 PM, Greg Kroah-Hartman wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Apr 02, 2025 at 02:13:22PM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 2, 2025, at 07:58, Akshay Gupta wrote:
>>> - AMD provides custom protocol to read Processor feature
>>>    capabilities and configuration information through side band.
>>>    The information is accessed by providing CPUID Function,
>>>    extended function and thread ID to the protocol.
>>>    Undefined function returns 0.
>>>
>>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@....com>
>>> Signed-off-by: Akshay Gupta <akshay.gupta@....com>
>>> ---
>>> Changes since v6:
>>> - Address Arnd comment
>>>    - Add padding to the uapi structure
>>> - Rebased patch, previously patch 8
>> This changes the UAPI again. since you change the common structure
>> layout.
>>
>>> @@ -134,6 +279,9 @@ static long sbrmi_ioctl(struct file *fp, unsigned
>>> int cmd, unsigned long arg)
>>>              /* Mailbox protocol */
>>>              ret = rmi_mailbox_xfer(data, &msg);
>>>              break;
>>> +   case APML_CPUID:
>>> +           ret = rmi_cpuid_read(data, &msg);
>>> +           break;
>>>      default:
>>>              return -EINVAL;
>> As I previously commented, I would prefer to have a highl-level
>> interface per specific mailbox item you transfer, but I think that
>> is something we can debate, in particular if Greg or the x86
>> maintainers think it's ok, I'm not going to object.
>>
>> However, having a combined ioctl command and data structure
>> for rmi_mailbox_xfer(), rmi_cpuid_read() and the commands
>> you add later seems to cause a lot of the extra complexity,
>> and I think this really has to be done differently, using
>> distinct ioctl command numbers for each of them, with an
>> appropriate structure to go along with it.
>>
>> This does mean the existing userspace tool will be incompatible
>> with the upstream driver, but it can be easily updated to
>> support both kernel interfaces (trying the new one first,
>> and falling back to the old on after -ENOTTY).
> Different structures per ioctl is the way to go.  It's more
> self-describing and easier to audit for reviewing that the code is
> working properly both in userspace and in the kernel (i.e. tools like
> strace work better.)
>
> So I agree with you here.
>
> thanks,
>
> greg k-h


Thank you Greg and Arnd,

As suggested, I will create different structures and IOCTLs for each 
protocol (Mailbox, CPUID, MCA MSR and others).

Thank you.