While the first kernel boots, it registers memory regions for fadump
such as:
    * CPU state data  (has to be populated by the platform)
    * HPTE state data (has to be populated by the platform)
    * Real Mode Regions (platform should copy it to requested
      destination addresses)
    * OS defined regions (such as parameter save area)

Platform is also expected to modify the 'bytes_dumped' to the length of
data preserved/copied by platform (ideally same as the source length
passed by kernel).

The kernel passes source address and length for the memory regions, and
a destination address to where the memory is to be copied.

Implement the preserving/copying of the Real Mode Regions and the
Parameter Save Area in QEMU Pseries

Note: As of this patch, the "kernel-dump" device tree entry is still not
added for the second boot, so after crash, the second kernel will boot
assuming fadump dump is "NOT" active, and try to register for fadump,
but since we already have fadump registered in QEMU internal state, the
register rtas call will fail with: "DUMP ACTIVE"

Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 hw/ppc/spapr_fadump.c         | 161 ++++++++++++++++++++++++++++++++--
 include/hw/ppc/spapr_fadump.h |  18 ++++
 2 files changed, 174 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index fedd2cde9a4f..c105b8d21da5 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -123,14 +123,165 @@ uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
     return RTAS_OUT_SUCCESS;
 }
 
+/*
+ * Copy the source region of given fadump section, to the destination
+ * address mentioned in the region
+ *
+ * Also set the region's error flag, if the copy fails due to non-existent
+ * address (MEMTX_DECODE_ERROR) or permission issues (MEMTX_ACCESS_ERROR)
+ *
+ * Returns true if successful copy
+ *
+ * Returns false in case of any other error, being treated as hardware
+ * error for fadump purposes
+ */
+static bool do_preserve_region(FadumpSection *region)
+{
+    AddressSpace *default_as = &address_space_memory;
+    MemTxResult io_result;
+    MemTxAttrs attrs;
+    uint64_t src_addr, src_len, dest_addr;
+    g_autofree void *copy_buffer = NULL;
+
+    src_addr  = be64_to_cpu(region->source_address);
+    src_len   = be64_to_cpu(region->source_len);
+    dest_addr = be64_to_cpu(region->destination_address);
+
+    /* Mark the memory transaction as privileged memory access */
+    attrs.user = 0;
+    attrs.memory = 1;
+
+    /*
+     * Optimisation: Skip copy if source and destination are same
+     * (eg. param area)
+     */
+    if (src_addr != dest_addr) {
+        /*
+         * Using 'g_try_malloc' as the source length is coming from kernel,
+         * which can be invalid/huge, due to which allocation can fail
+         */
+        copy_buffer = g_try_malloc(src_len + 1);
+        if (copy_buffer == NULL) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "FADump: Failed allocating memory (size: %ld) for copying"
+                " reserved memory regions\n", src_len);
+
+            return false;
+        }
+
+        /* Copy the source region to destination */
+        io_result = address_space_read(default_as, src_addr, attrs,
+                copy_buffer, src_len);
+        if ((io_result & MEMTX_DECODE_ERROR) ||
+            (io_result & MEMTX_ACCESS_ERROR)) {
+            /*
+             * Invalid source address is not an hardware error, instead
+             * wrong parameter from the kernel.
+             * Return true to let caller know to continue reading other
+             * sections
+             */
+            region->error_flags = FADUMP_ERROR_INVALID_SOURCE_ADDR;
+            region->bytes_dumped = 0;
+            return true;
+        } else if (io_result != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "FADump: Failed to read source region in section: %d\n",
+                region->source_data_type);
+
+            return false;
+        }
+
+        io_result = address_space_write(default_as, dest_addr, attrs,
+                copy_buffer, src_len);
+        if ((io_result & MEMTX_DECODE_ERROR) ||
+            (io_result & MEMTX_ACCESS_ERROR)) {
+            /*
+             * Invalid destination address is not an hardware error,
+             * instead wrong parameter from the kernel.
+             * Return true to let caller know to continue reading other
+             * sections
+             */
+            region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
+            region->bytes_dumped = 0;
+            return true;
+        } else if (io_result != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "FADump: Failed to write destination in section: %d\n",
+                region->source_data_type);
+
+            return false;
+        }
+    }
+
+    /*
+     * Considering address_space_write would have copied the
+     * complete region
+     */
+    region->bytes_dumped = cpu_to_be64(src_len);
+    return true;
+}
+
 /* Preserve the memory locations registered for fadump */
