[PATCH RFC 1/2] powerpc/pseries: papr-vpd char driver for VPD retrieval

[Nathan Lynch]

Michal Suchánek <msuchanek at suse.de> writes:
> On Thu, Aug 31, 2023 at 09:37:12PM +1000, Michael Ellerman wrote:
>> Michal Suchánek <msuchanek at suse.de> writes:
>> > On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
>> >> Michal Suchánek <msuchanek at suse.de> writes:
>> >> > On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay wrote:
>> >> >> From: Nathan Lynch <nathanl at linux.ibm.com>
>> >> >> 
>> >> >> PowerVM LPARs may retrieve Vital Product Data (VPD) for system
>> >> >> components using the ibm,get-vpd RTAS function.
>> >> >> 
>> >> >> We can expose this to user space with a /dev/papr-vpd character
>> >> >> device, where the programming model is:
>> >> >> 
>> >> >>   struct papr_location_code plc = { .str = "", }; /* obtain all VPD */
>> >> >>   int devfd = open("/dev/papr-vpd", O_WRONLY);
>> >> >>   int vpdfd = ioctl(devfd, PAPR_VPD_CREATE_HANDLE, &plc);
>> >> >>   size_t size = lseek(vpdfd, 0, SEEK_END);
>> >> >>   char *buf = malloc(size);
>> >> >>   pread(devfd, buf, size, 0);
>> >> >> 
>> >> >> When a file descriptor is obtained from ioctl(PAPR_VPD_CREATE_HANDLE),
>> >> >> the file contains the result of a complete ibm,get-vpd sequence. The
>> >> >
>> >> > Could this be somewhat less obfuscated?
>> >> >
>> >> > What the caller wants is the result of "ibm,get-vpd", which is a
>> >> > well-known string identifier of the rtas call.
>> >> 
>> >> Not really. What the caller wants is *the VPD*. Currently that's done
>> >> by calling the RTAS "ibm,get-vpd" function, but that could change in
>> >> future. There's RTAS calls that have been replaced with a "version 2" in
>> >> the past, that could happen here too. Or the RTAS call could be replaced
>> >> by a hypercall (though unlikely).
>> >> 
>> >> But hopefully if the underlying mechanism changed the kernel would be
>> >> able to hide that detail behind this new API, and users would not need
>> >> to change at all.
>> >> 
>> >> > Yet this identifier is never passed in. Instead we have this new
>> >> > PAPR_VPD_CREATE_HANDLE. This is a completely new identifier, specific to
>> >> > this call only as is the /dev/papr-vpd device name, another new
>> >> > identifier.
>> >> >
>> >> > Maybe the interface could provide a way to specify the service name?
>> >> >
>> >> >> file contents are immutable from the POV of user space. To get a new
>> >> >> view of VPD, clients must create a new handle.
>> >> >
>> >> > Which is basically the same as creating a file descriptor with open().
>> >> 
>> >> Sort of. But much cleaner becuase you don't need to create a file in the
>> >> filesystem and tell userspace how to find it.
>> >
>> > You very much do. There is the /dev/papr-vpd and PAPR_VPD_CREATE_HANDLE
>> > which userspace has to know about, the PAPR_VPD_CREATE_HANDLE is not
>> > even possible to find at all.
>> 
>> Well yeah you need the device itself :)
>
> And as named it's specific to this call, and new devices will be needed
> for any additional rtas called implemented.
>
>> 
>> And yes the ioctl is defined in a header, not in the filesystem, but
>> that's entirely normal for an ioctl based API.
>
> Of course, because the ioctl API has no safe way of passing a string
> identifier for the function. Then it needs to create these obscure IDs.
>
> Other APIs that don't have this problem exist.

Looking at the cover letter for the series, I wonder if my framing and
word choice is confusing? Instead of "new character devices for RTAS
functions", what I would really like to convey is "new character devices
for platform features that are currently only accessible through the
rtas() syscall". But that's too long :-)

You (Michal) seem to favor a kernel-user ABI where user space is allowed
to invoke arbitrary RTAS functions by name. But we already have that in
the form of the rtas() syscall. (User space looks up function tokens by
name in the DT.) The point of the series is that we need to move away
from that. It's too low-level and user space has to use /dev/mem when
invoking any of the less-simple RTAS functions.