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

2023-09-06 Thread Michal Suchánek
On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch 
> 
> 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, );
>   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
> file contents are immutable from the POV of user space. To get a new
> view of VPD, clients must create a new handle.
> 
> 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.)
> 
> 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.
> 
> 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?
> 
> Signed-off-by: Nathan Lynch 
> ---
>  Documentation/userspace-api/ioctl/ioctl-number.rst |   2 +
>  arch/powerpc/include/uapi/asm/papr-vpd.h   |  29 ++
>  arch/powerpc/platforms/pseries/Makefile|   1 +
>  arch/powerpc/platforms/pseries/papr-vpd.c  | 353 
> +
>  4 files changed, 385 insertions(+)
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
> b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 4ea5b837399a..a950545bf7cd 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -349,6 +349,8 @@ Code  Seq#Include File
>Comments
>   
> 
>  0xB1  00-1F  PPPoX
>   
> 
> +0xB2  00 arch/powerpc/include/uapi/asm/papr-vpd.h
> powerpc/pseries VPD API
> + 
> 
>  0xB3  00 linux/mmc/ioctl.h
>  0xB4  00-0F  linux/gpio.h
> 
>  0xB5  00-0F  uapi/linux/rpmsg.h  
> 
> diff --git a/arch/powerpc/include/uapi/asm/papr-vpd.h 
> b/arch/powerpc/include/uapi/asm/papr-vpd.h
> new file mode 100644
> index ..aa33217ad5de
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr-vpd.h
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
> +#ifndef _UAPI_PAPR_VPD_H_
> +#define _UAPI_PAPR_VPD_H_
> +
> +#include 
> +#include 
> +
> +struct papr_location_code {
> + /*
> +  * PAPR+ 12.3.2.4 Converged Location Code Rules - Length
> +  * Restrictions. 79 characters plus nul.
> +  */
> + char str[80];
> +};
> +
> +#define PAPR_VPD_IOCTL_BASE 0xb2
> +
> +#define PAPR_VPD_IO(nr) _IO(PAPR_VPD_IOCTL_BASE, nr)
> +#define PAPR_VPD_IOR(nr, type)  _IOR(PAPR_VPD_IOCTL_BASE, nr, type)
> +#define PAPR_VPD_IOW(nr, type)  _IOW(PAPR_VPD_IOCTL_BASE, nr, type)
> 

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

2023-09-05 Thread Michal Suchánek
On Tue, Sep 05, 2023 at 12:42:11PM +1000, Michael Ellerman wrote:
> Michal Suchánek  writes:
> > On Thu, Aug 31, 2023 at 12:59:25PM -0500, Nathan Lynch wrote:
> ...
> >> 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.
> >
> > We don't have that, directly accessing /dev/mem does not really work.
> > And that's what needs fixing in my view.
> >
> > The rtas calls are all mechanically the same, the function implemented
> > here should be able to call any of them if there was a way to specify
> > the call.
> >
> > Given that there is desire to have access to multiple calls I don't
> > think it makes sense to allocate a separate device with different name
> > for each.
> 
> I think it does make sense.
> 
> We explicitly don't want a general "call any RTAS function" API.
> 
> We want tightly scoped APIs that do one thing, or a family of related
> things, but not anything & everything.
> 
> Having different devices for each of those APIs means permissions can be
> granted separately on those devices. So a user/group can be given access
> to the "papr-vpd" device, but not some other unrelated device that also
> happens to expose an RTAS service (eg. error injection).

Yes, it does work as a kludge for setting permissions for individual
calls.

Thanks

Michal


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

2023-09-04 Thread Michael Ellerman
Michal Suchánek  writes:
> On Thu, Aug 31, 2023 at 12:59:25PM -0500, Nathan Lynch wrote:
...
>> 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.
>
> We don't have that, directly accessing /dev/mem does not really work.
> And that's what needs fixing in my view.
>
> The rtas calls are all mechanically the same, the function implemented
> here should be able to call any of them if there was a way to specify
> the call.
>
> Given that there is desire to have access to multiple calls I don't
> think it makes sense to allocate a separate device with different name
> for each.

I think it does make sense.

We explicitly don't want a general "call any RTAS function" API.

We want tightly scoped APIs that do one thing, or a family of related
things, but not anything & everything.

Having different devices for each of those APIs means permissions can be
granted separately on those devices. So a user/group can be given access
to the "papr-vpd" device, but not some other unrelated device that also
happens to expose an RTAS service (eg. error injection).

cheers


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

2023-09-04 Thread Michal Suchánek
On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
> Michal Suchánek  writes:
> > 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 
> >> 
> >> 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, );
> >>   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.

