Re: [PATCH v5 3/8] hw/ppc: Preserve memory regions registered for fadump

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 21/10/25 19:18, 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
>
> The regions are copied in chunks instead of copying all at once.
>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>   hw/ppc/spapr_fadump.c         | 183 +++++++++++++++++++++++++++++++++-
>   include/hw/ppc/spapr_fadump.h |  18 ++++
>   2 files changed, 196 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index 53e5c12c7641..5068a9d83dce 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -7,7 +7,9 @@
>   #include "qemu/osdep.h"
>   #include "qemu/log.h"
>   #include "hw/ppc/spapr.h"
> +#include "qemu/units.h"
>   #include "system/cpus.h"
> +#include <math.h>
>   
>   /*
>    * Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
> @@ -123,14 +125,185 @@ 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;
> +    uint64_t num_chunks;
> +    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) {
> +        region->bytes_dumped = cpu_to_be64(src_len);
> +        return true;
> +    }
> +
> +#define FADUMP_CHUNK_SIZE  ((size_t)(32 * MiB))
> +    copy_buffer = g_try_malloc(FADUMP_CHUNK_SIZE);
> +    if (copy_buffer == NULL) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "FADump: Failed allocating memory (size: %zu) for copying"
> +            " reserved memory regions\n", FADUMP_CHUNK_SIZE);
> +    }
> +
> +    num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
> +    for (uint64_t chunk_id = 0; chunk_id < num_chunks; ++chunk_id) {
> +        /* Take minimum of bytes left to copy, and chunk size */
> +        uint64_t copy_len = MIN(
> +                        src_len - (chunk_id * FADUMP_CHUNK_SIZE),
> +                        FADUMP_CHUNK_SIZE
> +                    );
> +
> +        /* Copy the source region to destination */
> +        io_result = address_space_read(default_as, src_addr, attrs,
> +                copy_buffer, copy_len);
> +        if ((io_result & MEMTX_DECODE_ERROR) ||
> +            (io_result & MEMTX_ACCESS_ERROR)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADump: Failed to decode/access address in section: %d\n",
> +                region->source_data_type);
> +
> +            /*
> +             * 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, copy_len);
> +        if ((io_result & MEMTX_DECODE_ERROR) ||
> +            (io_result & MEMTX_ACCESS_ERROR)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "FADump: Failed to decode/access address in section: %d\n",
> +                region->source_data_type);
> +
> +            /*
> +             * 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;
> +        }
> +
> +        src_addr += FADUMP_CHUNK_SIZE;
> +        dest_addr += FADUMP_CHUNK_SIZE;
> +    }
> +#undef FADUMP_CHUNK_SIZE
> +
> +    /*
> +     * 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 +324,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 1cb90c9d63af..71be2ad92c11 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

LGTM
Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>