[PATCH v2] ppc/fadump: collect dump if the collected size is lesser than reserved

[PATCH v2] ppc/fadump: collect dump if the collected size is lesser than reserved

[Shivang Upadhyay]

During Fadump in Qemu VM, when maxcpus value is set to more than current
cpus, following failure is observed.

    [0.000000] rtas: Dump taken by platform is incomplete (-1)

This is because the CPU_STATE_DATA is allocated for maxcpus, while the
data is only filled for current cpus. As per current implementation
of Fadump, dumped_bytes and source_len for a region have to match,
Which is failing for qemu's case, Even tough the dumped bytes are
reported correctly for only the current cpus. After this /proc/vmcore
generatition also fails.

Allowing dumped_bytes to be lesser than or equal to allocated length, for
CPU_STATE_DATA Fadump region.

Reported-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
---
ChangeLog:

v2: Only allowing lesser size for CPU_STATE_DATE Fadump region.

v1: https://lore.kernel.org/qemu-devel/20260429065127.366813-1-shivangu@linux.ibm.com/
---
 arch/powerpc/platforms/pseries/rtas-fadump.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.c b/arch/powerpc/platforms/pseries/rtas-fadump.c
index 3bb4ac2ab6cc..ea1a0a8ac9eb 100644
--- a/arch/powerpc/platforms/pseries/rtas-fadump.c
+++ b/arch/powerpc/platforms/pseries/rtas-fadump.c
@@ -459,7 +459,10 @@ static int __init rtas_fadump_process(struct fw_dump *fadump_conf)
 	/* Check if the dump data is valid. */
 	for (int i = 0; i < be16_to_cpu(fdm_active->header.dump_num_sections); i++) {
 		int type = be16_to_cpu(fdm_active->rgn[i].source_data_type);
+		uint64_t bytes_dumped = be64_to_cpu(fdm_active->rgn[i].bytes_dumped);
+		uint64_t source_len = be64_to_cpu(fdm_active->rgn[i].source_len);
 		int rc = 0;
+		int region_collected;
 
 		switch (type) {
 		case RTAS_FADUMP_CPU_STATE_DATA:
@@ -469,8 +472,18 @@ static int __init rtas_fadump_process(struct fw_dump *fadump_conf)
 				pr_err("Dump taken by platform is not valid (%d)\n", i);
 				rc = -EINVAL;
 			}
-			if (fdm_active->rgn[i].bytes_dumped != fdm_active->rgn[i].source_len) {
+			/*
+			 * Make sure that dump is collected for entire region.
+			 * CPU_STATE_DATA region is allowed to dump less than allocated space.
+			 */
+			region_collected = (bytes_dumped == source_len ||
+	                         (type == RTAS_FADUMP_CPU_STATE_DATA && bytes_dumped <= source_len));
+
+
+			if (! region_collected) {
 				pr_err("Dump taken by platform is incomplete (%d)\n", i);
+				pr_debug("type -> %d, bytes_dumped -> %llx, source_len -> %llx\n",
+				         type, bytes_dumped, source_len);
 				rc = -EINVAL;
 			}
 			if (rc) {
@@ -482,7 +495,7 @@ static int __init rtas_fadump_process(struct fw_dump *fadump_conf)
 			}
 			break;
 		case RTAS_FADUMP_PARAM_AREA:
-			if (fdm_active->rgn[i].bytes_dumped != fdm_active->rgn[i].source_len ||
+			if (bytes_dumped != source_len ||
 			    fdm_active->rgn[i].error_flags != 0) {
 				pr_warn("Failed to process additional parameters! Proceeding anyway..\n");
 				fadump_conf->param_area = 0;
-- 
2.54.0

Re: [PATCH v2] ppc/fadump: collect dump if the collected size is lesser than reserved

[Sourabh Jain]

On 26/08/26 18:26, Shivang Upadhyay wrote:
> During Fadump in Qemu VM, when maxcpus value is set to more than current
> cpus, following failure is observed.
>
>      [0.000000] rtas: Dump taken by platform is incomplete (-1)
>
> This is because the CPU_STATE_DATA is allocated for maxcpus, while the
> data is only filled for current cpus. As per current implementation
> of Fadump, dumped_bytes and source_len for a region have to match,
> Which is failing for qemu's case, Even tough the dumped bytes are
> reported correctly for only the current cpus. After this /proc/vmcore
> generatition also fails.
>
> Allowing dumped_bytes to be lesser than or equal to allocated length, for
> CPU_STATE_DATA Fadump region.

The overall logic looks good to me. And as per PAPR, firmware only 
promises to
send CPU data of online CPUs, so it is OK to accept bytes dumped less 
than the
source_len for CPU_STATE_DATA.

>
> Reported-by: Anushree Mathur <anushree.mathur@linux.vnet.ibm.com>
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> ---
> ChangeLog:
>
> v2: Only allowing lesser size for CPU_STATE_DATE Fadump region.
>
> v1: https://lore.kernel.org/qemu-devel/20260429065127.366813-1-shivangu@linux.ibm.com/
> ---
>   arch/powerpc/platforms/pseries/rtas-fadump.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/rtas-fadump.c b/arch/powerpc/platforms/pseries/rtas-fadump.c
> index 3bb4ac2ab6cc..ea1a0a8ac9eb 100644
> --- a/arch/powerpc/platforms/pseries/rtas-fadump.c
> +++ b/arch/powerpc/platforms/pseries/rtas-fadump.c
> @@ -459,7 +459,10 @@ static int __init rtas_fadump_process(struct fw_dump *fadump_conf)
>   	/* Check if the dump data is valid. */
>   	for (int i = 0; i < be16_to_cpu(fdm_active->header.dump_num_sections); i++) {
>   		int type = be16_to_cpu(fdm_active->rgn[i].source_data_type);
> +		uint64_t bytes_dumped = be64_to_cpu(fdm_active->rgn[i].bytes_dumped);
> +		uint64_t source_len = be64_to_cpu(fdm_active->rgn[i].source_len);
>   		int rc = 0;
> +		int region_collected;
>   
>   		switch (type) {
>   		case RTAS_FADUMP_CPU_STATE_DATA:
> @@ -469,8 +472,18 @@ static int __init rtas_fadump_process(struct fw_dump *fadump_conf)
>   				pr_err("Dump taken by platform is not valid (%d)\n", i);
>   				rc = -EINVAL;
>   			}
> -			if (fdm_active->rgn[i].bytes_dumped != fdm_active->rgn[i].source_len) {
> +			/*
> +			 * Make sure that dump is collected for entire region.
> +			 * CPU_STATE_DATA region is allowed to dump less than allocated space.
> +			 */
> +			region_collected = (bytes_dumped == source_len ||
> +	                         (type == RTAS_FADUMP_CPU_STATE_DATA && bytes_dumped <= source_len));

Do we really need region_collected variable? Can't we manage with rc only?

Is bytes_dumped < source_len is good enough instead of <=. There are a 
couple of warnings/errors reported by the checkpatch script.
Please address them in the next version.

- Sourabh Jain


> +
> +
> +			if (! region_collected) {
>   				pr_err("Dump taken by platform is incomplete (%d)\n", i);
> +				pr_debug("type -> %d, bytes_dumped -> %llx, source_len -> %llx\n",
> +				         type, bytes_dumped, source_len);
>   				rc = -EINVAL;
>   			}
>   			if (rc) {
> @@ -482,7 +495,7 @@ static int __init rtas_fadump_process(struct fw_dump *fadump_conf)
>   			}
>   			break;
>   		case RTAS_FADUMP_PARAM_AREA:
> -			if (fdm_active->rgn[i].bytes_dumped != fdm_active->rgn[i].source_len ||
> +			if (bytes_dumped != source_len ||
>   			    fdm_active->rgn[i].error_flags != 0) {
>   				pr_warn("Failed to process additional parameters! Proceeding anyway..\n");
>   				fadump_conf->param_area = 0;

Re: [PATCH v2] ppc/fadump: collect dump if the collected size is lesser than reserved

[Shivang Upadhyay]

On Thu, 2026-08-27 at 11:05 +0530, Sourabh Jain wrote:
> 
> > bytes_dumped <= source_len));
> 
> Do we really need region_collected variable? Can't we manage with rc
> only?
> 
> Is bytes_dumped < source_len is good enough instead of <=. There are
> a 
> couple of warnings/errors reported by the checkpatch script.
> Please address them in the next version.
> 
> - Sourabh Jain
> 
Hi Sourabh, 

bytes_dumped < source_len should also be correct, but "<=" is more
expressive. Why not leave such optimizations to compiler.

For the region_collected, RC should be used to manage the return code,
so I as per me it is more appropriate to use new variable here. Or
maybe, type the condition in the bracket itself(?).

Thanks for pointing out the checkpatch bugs, I'll send a revision soon.

~Shivang.

Re: [PATCH v2] ppc/fadump: collect dump if the collected size is lesser than reserved

[Sourabh Jain]

On 27/08/26 12:06, Shivang Upadhyay wrote:
> On Thu, 2026-08-27 at 11:05 +0530, Sourabh Jain wrote:
>>> bytes_dumped <= source_len));
>> Do we really need region_collected variable? Can't we manage with rc
>> only?
>>
>> Is bytes_dumped < source_len is good enough instead of <=. There are
>> a
>> couple of warnings/errors reported by the checkpatch script.
>> Please address them in the next version.
>>
>> - Sourabh Jain
>>
> Hi Sourabh,
>
> bytes_dumped < source_len should also be correct, but "<=" is more
> expressive. Why not leave such optimizations to compiler.
>
> For the region_collected, RC should be used to manage the return code,
> so I as per me it is more appropriate to use new variable here. Or
> maybe, type the condition in the bracket itself(?).

Yes this also works.

- Sourabh Jain

>
> Thanks for pointing out the checkpatch bugs, I'll send a revision soon.
>
> ~Shivang.