Re: [PATCH v4 12/12] [PowerPC] Add support for HTM registers

[Pedro Alves <palves@redhat.com>]

On 08/15/2018 01:06 AM, Pedro Franco de Carvalho wrote:
> From: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> 
> This patch adds support for Hardware Transactional Memory registers
> for the powerpc linux native and core file targets, and for the
> pwoerpc linux server stub.
> 
> These registers include both the HTM special-purpose registers (TFHAR,
> TEXASR and TFIAR) as well as the set of registers that are
> checkpointed (saved) when a transaction is initiated, which the
> processor restores in the event of a transaction failure.
> 
> The set of checkpointed general-purpose registers is returned by the
> linux kernel in the same format as the regular general-purpose
> registers, defined in struct pt_regs.  However, the architecture
> specifies that only some of the registers present in pt_regs are
> checkpointed (GPRs 0-31, CR, XER, LR and CTR).  The kernel fills the
> slots for other registers with other info (e.g., nip is filled with
> the contents of TFHAR).  GDB doesn't handle these other registers.
> This means that core files generated by GDB will show values of zero
> for these registers, while core files generated by the kernel will
> have other values.  Core files generated by the kernel have a note
> section for checkpointed GPRs with the same size for both 32-bit and
> 64-bit threads, and the values for the registers of a 32-bit thread
> are squeezed in the first half, with no useful data in the second
> half.  GDB generates a smaller note section for 32-bit threads, but
> can read both sizes.

I won't pretend to understand the above fully (not an Power expert),
but the question I ended up with was, after all this, will the
GDB-generated files end up looking like kernel-generated cores?
Or are there plans for that?

> 
> The checkpointed XER is required to be 32-bit in the target
> description documentation, even though the more recent ISAs define it
> as 64-bit wide, since the high-order 32-bits are reserved, and because
> in Linux there is no way to get a 64-bit checkpointed XER for 32-bit
> threads.  If this changes in the future, the target description
> feature requirement can be relaxed to allow for a 64-bit checkpointed
> XER.
> 
> Access to the checkpointed CR (condition register) can be confusing.
> The architecture only specifies that CR fields 1 to 7 (the 24 least
> significant bits) are checkpointed, but the kernel provides all 8
> fields (32 bits).  The value of field 0 is not masked by ptrace, so it
> will sometimes show the result of some kernel operation, probably
> treclaim., which sets this field.
> 
> The checkpointed registers are marked not to be saved and restored.
> Inferior function calls during an active transaction don't work well,
> and it's unclear what should be done in this case.  TEXASR and TFIAR
> can be altered asynchronously, during transaction failure recording,
> so they are also not saved and restored.  For consistency neither is
> TFHAR.
> 
> Record and replay also doesn't work well when transactions are
> involved.  This patch doesn't address this, so the values of the HTM
> SPRs will sometimes be innacurate when the record/relay target is
> enabled.  For instance, executing a "tbegin." alters TFHAR and TEXASR,
> but these changes are not currently recorded.
> 
> Because the checkpointed registers are only available when a
> transaction is active (or suspended), ptrace can return ENODATA when
> gdb tries to read these registers and the inferior is not in a
> transactional state.  The registers are set to the unavailable state
> when this happens.  When gbd tries to write to one of these registers,
> and it is unavailable, an error is raised.  When gdb tries to store to
> all registers in one go (when store_registers called with -1), the
> state of these registers is checked.  If they are all unavailable, no
> write is attempted, so that writes to all the other registers are
> unaffected.  If all are available, the write is attempted.  Otherwise
> an internal_error is raised.
> 
> The "fill" functions for checkpointed register sets in the server stub
> are not implemented for the same reason as for the EBB register set,
> since ptrace can also return ENODATA for checkpointed regsets.
> 
> Just like for the EBB registers, tracepoints will not mark the
> checkpointed registers as unavailable if the inferior was not in a
> transaction, so their content will also show 0 instead of
> <unavailable> when inspecting trace data.
> 
> The new tests record the values of the regular registers before
> stepping the inferior through a "tbegin." instruction to start a
> transaction, then the checkpointed registers are checked against the
> recorded pre-transactional values.  New values are written to the
> checkpointed registers and recorded, the inferior continues until the
> transaction aborts (which is usually immediately when it is resumed),
> and the regular registers are checked against the recorded values,
> because the abort should have reverted the registers to these values.

I'd find it useful to have an intro description like this last paragraph
above as a comment in the .exp file directly.  Likewise other
explanations throughout the series felt like would be useful in the code, but
I really haven't double checked to see if they were there already.

Speaking of comments, I noticed that the series adds a number of
functions and globals with no leading comment.

Thanks,
Pedro Alves