On Wed, 2024-11-27 at 10:11 +0100, Michal Suchánek wrote:

On Tue, Nov 26, 2024 at 12:40:20PM -0800, Haren Myneni wrote:

quoted

On Wed, 2024-11-27 at 00:42 +0530, Mahesh J Salgaonkar wrote:

quoted

On 2024-11-23 21:20:39 Sat, Haren Myneni wrote:
[...]

quoted

+static ssize_t papr_platform_dump_handle_read(struct file
*file,
+		char __user *buf, size_t size, loff_t *off)
+{
+	struct ibm_platform_dump_params *params = file-

quoted

private_data;

+	u64 total_bytes;
+	s32 fwrc;
+
+	/*
+	 * Dump already completed with the previous read calls.
+	 * In case if the user space issues further reads,
returns
+	 * -EINVAL.
+	 */
+	if (!params->buf_length) {
+		pr_warn_once("Platform dump completed for dump
ID
%llu\n",
+			(u64) (((u64)params->dump_tag_hi << 32)
|
+				params->dump_tag_lo));
+		return -EINVAL;
+	}
+
+	if (size < SZ_1K) {
+		pr_err_once("Buffer length should be minimum
1024
bytes\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * The hypervisor returns status 0 if no more data
available to
+	 * download. Then expects the last RTAS call with NULL
buffer
+	 * to invalidate the dump which means dump will be
freed in the
+	 * hypervisor.
+	 */
+	if (params->status == RTAS_IBM_PLATFORM_DUMP_COMPLETE)
{
+		params->buf_length = 0;
+		fwrc = rtas_ibm_platform_dump(params, 0, 0);
+		/*
+		 * Returns 0 (success) to the user space so
that user
+		 * space read stops.

Does this implicitly invalidates the dump irrespective of whether
userspace has requested or not ?

No, the RTAS call from the last read() will invalidate the dump.
Expect
the user space make the read() until returns 0 which means the last
read() will return 0 after invalidate the dump. 

size_t read() 
{
   if (params->status == RTAS_IBM_PLATFORM_DUMP_COMPLETE) {
           RTAS call to invalidate dump
           return 0;
   }
   get the data from RTAS call
   If the RTAS call return status is DUMP_COMPLETE
             params->status = RTAS_IBM_PLATFORM_DUMP_COMPLETE
   return bytes_written
}

If the RTAS call returns DUMP_COMPLETE, the hypervisor expects one
more
RTAS call to invalidate the dump which is done as part of the last
read()

quoted

Copy-pasting bellow code snippet from librtas side patch posted
by
you to
librtas-devel mailing list:
+ /*
+ * rtas_platform_dump() is called with buf = NULL and length = 0
+ * for "dump complete" RTAS call to invalidate dump.
+ * For kernel interface, read() will be continued until the
+ * return value = 0. Means kernel API will return this value
only
+ * after issued "dump complete" call. So nothing to do further
+ * for the last RTAS call.
+ */
+ if (buffer == NULL)
+ return 0;

If I understand this correctly, it looks like calling
rtas_platform_dump() with buf = NULL and length = 0, now does
nothing.

Following the same read() ABI - expects to make calls until returns
size 0.

The current usage of rtas_platform_dump() in ppc64-
diag/rtas_errd/extract_platdump.c

dump_complete = rtas_platform_dump(dump_tag, 0, dump_buf,
DUMP_BUF_SZ,
                                        &seq_next, &bytes);

while (dump_complete) {

   dump_complete = rtas_platform_dump(dump_tag, seq, dump_buf,
 				DUMP_BUF_SZ, &seq_next, &bytes);
                                
}

ret = rtas_platform_dump(dump_tag, seq, NULL, 0, 
                                &seq_next, &bytes);

we need to support both new and old interfaces and not changing the
above code which uses librtas API.

So the new rtas_platform_dump() interface
{
 if the buffer == NULL 
     return - nothing to do here. Dump is invalidated with the
previous
rtas_platform_dump()   
 
 size = read()
 if size == 0 
      dump complete and invalidate the dump
      return 0

  return 1;
}

No EOF?

read() returns size, 0 or < 0. Returns 0 is like EOF. 

So no standard file handling code can use this FD?

Yes, providing support for FD from ioctl for the following reasons:

- get-vpd char driver is providing only for FD from ioctl. So thought
of using same interface for platform-dump so that having consitent
interface for all RTAS function char drivers. 

- file->private_data is assigned to miscdevice in misc_register and
also assigned to some other miscdevice struct in driver specific code.
So was thinking of not following semantics in the existing code if I
private_data to save internal param struct.

Please let me know if you prefer to use FD from open() for platform-
dump read().

But also the size 0 read both indicates the EOF and invalidates the
dump, these should be separate.

Which differs from the hypervisor API that makes it impossible to
save
the dump without deleting it, and introduces a regression.

The hypervisor API says to invalidate the dump after saving it. In the
current interface it does - The user space makes the last read() (for
return 0) after saving the complete dump. 

All read() calls return size (> 0) == RTAS calls for dump
read() expects return 0 == same RTAS invalidate dump

So the only difference is if the user does not call to invalidate dump
explicitly even though saved the dump, but we do not have the current
usage, Only the extract-dump command is the only usage now and please
note that this command is used for non-HMC manages system. It is not
used on HMC managed system which has its own command gets the dump
directly from the hypervisor.    

If you are doing IOCTLs anyway the invalidation could be an IOCTL. Or
you could really follow the RTAS ABI and only incalidate if the user
passes NULL and 0.

I could use this ioctl interface to invalidate the dump.

devfd = ioctl(fd ..) for dump ID
read(devfd)
ret = ioctl(devfd ...) to invalidate dump 

I will make changes if you are OK with this interface

But more generally the previously added RTAS wrappers do not closely
follow the RTAS ABI, and do accumulation of read data in the kernel,
passing the whole buffer to userspace afterwards.

Why cannot it be done in this case?

platform-dump RTAS returns more data unlike in get-vpd. In one case
noticed 3G data which is not the ideal case to save in the kernel
buffer within ioctl. 

Also platform-dump RTAS function supports interleave calls for multiple
dump IDs at the same time unlike in get-vpd case.

Even more generally if the dump IDs are stable these could be listed
in
some sysfs or debugfs directory, and provide standard file
operations,
including unlink() to remove the dump.

dump IDs are not stable. The hypervisor can have several dumps with
different IDs at the same time. so unlink() can not be used.

With the bonus that at least one of these filesystems has some
provisions for confidentiality lockdown. The implementation could use
that to make the dumps unavailable in confidentiality lockdown level
if
the dumps are considered confidential without reimplementing the
check.

We are adding new driver (interfaces) to support lockdown. Otherwise
the current usage is sufficient. But we could modify to restrict the
interface for confidentiality lockdown in future if we have that
restriction.

Thanks
Haren

Thanks

Michal