Re: [PATCH v4 09/13] powerpc/pseries: Add papr-vpd character driver for VPD retrieval

2023-11-28 Thread Michael Ellerman 
Nathan Lynch  writes:
> Michael Ellerman  writes:
>
>> Nathan Lynch via B4 Relay 
>> writes:
>>> From: Nathan Lynch 
>>>
>>> PowerVM LPARs may retrieve Vital Product Data (VPD) for system
>>> components using the ibm,get-vpd RTAS function.
>> ...
>>>
>>> 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
>>> + 
>>> 
>>  
>> This hunk should probably go in the previous patch.
>
> The papr-sysparm driver (patch 11/13 "powerpc/pseries/papr-sysparm:
> Expose character device to user space") also adds a line to
> ioctl-number.rst. Are you saying all the additions to ioctl-number.rst
> should be contained in a single patch?

No.

I just meant that the previous patch is where we initially expose the
0xB2 value via uapi, which is the point of no return. So preferably the
documentation is updated by or before that point to reflect that the
0xB2 value is now reserved.

The change log of that patch also talks about allocating a value from
the ioctl-number table, but then doesn't update the table.

cheers

Re: [PATCH v4 09/13] powerpc/pseries: Add papr-vpd character driver for VPD retrieval

2023-11-28 Thread Nathan Lynch 
Michael Ellerman  writes:

> Nathan Lynch via B4 Relay 
> writes:
>> From: Nathan Lynch 
>>
>> PowerVM LPARs may retrieve Vital Product Data (VPD) for system
>> components using the ibm,get-vpd RTAS function.
> ...
>>
>> 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
>> + 
>> 
>  
> This hunk should probably go in the previous patch.

The papr-sysparm driver (patch 11/13 "powerpc/pseries/papr-sysparm:
Expose character device to user space") also adds a line to
ioctl-number.rst. Are you saying all the additions to ioctl-number.rst
should be contained in a single patch?

Re: [PATCH v4 09/13] powerpc/pseries: Add papr-vpd character driver for VPD retrieval

2023-11-28 Thread Michael Ellerman 
Nathan Lynch via B4 Relay 
writes:
> From: Nathan Lynch 
>
> PowerVM LPARs may retrieve Vital Product Data (VPD) for system
> components using the ibm,get-vpd RTAS function.
...
>
> 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
> + 
> 
 
This hunk should probably go in the previous patch.

cheers

Re: [PATCH v4 09/13] powerpc/pseries: Add papr-vpd character driver for VPD retrieval

2023-11-28 Thread Nathan Lynch 
Michal Suchánek  writes:
>
> On Fri, Nov 17, 2023 at 11:14:27PM -0600, Nathan Lynch via B4 Relay wrote:
>> +do {
>> +blob = papr_vpd_run_sequence(loc_code);
>> +if (!IS_ERR(blob)) /* Success. */
>> +break;
>> +if (PTR_ERR(blob) != -EAGAIN) /* Hard error. */
>> +break;
>> +pr_info_ratelimited("VPD changed during retrieval, retrying\n");
>> +cond_resched();
>> +} while (!fatal_signal_pending(current));
>
> this is defined in linux/sched/signal.h which is not included.
>


>> +static long papr_vpd_create_handle(struct papr_location_code __user *ulc)
>> +{
>> +struct papr_location_code klc;
>> +const struct vpd_blob *blob;
>> +struct file *file;
>> +long err;
>> +int fd;
>> +
>> +if (copy_from_user(, ulc, sizeof(klc)))
>> +return -EFAULT;
>
> This is defined in linux/uaccess.h which is not included.
>
> Same for the sysparm driver.
>
> Tested-by: Michal Suchánek 

Thanks, I'll fix these issues and add your T-B to this patch.

Re: [PATCH v4 09/13] powerpc/pseries: Add papr-vpd character driver for VPD retrieval

2023-11-21 Thread Michal Suchánek 
Hello,

On Fri, Nov 17, 2023 at 11:14:27PM -0600, 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_RDONLY);
>   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 the VPD, the client 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;
>   interleaved 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 abandon the sequence and start
>   over.
> 
> 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.
> 
> Note that the driver needs to serialize its call sequences with legacy
> sys_rtas(ibm,get-vpd) callers, so it exposes its internal lock for
> sys_rtas.
> 
> Signed-off-by: Nathan Lynch 
> ---
>  Documentation/userspace-api/ioctl/ioctl-number.rst |   2 +
>  arch/powerpc/include/uapi/asm/papr-vpd.h   |  22 +
>  arch/powerpc/platforms/pseries/Makefile|   1 +
>  arch/powerpc/platforms/pseries/papr-vpd.c  | 536 
> +
>  4 files changed, 561 insertions(+)

> diff --git a/arch/powerpc/platforms/pseries/Makefile 
> b/arch/powerpc/platforms/pseries/Makefile
> index 1476c5e4433c..f936962a2946 100644
> --- a/arch/powerpc/platforms/pseries/Makefile
> +++ b/arch/powerpc/platforms/pseries/Makefile
> @@ -4,6 +4,7 @@ ccflags-$(CONFIG_PPC_PSERIES_DEBUG)   += -DDEBUG
>  
>  obj-y:= lpar.o hvCall.o nvram.o reconfig.o \
>  of_helpers.o rtas-work-area.o papr-sysparm.o \
> +papr-vpd.o \
>  setup.o iommu.o event_sources.o ras.o \
>  firmware.o power.o dlpar.o mobility.o rng.o \
>  pci.o pci_dlpar.o eeh_pseries.o msi.o \
> diff --git a/arch/powerpc/platforms/pseries/papr-vpd.c 
> b/arch/powerpc/platforms/pseries/papr-vpd.c
> new file mode 100644
> index ..2bc52301a402
> --- /dev/null
> +++ b/arch/powerpc/platforms/pseries/papr-vpd.c
> @@ -0,0 +1,536 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) "papr-vpd: " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
...
> +/**
> + * papr_vpd_retrieve() - Return the VPD for a location code.
> + * @loc_code: Location code that defines the scope of VPD to return.
> + *
> + * Run VPD sequences against @loc_code until a blob is successfully
> + * instantiated, or a hard error is encountered, or a fatal signal is
> + * pending.
> + *
> + * Context: May sleep.
> + * Return: A fully populated VPD blob when successful. Encoded error
> + * pointer otherwise.
> + */
> +static const struct vpd_blob *papr_vpd_retrieve(const struct 
> papr_location_code *loc_code)
> +{
> + const struct vpd_blob *blob;
> +
> + /*
> +  * EAGAIN means the sequence errored with a -4 (VPD changed)
> +  * status from ibm,get-vpd, and we should attempt a new
> +  * sequence. PAPR+ R1–7.3.20–5 indicates that this should be a
> +  * transient condition, not something that happens
> +  * continuously. But we'll stop trying on a fatal signal.
> +  */
> + do {
> + blob = papr_vpd_run_sequence(loc_code);
> + if (!IS_ERR(blob)) /* Success. */
> + break;
> + if (PTR_ERR(blob) != -EAGAIN) /* Hard error. */
> + break;
> + pr_info_ratelimited("VPD changed during retrieval,

[PATCH v4 09/13] powerpc/pseries: Add papr-vpd character driver for VPD retrieval

2023-11-17 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_RDONLY);
  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 the VPD, the client 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;
  interleaved 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 abandon the sequence and start
  over.

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.

Note that the driver needs to serialize its call sequences with legacy
sys_rtas(ibm,get-vpd) callers, so it exposes its internal lock for
sys_rtas.

Signed-off-by: Nathan Lynch 
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   2 +
 arch/powerpc/include/uapi/asm/papr-vpd.h   |  22 +
 arch/powerpc/platforms/pseries/Makefile|   1 +
 arch/powerpc/platforms/pseries/papr-vpd.c  | 536 +
 4 files changed, 561 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 ..b62e4f897a70
--- /dev/null
+++ b/arch/powerpc/include/uapi/asm/papr-vpd.h
@@ -0,0 +1,22 @@
+/* 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];
+};
+
+/*
+ * ioctl for /dev/papr-vpd. Returns a VPD handle fd corresponding to
+ * the location code.
+ */
+#define PAPR_VPD_IOC_CREATE_HANDLE _IOW(PAPR_MISCDEV_IOC_ID, 0, struct 
papr_location_code)
+
+#endif /* _UAPI_PAPR_VPD_H_ */
diff --git a/arch/powerpc/platforms/pseries/Makefile 
b/arch/powerpc/platforms/pseries/Makefile
index 1476c5e4433c..f936962a2946 100644
--- a/arch/powerpc/platforms/pseries/Makefile
+++ b/arch/powerpc/platforms/pseries/Makefile
@@ -4,6 +4,7 @@ ccflags-$(CONFIG_PPC_PSERIES_DEBUG) += -DDEBUG
 
 obj-y  := lpar.o hvCall.o nvram.o reconfig.o \
   of_helpers.o rtas-work-area.o papr-sysparm.o \
+  papr-vpd.o \
   setup.o iommu.o event_sources.o ras.o \
   firmware.o power.o dlpar.o mobility.o rng.o \
   pci.o pci_dlpar.o eeh_pseries.o msi.o \
diff --git a/arch/powerpc/platforms/pseries/papr-vpd.c 
b/arch/powerpc/platforms/pseries/papr-vpd.c
new file mode 100644