[PATCH] powerpc/pseries: Add papr-platform-dump character driver for dump retrieval

[Haren Myneni]

On Wed, 2024-11-27 at 00:42 +0530, Mahesh J Salgaonkar wrote:
> On 2024-11-23 21:20:39 Sat, Haren Myneni wrote:
> [...]
> > +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->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()

> 
> 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;
}


> Dump is already invalidated even before userspace makes this call.
> Wouldn't this break ABI ? Correct me if I am wrong.
> 
> Consider a scenario where userspace tool using librtas interface
> rtas_platform_dump() reads the last set of data but unable to write
> it
> to the disk due to insufficient disk space. In that case, tool may
> error
> out without invalidating the dump and expect user to cleanup the disk
> space, re-run the tool to save platform dump to disk. If last read
> implicitly invalidates the dump, then in this scenario user will
> loose
> the platform dump. Shouldn't we wait for explicit request from user
> to
> invalidate the dump to avoid this ?

In case if the user space could not proceed with more read() calls due
to disk space issue or etc, the dump is still valid and the user space
can restart with sequenece 0.

it means the dump has to be written completly to the disk before
issuing the last read() call which returns size = 0 after invalidating
the dump.

Thanks
Haren

> 
> Thanks,
> -Mahesh.