On Thu, 23 Oct 2025 at 13:23, Harsh Prateek Bora <[email protected]> wrote:
>
> From: Aditya Gupta <[email protected]>
>
> 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,
> + ®_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