With the device named rtas-vpd it's clearly tied to rtas.

If 'version 2' of the call happens it's more likely than not going to
have new data format because limit of current format was reached. Then
emulating that old call with the new one would be counterproductive or
impossible.

Even if the same data is available through different call there is no
problem. If the user really used the well-known "ibm,get-vpd" identifier
documented in the specification then the kernel can translate it
internally to whatever new method for obtaining the data exists. The
current revisions of the specification are not going to go away, and the
identifier is still well-known and documented, even if newer revisions
of the platform use different way to provide the data to the kernel.

Sure, with the current syscall interface it would not work because the
user translates this well-known identifier into a function token, and
passes that to the kernel. With that if the "ibm,get-vpd" is gone the
kernel cannot provide the data anymore.

That was done to make it possible to call functions that were not yet
known when the kernel was written. This is no longer allowed, and the
kernel has functionality for translating function names to tokens for
the functions it does know about. Then it can do the translation for
userspace as well, and when the call is implemented differently in the
future abstract that detail away.

> > 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.

Instead, you create a device in the filesystem, and assign an IOCTL, and
need to tell the userspace how to find both.

> 
> This pattern of creating file descriptors from existing file descriptors
> to model a hiearachy of objects is well established in eg. the KVM and
> DRM APIs.

Yet there is no object hierarchy to speak of here. There is one device
with one ioctl on it. The device name is tied to this specific call so a
different call will need both a new device and new IOCTL.

> 
> > Maybe creating a directory in sysfs or procfs with filenames
> > corresponding to rtas services would do the same job without extra
> > obfuscation?
> 
> It's not obfuscation, it's abstraction. The kernel talks to firmware to
> do things, and provides an API to user space. Not all the details of how
> the firmware works are relevant to user space, including the exact
> firmware calls required to implement a certain feature.

Well, it's not static data, it's a call. There might be different ways
to go around passing the arguments in and getting the results out.

Hiding the buffer management and chunking of the resulting data is an
abstraction, great.

Hiding the translation of well-known function name to the
system-specific token is an abstraction, great.

Translating the rtas error codes to well-known 

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

2023-09-04 Thread Michal Suchánek
Hello,

On Thu, Aug 31, 2023 at 12:59:25PM -0500, Nathan Lynch wrote:
> Michal Suchánek  writes:
> > On Thu, Aug 31, 2023 at 09:37:12PM +1000, Michael Ellerman wrote:
> >> Michal Suchánek  writes:
> >> > On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
> >> >> Michal Suchánek  writes:
> >> >> > On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay 
> >> >> > wrote:
> >> >> >> From: Nathan Lynch 
> >> >> >> 
> >> >> >> 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, );
> >> >> >>   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 :-)

and does that really change anything?

> 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.

We don't have that, directly accessing /dev/mem does not really work.
And that's what needs fixing in my view.

The rtas calls are all mechanically the same, the function implemented
here should be able to call any of them if there was a way to specify
the call.

Given that there is desire to have access to multiple calls I don't
think it makes sense to allocate a separate device with different name
for each.

The ioclt interface is not nice for this. however.

Given that different calls have different parameters having one ioctl
for all of them would be quite messy.

Still even as is the ioctl takes a string argument which is problematic
on its own.

Given that there is an argument to the call it 

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

2023-08-31 Thread Nathan Lynch
Michal Suchánek  writes:
> On Thu, Aug 31, 2023 at 09:37:12PM +1000, Michael Ellerman wrote:
>> Michal Suchánek  writes:
>> > On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
>> >> Michal Suchánek  writes:
>> >> > On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay 
>> >> > wrote:
>> >> >> From: Nathan Lynch 
>> >> >> 
>> >> >> 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, );
>> >> >>   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.


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

2023-08-31 Thread Nathan Lynch
Michal Suchánek  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 
>> 
>> 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, );
>>   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.


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

2023-08-31 Thread Michal Suchánek
On Thu, Aug 31, 2023 at 09:37:12PM +1000, Michael Ellerman wrote:
> Michal Suchánek  writes:
> > On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
> >> Michal Suchánek  writes:
> >> > On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay 
> >> > wrote:
> >> >> From: Nathan Lynch 
> >> >> 
> >> >> 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, );
> >> >>   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


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

2023-08-31 Thread Michael Ellerman
Michal Suchánek  writes:
> On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
>> Michal Suchánek  writes:
>> > On Tue, Aug 22, 2023 at 04:33:39PM -0500, Nathan Lynch via B4 Relay wrote:
>> >> From: Nathan Lynch 
>> >> 
>> >> 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, );
>> >>   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 yes the ioctl is defined in a header, not in the filesystem, but
that's entirely normal for an ioctl based API.

