On Mon, Jan 19, 2015 at 9:17 AM, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> Add assertion checking when cpreg structures are registered that they
> either forbid raw-access attempts or at least make an attempt at
> handling them. Also add an assert in the raw-accessor-of-last-resort,
> to avoid silently doing a read or write from offset zero, which is
> actually AArch32 CPU register r0.
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> target-arm/helper.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 18f04b2..39c0ad9 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -119,6 +119,7 @@ static int aarch64_fpu_gdb_set_reg(CPUARMState *env,
> uint8_t *buf, int reg)
>
> static uint64_t raw_read(CPUARMState *env, const ARMCPRegInfo *ri)
> {
> + assert(ri->fieldoffset);
> if (cpreg_field_is_64bit(ri)) {
> return CPREG_FIELD64(env, ri);
> } else {
> @@ -129,6 +130,7 @@ static uint64_t raw_read(CPUARMState *env, const
> ARMCPRegInfo *ri)
> static void raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> + assert(ri->fieldoffset);
> if (cpreg_field_is_64bit(ri)) {
> CPREG_FIELD64(env, ri) = value;
> } else {
> @@ -174,6 +176,26 @@ static void write_raw_cp_reg(CPUARMState *env, const
> ARMCPRegInfo *ri,
> }
> }
>
> +static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
> +{
> + /* Return true if the regdef would cause an assertion if you called
> + *
>
> read_raw_cp_reg() or write_raw_cp_reg() on it (ie if it is a
> + * program bug for it not to have the NO_RAW flag).
> + * NB that returning false here doesn't necessarily mean that calling
> + * read/write_raw_cp_reg() is safe, because we can't distinguish "has
> + * read/write access functions which are safe for raw use" from "has
> + * read/write access functions which have side effects but has
> forgotten
> + * to provide raw access functions".
> + * The tests here line up with the conditions in
> read/write_raw_cp_reg()
> + * and assertions in raw_read()/raw_write().
> + */
> + if (ri->type & ARM_CP_CONST) {
> + return true;
> + }
>
Had to refresh my memory on this. It appears we changed the name
(polarity) of the function based on our previous discussion. However,
according to the above description, we should return 'true' if read/write
would cause an assertion. but in the case of CONST we would not assert, but
still return 'true'?
> + return (ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
> + (ri->raw_readfn || ri->readfn || ri->fieldoffset);
>
This case appears to need inverting as it will resolve to 'true' but
should be valid.
NIT: It would be cleaner to pull the ri->fieldoffset check above this check
to simplify the conditional.
> +}
> +
> bool write_cpustate_to_list(ARMCPU *cpu)
> {
> /* Write the coprocessor state from cpu->env to the (index,value)
> list. */
> @@ -3509,6 +3531,14 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
> const ARMCPRegInfo *r,
> r2->type |= ARM_CP_ALIAS;
> }
>
> + /* Check that raw accesses are either forbidden or handled. Note that
> + * we can't assert this earlier because the setup of fieldoffset for
> + * banked registers has to be done first.
> + */
> + if (!(r2->type & ARM_CP_NO_RAW)) {
> + assert(!raw_accessors_invalid(r2));
> + }
> +
> /* Overriding of an existing definition must be explicitly
> * requested.
> */
> --
> 1.9.1
>
>
