Re: [PATCH v5 4/8] hw/ppc: Implement saving CPU state in Fadump

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 10/23/25 16:46, Sourabh Jain wrote:
> 
> 
> On 23/10/25 16:41, Aditya Gupta wrote:
>> On 25/10/23 02:35PM, Sourabh Jain wrote:
>>>
>>>> <...snip...>
>>>> +    /*
>>>> +     * CPUSTRT and CPUEND register entries follow this format:
>>>> +     *
>>>> +     * 8 Bytes Reg ID (BE) | 4 Bytes (0x0) | 4 Bytes Logical CPU ID 
>>>> (BE)
>>>> +     */
>>>> +    curr_reg_entry->reg_id =
>>>> +        cpu_to_be64(fadump_str_to_u64("CPUSTRT"));
>>>> +    curr_reg_entry->reg_value = cpu_to_be64(
>>>> +            ppc_cpu->vcpu_id & FADUMP_CPU_ID_MASK);
>>> Seems like converting full 64 bit CPU ID to 64 bit BE value will not 
>>> bring
>>> reg
>>> entry in below format. Isn't it?
>>>
>>> 8 Bytes Identifier (BE) | 4 Bytes Reserved (0x0) | 4 Bytes Logical 
>>> CPU ID
>>> (BE)
>>>
>>>> <...snip...>
>>>> +    /* End the registers for this CPU with "CPUEND" reg entry */
>>>> +    curr_reg_entry->reg_id =
>>>> +        cpu_to_be64(fadump_str_to_u64("CPUEND"));
>>>> +    curr_reg_entry->reg_value = cpu_to_be64(
>>>> +            ppc_cpu->vcpu_id & FADUMP_CPU_ID_MASK);
>>> Same here.
>> It will be in the same format, since even with storing 8 bytes at once,
>> we do a 8 byte swap on the CPU ID, thus bringing the cpu id in the
>> higher 4 bytes only (considering CPU ID fits in 4 bytes as ensured by
>> the FADUMP_CPU_ID_MASK).
>>
>> So, it still follows the above format, just that it does not explicitly
>> use 4 byte blocks.
>>
>> This is also consistent with how the Linux kernel reads this field:
>>
>>     /* Lower 4 bytes of reg_value contains logical cpu id */
>>     cpu = (be64_to_cpu(reg_entry->reg_value) &
>>            RTAS_FADUMP_CPU_ID_MASK);
> 
> Yeah looks good to me now. Thanks.
> 
> Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>

Thanks Sourabh and Shivang for your diligent reviews and validation efforts.

Queued.

regards,
Harsh
>