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

[Michal Suchánek]

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.

Thanks

Michal