-static bool fadump_preserve_mem(void)
+static bool fadump_preserve_mem(SpaprMachineState *spapr)
 {
+    FadumpMemStruct *fdm = &spapr->registered_fdm;
+    uint16_t dump_num_sections, data_type;
+
+    assert(spapr->fadump_registered);
+
     /*
-     * TODO: Implement preserving memory regions requested during fadump
-     * registration
+     * Handle all sections
+     *
+     * CPU State Data and HPTE regions are handled in their own cases
+     *
+     * RMR regions and any custom OS reserved regions such as parameter
+     * save area, are handled by simply copying the source region to
+     * destination address
      */
-    return false;
+    dump_num_sections = be16_to_cpu(fdm->header.dump_num_sections);
+    for (int i = 0; i < dump_num_sections; ++i) {
+        data_type = be16_to_cpu(fdm->rgn[i].source_data_type);
+
+        /* Reset error_flags & bytes_dumped for now */
+        fdm->rgn[i].error_flags = 0;
+        fdm->rgn[i].bytes_dumped = 0;
+
+        /* If kernel did not request for the memory region, then skip it */
+        if (be32_to_cpu(fdm->rgn[i].request_flag) != FADUMP_REQUEST_FLAG) {
+            qemu_log_mask(LOG_UNIMP,
+                "FADump: Skipping copying region as not requested\n");
+            continue;
+        }
+
+        switch (data_type) {
+        case FADUMP_CPU_STATE_DATA:
+            /* TODO: Add CPU state data */
+            break;
+        case FADUMP_HPTE_REGION:
+            /* TODO: Add hpte state data */
+            break;
+        case FADUMP_REAL_MODE_REGION:
+        case FADUMP_PARAM_AREA:
+            /* Copy the memory region from region's source to its destination */
+            if (!do_preserve_region(&fdm->rgn[i])) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                    "FADump: Failed to preserve dump section: %d\n",
+                    be16_to_cpu(fdm->rgn[i].source_data_type));
+                fdm->header.dump_status_flag |=
+                    cpu_to_be16(FADUMP_STATUS_DUMP_ERROR);
+            }
+
+            break;
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "FADump: Skipping unknown source data type: %d\n", data_type);
+
+            fdm->rgn[i].error_flags =
+                cpu_to_be16(FADUMP_ERROR_INVALID_DATA_TYPE);
+        }
+    }
+
+    return true;
 }
 
 /*
@@ -151,7 +302,7 @@ void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
     pause_all_vcpus();
 
     /* Preserve the memory locations registered for fadump */
