| Message ID | 1421680645-14776-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 19, 2015 at 9:17 AM, 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 | 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 > >
On 19 January 2015 at 18:05, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On Mon, Jan 19, 2015 at 9:17 AM, Peter Maydell <peter.maydell@linaro.org> > wrote: >> + 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'? Doh. I inverted the name and polarity but forgot to change the function body. (I have no idea why that didn't blow up). Will fix (and test a bit more thoroughly...) >> >> + 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. That's deliberately matching the checks in the read/write_raw_cp_reg functions, but then I didn't do that for the CP_CONST check I guess. -- PMM
On Mon, Jan 19, 2015 at 1:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 January 2015 at 18:05, Greg Bellows <greg.bellows@linaro.org> wrote: > > > > > > On Mon, Jan 19, 2015 at 9:17 AM, Peter Maydell <peter.maydell@linaro.org > > > > wrote: > >> + 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'? > > Doh. I inverted the name and polarity but forgot to change the function > body. (I have no idea why that didn't blow up). Will fix (and test a > bit more thoroughly...) > > FYI, I actually ran the changes and they do unwantedly assert. > >> > >> + 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. > > That's deliberately matching the checks in the read/write_raw_cp_reg > functions, but then I didn't do that for the CP_CONST check I guess. > > -- PMM >
On 19 January 2015 at 19:05, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On Mon, Jan 19, 2015 at 1:03 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: >> Doh. I inverted the name and polarity but forgot to change the function >> body. (I have no idea why that didn't blow up). Will fix (and test a >> bit more thoroughly...) >> > > FYI, I actually ran the changes and they do unwantedly assert. I must have had one of those "build one config and run tests with an old binary" moments. Apologies... -- PMM
On Mon, Jan 19, 2015 at 1:07 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 January 2015 at 19:05, Greg Bellows <greg.bellows@linaro.org> wrote: > > > > > > On Mon, Jan 19, 2015 at 1:03 PM, Peter Maydell <peter.maydell@linaro.org > > > > wrote: > >> Doh. I inverted the name and polarity but forgot to change the function > >> body. (I have no idea why that didn't blow up). Will fix (and test a > >> bit more thoroughly...) > >> > > > > FYI, I actually ran the changes and they do unwantedly assert. > > I must have had one of those "build one config and run tests with > an old binary" moments. Apologies... > > -- PMM > No worries, just thought you should know.
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; + } + return (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. */ @@ -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. */
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 | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)