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

[Peter Maydell]

On 10 December 2014 at 23:18, Greg Bellows <address@hidden> wrote:
>
>
> On 10 December 2014 at 16:50, Peter Maydell <address@hidden>
> wrote:
>>
>> On 10 December 2014 at 22:26, Greg Bellows <address@hidden>
>> wrote:
>> >
>> >
>> > On 9 December 2014 at 13:46, Peter Maydell <address@hidden>
>> > 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