-    if (!fadump_preserve_mem()) {
+    if (!fadump_preserve_mem(spapr)) {
         /* Failed to preserve the registered memory regions */
         rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
 
diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
index e484604c1c70..d56ca1d6d651 100644
--- a/include/hw/ppc/spapr_fadump.h
+++ b/include/hw/ppc/spapr_fadump.h
@@ -16,11 +16,29 @@
 
 #define FADUMP_VERSION                 1
 
+/* Firmware provided dump sections */
+#define FADUMP_CPU_STATE_DATA   0x0001
+#define FADUMP_HPTE_REGION      0x0002
+#define FADUMP_REAL_MODE_REGION 0x0011
+
+/* OS defined sections */
+#define FADUMP_PARAM_AREA       0x0100
+
+/* Dump request flag */
+#define FADUMP_REQUEST_FLAG     0x00000001
+
 /* Dump status flags */
 #define FADUMP_STATUS_DUMP_PERFORMED            0x8000
 #define FADUMP_STATUS_DUMP_TRIGGERED            0x4000
 #define FADUMP_STATUS_DUMP_ERROR                0x2000
 
+/* Region dump error flags */
+#define FADUMP_ERROR_INVALID_DATA_TYPE          0x8000
+#define FADUMP_ERROR_INVALID_SOURCE_ADDR        0x4000
+#define FADUMP_ERROR_LENGTH_EXCEEDS_SOURCE      0x2000
+#define FADUMP_ERROR_INVALID_DEST_ADDR          0x1000
+#define FAUDMP_ERROR_DEST_TOO_SMALL             0x0800
+
 /*
  * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
  * in the dump memory structure. Presently, three sections are used for
-- 
2.49.0

On 23/03/25 23:10, Aditya Gupta wrote:
> While the first kernel boots, it registers memory regions for fadump
> such as:
>      * CPU state data  (has to be populated by the platform)
>      * HPTE state data (has to be populated by the platform)
>      * Real Mode Regions (platform should copy it to requested
>        destination addresses)
>      * OS defined regions (such as parameter save area)
>
> Platform is also expected to modify the 'bytes_dumped' to the length of
> data preserved/copied by platform (ideally same as the source length
> passed by kernel).
>
> The kernel passes source address and length for the memory regions, and
> a destination address to where the memory is to be copied.
>
> Implement the preserving/copying of the Real Mode Regions and the
> Parameter Save Area in QEMU Pseries
>
> Note: As of this patch, the "kernel-dump" device tree entry is still not
> added for the second boot, so after crash, the second kernel will boot
> assuming fadump dump is "NOT" active, and try to register for fadump,
> but since we already have fadump registered in QEMU internal state, the
> register rtas call will fail with: "DUMP ACTIVE"
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>   hw/ppc/spapr_fadump.c         | 161 ++++++++++++++++++++++++++++++++--
>   include/hw/ppc/spapr_fadump.h |  18 ++++
>   2 files changed, 174 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index fedd2cde9a4f..c105b8d21da5 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -123,14 +123,165 @@ uint32_t do_fadump_register(SpaprMachineState *spapr, target_ulong args)
>       return RTAS_OUT_SUCCESS;
>   }
>   
> +/*
> + * Copy the source region of given fadump section, to the destination
> + * address mentioned in the region
> + *
> + * Also set the region's error flag, if the copy fails due to non-existent
> + * address (MEMTX_DECODE_ERROR) or permission issues (MEMTX_ACCESS_ERROR)
> + *
> + * Returns true if successful copy
> + *
> + * Returns false in case of any other error, being treated as hardware
> + * error for fadump purposes
> + */
> +static bool do_preserve_region(FadumpSection *region)
> +{
> +    AddressSpace *default_as = &address_space_memory;
> +    MemTxResult io_result;
> +    MemTxAttrs attrs;
> +    uint64_t src_addr, src_len, dest_addr;
> +    g_autofree void *copy_buffer = NULL;
> +
> +    src_addr  = be64_to_cpu(region->source_address);
> +    src_len   = be64_to_cpu(region->source_len);
> +    dest_addr = be64_to_cpu(region->destination_address);
> +
> +    /* Mark the memory transaction as privileged memory access */
> +    attrs.user = 0;
> +    attrs.memory = 1;
> +
> +    /*
> +     * Optimisation: Skip copy if source and destination are same
> +     * (eg. param area)
> +     */
> +    if (src_addr != dest_addr) {
> +        /*
> +         * Using 'g_try_malloc' as the source length is coming from kernel,
> +         * which can be invalid/huge, due to which allocation can fail
> +         */
> +        copy_buffer = g_try_malloc(src_len + 1);

The region size could be in GBs. I think it is better to do it in chunks.

And don't we need to free the copy_buffer?

> +        if (copy_buffer == NULL) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADump: Failed allocating memory (size: %ld) for copying"
> +                " reserved memory regions\n", src_len);
> +
> +            return false;
> +        }
> +
> +        /* Copy the source region to destination */
> +        io_result = address_space_read(default_as, src_addr, attrs,
> +                copy_buffer, src_len);
> +        if ((io_result & MEMTX_DECODE_ERROR) ||
> +            (io_result & MEMTX_ACCESS_ERROR)) {
> +            /*
> +             * Invalid source address is not an hardware error, instead
> +             * wrong parameter from the kernel.
> +             * Return true to let caller know to continue reading other
> +             * sections
> +             */
> +            region->error_flags = FADUMP_ERROR_INVALID_SOURCE_ADDR;
> +            region->bytes_dumped = 0;
> +            return true;
> +        } else if (io_result != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADump: Failed to read source region in section: %d\n",
> +                region->source_data_type);
> +
> +            return false;
> +        }
> +
> +        io_result = address_space_write(default_as, dest_addr, attrs,
> +                copy_buffer, src_len);
> +        if ((io_result & MEMTX_DECODE_ERROR) ||
> +            (io_result & MEMTX_ACCESS_ERROR)) {
> +            /*
> +             * Invalid destination address is not an hardware error,
> +             * instead wrong parameter from the kernel.
> +             * Return true to let caller know to continue reading other
> +             * sections

Logging MEMTX_DECODE_ERROR would help identify kernel bugs. I think
we should log this for both read and write.

> +             */
> +            region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
> +            region->bytes_dumped = 0;

Seems like caller is already setting bytes_dump to 0. But even if you 
wanna do here.
How about setting region->bytes_dumped = 0 early to avoid setting at 
multiple
places?

In cases you need to free the copy_buffer I recommend having an exit label
in this function.

> +            return true;
> +        } else if (io_result != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADump: Failed to write destination in section: %d\n",
> +                region->source_data_type);
> +
> +            return false;
> +        }
> +    }
> +
> +    /*
> +     * Considering address_space_write would have copied the
> +     * complete region
> +     */
> +    region->bytes_dumped = cpu_to_be64(src_len);
> +    return true;
> +}
> +
>   /* Preserve the memory locations registered for fadump */
> -static bool fadump_preserve_mem(void)
> +static bool fadump_preserve_mem(SpaprMachineState *spapr)
>   {
> +    FadumpMemStruct *fdm = &spapr->registered_fdm;
> +    uint16_t dump_num_sections, data_type;
> +
> +    assert(spapr->fadump_registered);
> +
>       /*
> -     * TODO: Implement preserving memory regions requested during fadump
> -     * registration
> +     * Handle all sections
> +     *
> +     * CPU State Data and HPTE regions are handled in their own cases
> +     *
> +     * RMR regions and any custom OS reserved regions such as parameter
> +     * save area, are handled by simply copying the source region to
> +     * destination address
>        */
> -    return false;
> +    dump_num_sections = be16_to_cpu(fdm->header.dump_num_sections);
> +    for (int i = 0; i < dump_num_sections; ++i) {
> +        data_type = be16_to_cpu(fdm->rgn[i].source_data_type);
> +
> +        /* Reset error_flags & bytes_dumped for now */
> +        fdm->rgn[i].error_flags = 0;
> +        fdm->rgn[i].bytes_dumped = 0;
> +
> +        /* If kernel did not request for the memory region, then skip it */
> +        if (be32_to_cpu(fdm->rgn[i].request_flag) != FADUMP_REQUEST_FLAG) {
> +            qemu_log_mask(LOG_UNIMP,
> +                "FADump: Skipping copying region as not requested\n");
> +            continue;
> +        }
> +
> +        switch (data_type) {
> +        case FADUMP_CPU_STATE_DATA:
> +            /* TODO: Add CPU state data */
> +            break;
> +        case FADUMP_HPTE_REGION:
> +            /* TODO: Add hpte state data */
> +            break;
> +        case FADUMP_REAL_MODE_REGION:
> +        case FADUMP_PARAM_AREA:
> +            /* Copy the memory region from region's source to its destination */
> +            if (!do_preserve_region(&fdm->rgn[i])) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                    "FADump: Failed to preserve dump section: %d\n",
> +                    be16_to_cpu(fdm->rgn[i].source_data_type));
> +                fdm->header.dump_status_flag |=
> +                    cpu_to_be16(FADUMP_STATUS_DUMP_ERROR);
> +            }
> +
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADump: Skipping unknown source data type: %d\n", data_type);
> +
> +            fdm->rgn[i].error_flags =
> +                cpu_to_be16(FADUMP_ERROR_INVALID_DATA_TYPE);
> +        }
> +    }
> +
> +    return true;

