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

[Aditya Gupta <adityag@linux.ibm.com>]

On 05/03/25 12:10, Harsh Prateek Bora wrote:

>
>
> On 2/17/25 12:47, Aditya Gupta wrote:
>> <...snip...>
>>
>> +        /* Reset error_flags & bytes_dumped for now */
>> +        fdm->rgn[i].error_flags = 0;
>> +        fdm->rgn[i].bytes_dumped = 0;
>> +
>> +        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:
>> +            /* Skip copy if source and destination are same (eg. 
>> param area) */
>> +            if (src_addr != dest_addr) {
>> +                copy_buffer = g_malloc(src_len + 1);
>> +                if (copy_buffer == NULL) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                        "FADUMP: Failed allocating memory for 
>> copying reserved memory regions\n");
>> +                    fdm->rgn[i].error_flags =
>> + cpu_to_be16(FADUMP_ERROR_LENGTH_EXCEEDS_SOURCE);
>> +
>> +                    continue;
>> +                }
>> +
>> +                /* Copy the source region to destination */
>> +                cpu_physical_memory_read(src_addr, copy_buffer, 
>> src_len);
>> +                cpu_physical_memory_write(dest_addr, copy_buffer, 
>> src_len);
>> +                g_free(copy_buffer);
>> +            }
>> +
>> +            /*
>> +             * Considering cpu_physical_memory_write would have 
>> copied the
>> +             * complete region
>> +             */
>> +            fdm->rgn[i].bytes_dumped = cpu_to_be64(src_len);
>
> Is this really valid for FADUMP_PARAM_AREA where we intend to skip copy?
>
Yes I think it's good to keep it. Because that's an optimisation i did 
to skip the copy if src and dest are same.

But the actual copy depends on the OS passing us the 
"FADUMP_REQUEST_FLAG" in the region's request flag.

If the flag is set, i am expecting the kernel asked us to copy, and 
hence the .bytes_dumped should be same as the number of bytes asked by 
kernel to copy ideally.

If the flag is not set, we return early, so we let the .bytes_dumped be 0.

>> +
>> +            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;
>> +}
>> +
>>   static void trigger_fadump_boot(target_ulong spapr_retcode)
>>   {
>>       /*
>> @@ -353,7 +467,8 @@ static void trigger_fadump_boot(target_ulong 
>> spapr_retcode)
>>        */
>>       pause_all_vcpus();
>>   -    if (true /* TODO: Preserve memory registered for fadump */) {
>> +    /* Preserve the memory locations registered for fadump */
>> +    if (!fadump_preserve_mem()) {
>
> This change can be avoided as suggested in previous patch.
Agreed, will do it in previous patch.
>
>>           /* Failed to preserve the registered memory regions */
>>           rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
>>   diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index efa2f891a8a7..a80704187583 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -776,7 +776,32 @@ void push_sregs_to_kvm_pr(SpaprMachineState 
>> *spapr);
>>   #define FADUMP_CMD_UNREGISTER          2
>>   #define FADUMP_CMD_INVALIDATE          3
>>   -#define FADUMP_VERSION    1
>> +#define FADUMP_VERSION                 1
>
> This change can be avoided if taken care initially.

Nice, I missed this diff. Will fix it in the patch which introduced this.


Thanks,

- Aditya Gupta

>
> Thanks
> Harsh
>> +
>> +/*
>> + * The Firmware Assisted Dump Memory structure supports a maximum of 
>> 10 sections
>> + * in the dump memory structure. Presently, three sections are used for
>> + * CPU state data, HPTE & Parameters area, while the remaining seven 
>> sections
>> + * can be used for boot memory regions.
>> + */
>> +#define FADUMP_MAX_SECTIONS            10
>> +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS  7
>> +
>> +/* 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 flag */
>> +#define FADUMP_ERROR_INVALID_DATA_TYPE          0x8000
>> +#define FADUMP_ERROR_INVALID_SOURCE_ADDR        0x4000
>> +#define FADUMP_ERROR_LENGTH_EXCEEDS_SOURCE      0x2000
>>     /*
>>    * The Firmware Assisted Dump Memory structure supports a maximum 
>> of 10 sections