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 ?
>
> > + * 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.
>
> > <...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 ?
Thanks for your reviews Sourabh,
- Aditya G