cheers


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

2023-08-31 Thread Michal Suchánek
On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
> Michal Suchánek  writes:
> > 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 
> >> 
> >> 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, );
> >>   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.

Still the kernel could use the name that is well-known today even if it
uses different implementation internally in the future.

> 
> > 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.
> 
> This pattern of creating file descriptors from existing file descriptors
> to model a hiearachy of objects is well established in eg. the KVM and
> DRM APIs.

> 
> >> 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?
> 
> I don't see why that is related to netlink. It's entirely normal for
> file descriptor based APIs to implement poll.
> 
> netlink adds a lot of complexity for zero gain IMO.

It kind of solves the problem with shoehorning something that's not
really a file into file descriptors. You don't have to when not using
them. It also solves how to access multiple services without creating a
large number of files and large number of obscure constants.

Thanks

Michal


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

2023-08-31 Thread Michal Suchánek
On Thu, Aug 31, 2023 at 03:34:37PM +1000, Michael Ellerman wrote:
> Michal Suchánek  writes:
> > 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 
> >> 
> >> 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, );
> >>   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.

Thanks

Michal


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

2023-08-30 Thread Michael Ellerman
Michal Suchánek  writes:
> 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 
>> 
>> 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, );
>>   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.

This pattern of creating file descriptors from existing file descriptors
to model a hiearachy of objects is well established in eg. the KVM and
DRM APIs.

> Maybe creating a directory in sysfs or procfs with filenames
> corresponding to rtas services would do the same job without extra
> obfuscation?

It's not obfuscation, it's abstraction. The kernel talks to firmware to
do things, and provides an API to user space. Not all the details of how
the firmware works are relevant to user space, including the exact
firmware calls required to implement a certain feature.

>> 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.

Yes :)

>> 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?

I don't see why that is related to netlink. It's entirely normal for
file descriptor based APIs to implement poll.

netlink adds a lot of complexity for zero gain IMO.

cheers


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

2023-08-30 Thread Michal Suchánek
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 
> 
> 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, );
>   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


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

2023-08-22 Thread Nathan Lynch via B4 Relay
From: Nathan Lynch 

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, );
  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
file contents are immutable from the POV of user space. To get a new
view of VPD, clients must create a new handle.

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.)

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.

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?

Signed-off-by: Nathan Lynch 
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   2 +
 arch/powerpc/include/uapi/asm/papr-vpd.h   |  29 ++
 arch/powerpc/platforms/pseries/Makefile|   1 +
 arch/powerpc/platforms/pseries/papr-vpd.c  | 353 +
 4 files changed, 385 insertions(+)

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 4ea5b837399a..a950545bf7cd 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -349,6 +349,8 @@ Code  Seq#Include File  
 Comments
  

 0xB1  00-1F  PPPoX
  

+0xB2  00 arch/powerpc/include/uapi/asm/papr-vpd.h
powerpc/pseries VPD API
+ 

 0xB3  00 linux/mmc/ioctl.h
 0xB4  00-0F  linux/gpio.h

 0xB5  00-0F  uapi/linux/rpmsg.h  

diff --git a/arch/powerpc/include/uapi/asm/papr-vpd.h 
b/arch/powerpc/include/uapi/asm/papr-vpd.h
new file mode 100644
index ..aa33217ad5de
--- /dev/null
+++ b/arch/powerpc/include/uapi/asm/papr-vpd.h
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
+#ifndef _UAPI_PAPR_VPD_H_
+#define _UAPI_PAPR_VPD_H_
+
+#include 
+#include 
+
+struct papr_location_code {
+   /*
+* PAPR+ 12.3.2.4 Converged Location Code Rules - Length
+* Restrictions. 79 characters plus nul.
+*/
+   char str[80];
+};
+
+#define PAPR_VPD_IOCTL_BASE 0xb2
+
+#define PAPR_VPD_IO(nr) _IO(PAPR_VPD_IOCTL_BASE, nr)
+#define PAPR_VPD_IOR(nr, type)  _IOR(PAPR_VPD_IOCTL_BASE, nr, type)
+#define PAPR_VPD_IOW(nr, type)  _IOW(PAPR_VPD_IOCTL_BASE, nr, type)
+#define PAPR_VPD_IOWR(nr, type) _IOWR(PAPR_VPD_IOCTL_BASE, nr, type)
+
+/*
+ * ioctl for /dev/papr-vpd. Returns a VPD handle fd corresponding to
+ * the location code.
+ */
+#define PAPR_VPD_CREATE_HANDLE PAPR_VPD_IOW(0, struct papr_location_code)
+
+#endif /* _UAPI_PAPR_VPD_H_ */
diff --git