Hello,
thanks for working on this.
On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch <nath...@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?
> This design choice insulates user space from most of the complexities
> that ibm,get-vpd brings:
>
> * ibm,get-vpd must be called more than once to obtain complete
> results.
> * Only one ibm,get-vpd call sequence should be in progress at a time;
> concurrent sequences will disrupt each other. Callers must have a
> protocol for serializing their use of the function.
> * A call sequence in progress may receive a "VPD changed, try again"
> status, requiring the client to start over. (The driver does not yet
> handle this, but it should be easy to add.)
That certainly reduces the complexity of the user interface making it
much saner.
> The memory required for the VPD buffers seems acceptable, around 20KB
> for all VPD on one of my systems. And the value of the
> /rtas/ibm,vpd-size DT property (the estimated maximum size of VPD) is
> consistently 300KB across various systems I've checked.
>
> I've implemented support for this new ABI in the rtas_get_vpd()
> function in librtas, which the vpdupdate command currently uses to
> populate its VPD database. I've verified that an unmodified vpdupdate
> binary generates an identical database when using a librtas.so that
> prefers the new ABI.
>
> Likely remaining work:
>
> * Handle RTAS call status -4 (VPD changed) during ibm,get-vpd call
> sequence.
> * Prevent ibm,get-vpd calls via rtas(2) from disrupting ibm,get-vpd
> call sequences in this driver.
> * (Maybe) implement a poll method for delivering notifications of
> potential changes to VPD, e.g. after a partition migration.
That sounds like something for netlink. If that is desired maybe it
should be used in the first place?
> Questions, points for discussion:
>
> * Am I allocating the ioctl numbers correctly?
> * 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.
Thanks
Michal
