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

Message-ID: <2025040258-snap-aerospace-6e43@gregkh>
Date: Wed, 2 Apr 2025 13:16:58 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: Akshay Gupta <akshay.gupta@....com>, 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 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