This function only returns true. Since caller has some action when this 
function
returns false I am considering there is something wrong in this function.

If this suppose to return true always then lets not have return at all.

>   }
>   
>   /*
> @@ -151,7 +302,7 @@ void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
>       pause_all_vcpus();
>   
>       /* Preserve the memory locations registered for fadump */
> -    if (!fadump_preserve_mem()) {
> +    if (!fadump_preserve_mem(spapr)) {
>           /* Failed to preserve the registered memory regions */
>           rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
>   
> diff --git a/include/hw/ppc/spapr_fadump.h b/include/hw/ppc/spapr_fadump.h
> index e484604c1c70..d56ca1d6d651 100644
> --- a/include/hw/ppc/spapr_fadump.h
> +++ b/include/hw/ppc/spapr_fadump.h
> @@ -16,11 +16,29 @@
>   
>   #define FADUMP_VERSION                 1
>   
> +/* Firmware provided dump sections */
> +#define FADUMP_CPU_STATE_DATA   0x0001
> +#define FADUMP_HPTE_REGION      0x0002
> +#define FADUMP_REAL_MODE_REGION 0x0011
> +
> +/* OS defined sections */
> +#define FADUMP_PARAM_AREA       0x0100
> +
> +/* Dump request flag */
> +#define FADUMP_REQUEST_FLAG     0x00000001
> +
>   /* Dump status flags */
>   #define FADUMP_STATUS_DUMP_PERFORMED            0x8000
>   #define FADUMP_STATUS_DUMP_TRIGGERED            0x4000
>   #define FADUMP_STATUS_DUMP_ERROR                0x2000
>   
> +/* Region dump error flags */
> +#define FADUMP_ERROR_INVALID_DATA_TYPE          0x8000
> +#define FADUMP_ERROR_INVALID_SOURCE_ADDR        0x4000
> +#define FADUMP_ERROR_LENGTH_EXCEEDS_SOURCE      0x2000
> +#define FADUMP_ERROR_INVALID_DEST_ADDR          0x1000
> +#define FAUDMP_ERROR_DEST_TOO_SMALL             0x0800
> +
>   /*
>    * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections
>    * in the dump memory structure. Presently, three sections are used for

On 25/10/17 06:36PM, Sourabh Jain wrote:
> 
> 
> On 23/03/25 23:10, Aditya Gupta wrote:
> > <...snip...>
> > +    /*
> > +     * Optimisation: Skip copy if source and destination are same
> > +     * (eg. param area)
> > +     */
> > +    if (src_addr != dest_addr) {
> > +        /*
> > +         * Using 'g_try_malloc' as the source length is coming from kernel,
> > +         * which can be invalid/huge, due to which allocation can fail
> > +         */
> > +        copy_buffer = g_try_malloc(src_len + 1);
> 
> The region size could be in GBs. I think it is better to do it in chunks.

Sure Sourabh, will do in chunks of 32MB in v5.

> 
> And don't we need to free the copy_buffer?

No, since I declare it as 'g_autofree', so it's freed automatically at
all exits.

> 
> > +        if (copy_buffer == NULL) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                "FADump: Failed allocating memory (size: %ld) for copying"
> > +                " reserved memory regions\n", src_len);
> > +
> > +            return false;
> > +        }
> > +
> > +        /* Copy the source region to destination */
> > +        io_result = address_space_read(default_as, src_addr, attrs,
> > +                copy_buffer, src_len);
> > +        if ((io_result & MEMTX_DECODE_ERROR) ||
> > +            (io_result & MEMTX_ACCESS_ERROR)) {
> > +            /*
> > +             * Invalid source address is not an hardware error, instead
> > +             * wrong parameter from the kernel.
> > +             * Return true to let caller know to continue reading other
> > +             * sections
> > +             */
> > +            region->error_flags = FADUMP_ERROR_INVALID_SOURCE_ADDR;
> > +            region->bytes_dumped = 0;
> > +            return true;
> > +        } else if (io_result != MEMTX_OK) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                "FADump: Failed to read source region in section: %d\n",
> > +                region->source_data_type);
> > +
> > +            return false;
> > +        }
> > +
> > +        io_result = address_space_write(default_as, dest_addr, attrs,
> > +                copy_buffer, src_len);
> > +        if ((io_result & MEMTX_DECODE_ERROR) ||
> > +            (io_result & MEMTX_ACCESS_ERROR)) {
> > +            /*
> > +             * Invalid destination address is not an hardware error,
> > +             * instead wrong parameter from the kernel.
> > +             * Return true to let caller know to continue reading other
> > +             * sections
> 
> Logging MEMTX_DECODE_ERROR would help identify kernel bugs. I think
> we should log this for both read and write.

Makes sense. Will do.

> 
> > +             */
> > +            region->error_flags = FADUMP_ERROR_INVALID_DEST_ADDR;
> > +            region->bytes_dumped = 0;
> 
> Seems like caller is already setting bytes_dump to 0. But even if you wanna
> do here.
> How about setting region->bytes_dumped = 0 early to avoid setting at
> multiple
> places?
> 
> In cases you need to free the copy_buffer I recommend having an exit label
> in this function.

As we declare it as 'g_autofree', 'g_free' will automatically get
invoked when the variable goes out of scope.

> 
> > +            return true;
> > +        } else if (io_result != MEMTX_OK) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                "FADump: Failed to write destination in section: %d\n",
> > +                region->source_data_type);
> > +
> > +            return false;
> > +        }
> > +    }
> > +
> > <...snip...>
> > +            fdm->rgn[i].error_flags =
> > +                cpu_to_be16(FADUMP_ERROR_INVALID_DATA_TYPE);
> > +        }
> > +    }
> > +
> > +    return true;
> 
> This function only returns true. Since caller has some action when this
> function
> returns false I am considering there is something wrong in this function.
> 
> If this suppose to return true always then lets not have return at all.

Agreed return type is of no use as of this patch. The return type being
bool is needed when CPU register saving is implemented, where we return
false, signalling a HW error that we failed to save registers.

Thanks,
- Aditya G