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

[Gupta, Akshay]

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@xxxxxxx>
> > > Signed-off-by: Akshay Gupta <akshay.gupta@xxxxxxx>
> > > ---
> > > 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.