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

[Nathan Lynch]

Michal Suchánek <msuchanek at suse.de> writes:
>
> thanks for working on this.

Thanks for reviewing!


> 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.
>
> 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().
>
> Maybe creating a directory in sysfs or procfs with filenames
> corresponding to rtas services would do the same job without extra
> obfuscation?

Michael has already said most of what I would have on these points. I'll
add that my experience with user space software that interacts closely
with RTAS leads me to believe that the kernel can and should expose
these platform features in higher-level ways that address fundamental
needs while encapsulating complexities such as caller serialization and
the mechanism (RTAS vs hcall vs DT). In this case, the fact that the
ibm,get-vpd RTAS function is the PAPR-specified interface for the OS to
retrieve VPD is much less essential to vpdupdate/libvpd than the format
of the VPD returned.


>> * The only way to discover the size of a VPD buffer is
>>   lseek(SEEK_END). fstat() doesn't work for anonymous fds like
>>   this. Is this OK, or should the buffer length be discoverable some
>>   other way?
>
> So long as users have /rtas/ibm,vpd-size as the top bound of the data
> they can receive I don't think it's critical to know the current VPD
> size.

OK, well I want to be transparent that the spec language says it's the
"estimated" max size, so I would hesitate to use it to size buffers
without some fallback for when it's wrong.