[2/2] target-arm: Add checks that cpreg raw accesses are handled

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.maydell@linaro.org>
---
 target-arm/helper.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

On 9 December 2014 at 13:46, Peter Maydell <peter.maydell@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.maydell@linaro.org>
> ---
>  target-arm/helper.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d1b856c..99a092c 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,18 @@ static void write_raw_cp_reg(CPUARMState *env, const
> ARMCPRegInfo *ri,
>      }
>  }
>
> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
> +{
> +    /* Return true if a raw access on this register is OK (ie will not
> +     * fall into the assert in raw_read() or raw_write())
> +     */
>

I believe this comment is somewhat misleading as there are registers that
would return true from this function yet still hit the aforementioned
asserts.


> +    if (ri->type & ARM_CP_CONST) {
> +        return true;
> +    }
>

I realize CONST registers would not likely go through the raw functions but
these too make the above comment incorrect.


> +    return (ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
> +        (ri->raw_readfn || ri->readfn || ri->fieldoffset);

+}
> +
>

Unless we are going to use this function elsewhere, wouldn't it be better
to just inline the code?  The name is otherwise misleading and the comment
may cause this to be incorrectly used elsewhere in the future.  Maybe
instead just update the call location with something like:

if (!(r2->type & (ARM_CP_NO_RAW | ARM_CP_CONST)) {
    assert((ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
           (ri->raw_readfn || ri->readfn || ri->fieldoffset);
}



>  bool write_cpustate_to_list(ARMCPU *cpu)
>  {
>      /* Write the coprocessor state from cpu->env to the (index,value)
> list. */
> @@ -3516,6 +3530,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_valid(r2));
> +    }
> +
>      /* Overriding of an existing definition must be explicitly
>       * requested.
>       */
> --
> 1.9.1
>
>
>

On 10 December 2014 at 22:26, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On 9 December 2014 at 13:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
>> +{
>> +    /* Return true if a raw access on this register is OK (ie will not
>> +     * fall into the assert in raw_read() or raw_write())
>> +     */
>
>
> I believe this comment is somewhat misleading as there are registers that
> would return true from this function yet still hit the aforementioned
> asserts.

Really? I think it is misleading (really it will return false if
a raw access is definitely not valid, but may return true even if
a raw access is still a bad idea), but I don't think there are any
cases that would return true and then hit the assert.

>>
>> +    if (ri->type & ARM_CP_CONST) {
>> +        return true;
>> +    }
>
>
> I realize CONST registers would not likely go through the raw functions but
> these too make the above comment incorrect.

No, read_raw_cp_reg and write_raw_cp_reg both handle CONST,
so it's correct to return true here.

>> +    return (ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
>> +        (ri->raw_readfn || ri->readfn || ri->fieldoffset);
>>
>> +}
>> +
>
>
> Unless we are going to use this function elsewhere, wouldn't it be better to
> just inline the code?  The name is otherwise misleading and the comment may
> cause this to be incorrectly used elsewhere in the future.  Maybe instead
> just update the call location with something like:
>
> if (!(r2->type & (ARM_CP_NO_RAW | ARM_CP_CONST)) {
>     assert((ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
>            (ri->raw_readfn || ri->readfn || ri->fieldoffset);
> }

I wanted to keep the logic checking for validity next to the logic
that it matches in read/write_raw_cp_reg(). Otherwise the two are
a long way apart in the file and it's not obvious why we check what
we do. (I guess it wasn't obvious anyway, so the comment needs work.)

-- PMM

On 10 December 2014 at 16:50, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 December 2014 at 22:26, Greg Bellows <greg.bellows@linaro.org>
> wrote:
> >
> >
> > On 9 December 2014 at 13:46, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
> >> +{
> >> +    /* Return true if a raw access on this register is OK (ie will not
> >> +     * fall into the assert in raw_read() or raw_write())
> >> +     */
> >
> >
> > I believe this comment is somewhat misleading as there are registers that
> > would return true from this function yet still hit the aforementioned
> > asserts.
>
> Really? I think it is misleading (really it will return false if
> a raw access is definitely not valid, but may return true even if
> a raw access is still a bad idea), but I don't think there are any
> cases that would return true and then hit the assert.
>
>
If you called the routine on PMCCNTR, for instance, this routine would
return true and if you then called raw_read or raw_write you would hit the
assert, correct?  This may be contrived, but I believe there are cases that
the comment is incorrect.


> >>
> >> +    if (ri->type & ARM_CP_CONST) {
> >> +        return true;
> >> +    }
> >
> >
> > I realize CONST registers would not likely go through the raw functions
> but
> > these too make the above comment incorrect.
>
> No, read_raw_cp_reg and write_raw_cp_reg both handle CONST,
> so it's correct to return true here.
>
> >> +    return (ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
> >> +        (ri->raw_readfn || ri->readfn || ri->fieldoffset);
> >>
> >> +}
> >> +
> >
> >
> > Unless we are going to use this function elsewhere, wouldn't it be
> better to
> > just inline the code?  The name is otherwise misleading and the comment
> may
> > cause this to be incorrectly used elsewhere in the future.  Maybe instead
> > just update the call location with something like:
> >
> > if (!(r2->type & (ARM_CP_NO_RAW | ARM_CP_CONST)) {
> >     assert((ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
> >            (ri->raw_readfn || ri->readfn || ri->fieldoffset);
> > }
>
> I wanted to keep the logic checking for validity next to the logic
> that it matches in read/write_raw_cp_reg(). Otherwise the two are
> a long way apart in the file and it's not obvious why we check what
> we do. (I guess it wasn't obvious anyway, so the comment needs work.)
>

I can go either way and understand the thought behind keeping the check
near the code. Given that the function is tied to those functions it may
just need to be more clear.  Maybe even call out which asserts you are
concerned about.


>
> -- PMM
>

On 10 December 2014 at 23:18, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On 10 December 2014 at 16:50, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 10 December 2014 at 22:26, Greg Bellows <greg.bellows@linaro.org>
>> wrote:
>> >
>> >
>> > On 9 December 2014 at 13:46, Peter Maydell <peter.maydell@linaro.org>
>> > wrote:
>> >> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
>> >> +{
>> >> +    /* Return true if a raw access on this register is OK (ie will not
>> >> +     * fall into the assert in raw_read() or raw_write())
>> >> +     */
>> >
>> >
>> > I believe this comment is somewhat misleading as there are registers
>> > that
>> > would return true from this function yet still hit the aforementioned
>> > asserts.
>>
>> Really? I think it is misleading (really it will return false if
>> a raw access is definitely not valid, but may return true even if
>> a raw access is still a bad idea), but I don't think there are any
>> cases that would return true and then hit the assert.
>>
>
> If you called the routine on PMCCNTR, for instance, this routine would
> return true and if you then called raw_read or raw_write you would hit the
> assert, correct?  This may be contrived, but I believe there are cases that
> the comment is incorrect.

Ah, I see the confusion. By 'raw access' in the comment I meant "a call
to read_raw_cp_reg/write_raw_cp_reg" -- doing that for PMCCNTR will
end up calling its read and write accessors, so we don't fall into
the raw_read() or raw_write() calls and won't hit the assert.

How about we invert the sense of the function and call it
raw_accessors_invalid(), and make the comment read:

   /* 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().
    */

?

-- PMM

On 11 December 2014 at 03:34, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 10 December 2014 at 23:18, Greg Bellows <greg.bellows@linaro.org>
> wrote:
> >
> >
> > On 10 December 2014 at 16:50, Peter Maydell <peter.maydell@linaro.org>
> > wrote:
> >>
> >> On 10 December 2014 at 22:26, Greg Bellows <greg.bellows@linaro.org>
> >> wrote:
> >> >
> >> >
> >> > On 9 December 2014 at 13:46, Peter Maydell <peter.maydell@linaro.org>
> >> > wrote:
> >> >> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
> >> >> +{
> >> >> +    /* Return true if a raw access on this register is OK (ie will
> not
> >> >> +     * fall into the assert in raw_read() or raw_write())
> >> >> +     */
> >> >
> >> >
> >> > I believe this comment is somewhat misleading as there are registers
> >> > that
> >> > would return true from this function yet still hit the aforementioned
> >> > asserts.
> >>
> >> Really? I think it is misleading (really it will return false if
> >> a raw access is definitely not valid, but may return true even if
> >> a raw access is still a bad idea), but I don't think there are any
> >> cases that would return true and then hit the assert.
> >>
> >
> > If you called the routine on PMCCNTR, for instance, this routine would
> > return true and if you then called raw_read or raw_write you would hit
> the
> > assert, correct?  This may be contrived, but I believe there are cases
> that
> > the comment is incorrect.
>
> Ah, I see the confusion. By 'raw access' in the comment I meant "a call
> to read_raw_cp_reg/write_raw_cp_reg" -- doing that for PMCCNTR will
> end up calling its read and write accessors, so we don't fall into
> the raw_read() or raw_write() calls and won't hit the assert.


> How about we invert the sense of the function and call it
> raw_accessors_invalid(), and make the comment read:
>
>    /* 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().
>     */
>
> ?
>
>
The comment is certainly clearer.  I'm not sure you need the NB, but there
is never harm in too much detail.


> -- PMM
>