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

["Michal Suchánek" <msuchanek@suse.de>]

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 <nathanl@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