Re: [PULL 28/32] hw/ppc: Implement saving CPU state in Fadump

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 23 Oct 2025 at 13:23, Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>
> From: Aditya Gupta <adityag@linux.ibm.com>
>
> Kernel expects CPU states/register states in the format mentioned in
> "Register Save Area" in PAPR.
>
> The platform (in our case, QEMU) saves each CPU register in the form of
> an array of "register entries", the start and end of this array is
> signified by "CPUSTRT" and "CPUEND" register entries respectively.
>
> The CPUSTRT and CPUEND register entry also has 4-byte logical CPU ID,
> thus storing the CPU ID corresponding to the array of register entries.
>
> Each register, and CPUSTRT, CPUEND has a predefined identifier.
> Implement calculating identifier for a given register in
> 'fadump_str_to_u64', which has been taken from the linux kernel
>
> Similarly GPRs also have predefined identifiers, and a corresponding
> 64-bit resiter value (split into two 32-bit cells). Implement
> calculation of GPR identifiers with 'fadump_gpr_id_to_u64'
>
> PAPR has restrictions on particular order of few registers, and is
> free to be in any order for other registers.
> Some registers mentioned in PAPR have not been exported as they are not
> implemented in QEMU / don't make sense in QEMU.
>
> Implement saving of CPU state according to the PAPR document

Hi; Coverity points out what looks like a memory leak in this
code (CID 1642024):

> +static void *get_cpu_state_data(uint64_t *cpu_state_len)
> +{
> +    FadumpRegSaveAreaHeader reg_save_hdr;
> +    FadumpRegEntry *reg_entries;
> +    FadumpRegEntry *curr_reg_entry;
> +    CPUState *cpu;
> +
> +    uint32_t num_reg_entries;
> +    uint32_t reg_entries_size;
> +    uint32_t num_cpus = 0;
> +
> +    void *cpu_state_buffer = NULL;
> +    uint64_t offset = 0;
> +
> +    CPU_FOREACH(cpu) {
> +        ++num_cpus;
> +    }
> +
> +    reg_save_hdr.version = cpu_to_be32(0);
> +    reg_save_hdr.magic_number =
> +        cpu_to_be64(fadump_str_to_u64("REGSAVE"));
> +
> +    /* Reg save area header is immediately followed by num cpus */
> +    reg_save_hdr.num_cpu_offset =
> +        cpu_to_be32(sizeof(FadumpRegSaveAreaHeader));
> +
> +    num_reg_entries = num_cpus * FADUMP_PER_CPU_REG_ENTRIES;
> +    reg_entries_size = num_reg_entries * sizeof(FadumpRegEntry);
> +
> +    reg_entries = g_new(FadumpRegEntry, num_reg_entries);

Here we allocate memory into reg_entries...

> +
> +    /* Pointer to current CPU's registers */
> +    curr_reg_entry = reg_entries;
> +
> +    /* Populate register entries for all CPUs */
> +    CPU_FOREACH(cpu) {
> +        cpu_synchronize_state(cpu);
> +        curr_reg_entry = populate_cpu_reg_entries(cpu, curr_reg_entry);
> +    }

...then we populate reg_entries with data...

> +
> +    *cpu_state_len = 0;
> +    *cpu_state_len += sizeof(reg_save_hdr);     /* reg save header */
> +    *cpu_state_len += 0xc;                      /* padding as in PAPR */
> +    *cpu_state_len += sizeof(__be32);           /* num_cpus */
> +    *cpu_state_len += reg_entries_size;         /* reg entries */
> +
> +    cpu_state_buffer = g_malloc(*cpu_state_len);
> +
> +    memcpy(cpu_state_buffer + offset,
> +            &reg_save_hdr, sizeof(reg_save_hdr));
> +    offset += sizeof(reg_save_hdr);
> +
> +    /* Write num_cpus */
> +    num_cpus = cpu_to_be32(num_cpus);
> +    memcpy(cpu_state_buffer + offset, &num_cpus, sizeof(__be32));
> +    offset += sizeof(__be32);
> +
> +    /* Write the register entries */
> +    memcpy(cpu_state_buffer + offset, reg_entries, reg_entries_size);
> +    offset += reg_entries_size;

...and then we eventually copy the reg_entries data into
the cpu_state_buffer. But then we never free reg_entries.

g_autofree would fix this.

> +
> +    return cpu_state_buffer;
> +}

thanks
-- PMM