On 20/10/25 00:52, Aditya Gupta wrote:
On 25/10/18 04:24PM, Sourabh Jain wrote:
<...snip...>
+ curr_reg_entry->reg_id =
+ cpu_to_be64(fadump_str_to_u64("CPUSTRT"));
+ curr_reg_entry->reg_value = ppc_cpu->vcpu_id;
Aren't we suppose to store CPU ID in big endian?
As per PAPR the format CPUSTRT and CPUEND node is
8 Bytes Identifier (BE) | 4 Bytes Reserved (0x0) | 4 Bytes Logical CPU ID
(BE)
It feels like storing vcpu_id as little endian may not get the
reg entrie for CPUSTRT in above format, isn't it?
Will fix in v5.
Ideally we should have two struct to mange two types of Reg Entries.
That way we can manage reserved bytes in CPUSTRT/CPUEND node
easily. I understand that it will bring unnecessary complexity in
populate_cpu_state_data function. So how about adding a note for
future reference?
Will add a note about the format.
I also think another struct might be unnecessary complexity since we
just need it at 2 instances (CPUSTRT & CPUEND), and can be done with
simple bit math.
+ ++curr_reg_entry;
+
<...snip...>
+ /* End the registers for this CPU with "CPUEND" reg entry */
+ curr_reg_entry->reg_id =
+ cpu_to_be64(fadump_str_to_u64("CPUEND"));
CPU ID as value to CPUEND reg entry needed, right?
Yes, will add in v5.
+
<...snip...>
+ /*
+ * We will write the cpu state data later, as otherwise it
+ * might get overwritten by other fadump regions
Isn't the destination address of cpu state data and RMR region are
different?
I don't understand the above comment. Can you please give more details.
+ */
+
<...snip...>
+ /*
+ * Write the Register Save Area
+ *
+ * CPU State/Register Save Area should be written after dumping the
+ * memory to prevent overwriting while saving other memory regions
+ *
+ * eg. If boot memory region is 1G, then both the first 1GB memory, and
+ * the Register Save Area needs to be saved at 1GB.
Every region is copied to their destination address and as per below kernel
function:
https://github.com/torvalds/linux/blob/f406055cb18c6e299c4a783fc1effeb16be41803/arch/powerpc/platforms/pseries/rtas-fadump.c#L98
the destination address shouldn't be same for fadump region then how come
there
is overwrite scenario? Am I missing something?
Yes, with current kernel logic, destination address of cpu state data
and RMR region are different.
I remember having faced some issue in past (don't recall, might have
been with powernv, or it might not exist anymore) hence the comment.
Ideally kernel should ensure CPU and memory regions don't overlap.
Possible overlap can be like, memory region being 0x0 - 512MB, and
generally the cpu region's destination also lies in this low memory.
PAPR says nothing about order of saving iirc. I kept current way to
ensure we have CPU registers even if such overlap happens.
What do you say ? Should we save CPU region first and then go with
saving memory regions ?
Yeah, I think we shouldn’t delay the CPU region. Even in the case of
such overlap, corruption in register values is easier to identify and fix
it in the kernel than corruption in the kernel memory dump.
+ * And as the CPU_STATE_DATA region comes first than the
+ * REAL_MODE_REGION region to be copied, the CPU_STATE_DATA will get
+ * overwritten if saved before the 0GB - 1GB region is copied after
+ * saving CPU state data
+ */
+ io_result = address_space_write(default_as, cpu_state_addr, attrs,
+ cpu_state_buffer, cpu_state_len);
Hope cpu_state_buffer will be freed automatically...
Yes, as it's declared with g_autofree, glibc/compiler takes care of
adding cleanup functions ensuring it gets freed.
That's nice.
<...snip...>
+/* Number of registers stored per cpu */
+#define FADUMP_NUM_PER_CPU_REGS (32 /*GPR*/ + 45 /*others*/ + 2 /*STRT & END*/)
#nit-pick
How about FADUMP_PER_CPU_REG_ENTRY ?
How about FADUMP_PER_CPU_NUM_REGS ? Basically explicitly saying it's
number of registers per cpu ?
I suggested FADUMP_PER_CPU_REG_ENTRY because it’s a reg entry count
which include also includes the start and end. But it’s okay, I’m not
picky about it. Feel free to use any of the three. - Sourabh
